-
Notifications
You must be signed in to change notification settings - Fork 719
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
Update e2e test to cover upgrade using desired nodes API #7679
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thbkrkr
changed the title
Update upgrade e2e test with resources
Update e2e test to cover upgrade using desired nodes API
Mar 28, 2024
thbkrkr
commented
Mar 29, 2024
@@ -18,7 +18,7 @@ const ( | |||
// LatestReleasedVersion7x is the latest released version for 7.x | |||
LatestReleasedVersion7x = "7.17.8" | |||
// LatestReleasedVersion8x is the latest release version for 8.x | |||
LatestReleasedVersion8x = "8.12.1" | |||
LatestReleasedVersion8x = "8.13.0" |
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.
Same change in #7661.
pebrc
approved these changes
Apr 2, 2024
@@ -182,13 +185,22 @@ func TestVersionUpgradeSingleToLatest8x(t *testing.T) { | |||
|
|||
test.SkipInvalidUpgrade(t, srcVersion, dstVersion) | |||
|
|||
resources := corev1.ResourceRequirements{ |
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.
Maybe add a comment explaining your motivation for adding the explicit resource requirement.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates
TestVersionUpgradeSingleToLatest8x
to use CPU requests and memory limits, so the desired nodes API is used during an upgrade.Why update juste one test? I think there are two reasons why we don't set CPU resources by default for all tests:
Relates to #7664.
This first updates the test:
For testing, I reverted the fix to validate that this change in the test effectively fails without the #7663 fix:
🟡 The build succeeded instead of failing? Normal, the test was not doing an ES upgrade to
8.13.0
. Let's update the latest 8x. (Not smart, I could have simply sett=8.13.0
when triggering the test.)🔴 Now the build failed and we have the expected error (
elasticsearch client failed - [update_desired_nodes_request] failed to parse field [nodes]
) in the operator log in the diagnostics.Reverting the revert to now validate that the fix already comitted fixes the new test:
🟢 The build succeeded.