-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fixed LogsIndexModeRollingUpgradeIT #133774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed LogsIndexModeRollingUpgradeIT #133774
Conversation
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
||
// then continued - verify that only the latest write index has logsdb enabled | ||
assertIndexSettings(0, Matchers.nullValue()); | ||
assertIndexSettings(1, Matchers.nullValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we no longer rollover when the cluster is in a mixed state, we have only two indices at the end
|
||
// then - ensure no issues | ||
assertOK(response); | ||
assertThat("errors in response:\n " + responseBody, responseBody.get("errors"), Matchers.is(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printing the error in response here makes finding the errors significantly easier. Previously, I had to log dive to understand what happened
/** | ||
* This test starts with LogsDB disabled, performs an upgrade, enables LogsDB and indexes some documents. | ||
*/ | ||
public class LogsIndexModeRollingUpgradeIT extends AbstractRollingUpgradeWithSecurityTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped AbstractRollingUpgradeTestCase
for AbstractRollingUpgradeWithSecurityTestCase
since this test has auth enabled.
947b5f6
to
337ddc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
* Fixed LogsIndexModeRollingUpgradeIT * Renamed test class
This is a follow up for #133493 and #133562.
This resolves #133543.
There a series of problems with this test case, namely:
OLD_CLUSTER_VERSION
, meaning the cluster upgrade just upgrades from newest version to newest versiongetUpgradeCluster()
), meaning we upgrade the base cluster, defined in the parent class, instead of the correct oneThis PR addresses all of these problems + refactors the class to be cleaner and more readable.
Note, even if this test were setup correctly, it would not have caught the norms bwc bug. This is because that bug is triggered on existing logsdb indices during upgrade. In this case however, we only enable logsdb once the upgrade is completed. There is already a test that verifies this, its
LogsdbIndexingRollingUpgradeIT
.To verify that the changes in this PR are effective, I enabled logsdb early and reverted my change in #133493. The result is the following error, as expected: