-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
e2e: only expect cluster's major version is > 3 in release upgrade test #11266
Conversation
CI test failed due to #11264 |
@gyuho Could you help take a look? |
@YoyinZyc Can you fix
? |
This failure is due to the upgrade issue #11264. |
To make the test pass, we need to merge #11275, release 3.4.2, and update MANUAL_VER to 3.4.2 here: etcd/tests/semaphore.test.bash Line 11 in 9c48dfa
Is my understanding correct? |
Yep. The test will pass without setting MANUAL_VER to 3.4.2. But since we have already released 3.4.2, we should update it. It seems that there are couple of things we need to update manually when we want to release or upgrade to new version. Do we have a checklist or test for that? |
@YoyinZyc Then, let's finish up the fix in master branch. Then, I can release a new patch release, and fix the test script. |
5544c02
to
a44eb1c
Compare
} | ||
if err := cURLGet(cx.epc, cURLReq{endpoint: "/metrics", expected: fmt.Sprintf(`etcd_cluster_version{cluster_version="%s"} 1`, ver), metricsURLScheme: cx.cfg.metricsURLScheme}); err != nil { |
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.
Prior to your change, the test was using the /version endpoint to check the version before upgrade then use the /metrics endpoint to check the version afterward. I think it's better with your change that we check the version with the consistent way. But I am just curious, @gyuho is there specific reason that we previous check the /metrics endpoint afterward? Just want to make sure we are not missing testing anything.
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.
@wenjiaswe I think either /version
or /metrics
is fine, since they are all set by the same constant (version.Version
). Since this is just upgrade test, using /version
makes more sense.
a44eb1c
to
cab4cac
Compare
Can we merge the test fix since we have already released 3.4.3 ? |
Codecov Report
@@ Coverage Diff @@
## master #11266 +/- ##
==========================================
+ Coverage 64.11% 64.78% +0.66%
==========================================
Files 403 403
Lines 37973 37973
==========================================
+ Hits 24348 24599 +251
+ Misses 11995 11742 -253
- Partials 1630 1632 +2
Continue to review full report at Codecov.
|
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
Fix e2e test
TestReleaseUpgrade
.The test will skip only when the cluster version is <3.0.