-
Notifications
You must be signed in to change notification settings - Fork 256
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
🌱 Add BMO e2e upgrade test #1451
Conversation
95c2149
to
fa9dc42
Compare
/metal3-bmo-e2e-test |
1 similar comment
/metal3-bmo-e2e-test |
545e205
to
216772a
Compare
cd4830e
to
ad37ef2
Compare
d1215ff
to
9336c5b
Compare
/metal3-bmo-e2e-test |
/override test-centos-e2e-integration-main |
@mquhuy: mquhuy unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/metal3-bmo-e2e-test |
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.
Good progress!
I hope all my comments are not discouraging!
b96c192
to
e52d63d
Compare
/metal3-bmo-e2e-test |
e52d63d
to
57d6593
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.
This looks pretty good now!
Could you set up a pipeline for this upgrade job also? Then we could test it from this PR before we merge 🙂
test/e2e/config/ironic-upgrade.yaml
Outdated
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.
Not sure if it is worth it to have a separate file for this. Is it more than just the *DEPLOY_*
variables that differ?
If it is just them I think we can just override them in ci-e2e.sh
(e.g. export DEPLOY_BMO=false
).
437df13
to
bb0d40e
Compare
Sure. I added the pipeline in these commits: And I updated this PR with your suggested changes: the two jobs are now differentiated by |
hack/ci-e2e.sh
Outdated
*) | ||
echo "'upgrade' not found in env var GINKGO_FOCUS. Will NOT run upgrade tests." | ||
export GINKGO_SKIP="upgrade ${GINKGO_SKIP:-}" | ||
;; |
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.
The less logic we have here the better. If someone wants to try to run upgrade together with the rest of the tests that is allowed. Maybe they have a configuration that works?
*) | |
echo "'upgrade' not found in env var GINKGO_FOCUS. Will NOT run upgrade tests." | |
export GINKGO_SKIP="upgrade ${GINKGO_SKIP:-}" | |
;; |
hack/ci-e2e.sh
Outdated
@@ -142,6 +157,14 @@ echo "${IRONIC_PASSWORD}" > "${BMO_OVERLAY}/ironic-password" | |||
echo "${IRONIC_INSPECTOR_USERNAME}" > "${BMO_OVERLAY}/ironic-inspector-username" | |||
echo "${IRONIC_INSPECTOR_PASSWORD}" > "${BMO_OVERLAY}/ironic-inspector-password" | |||
|
|||
if [[ "${GINKGO_FOCUS:-}" == "upgrade" ]]; then |
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.
if [[ "${GINKGO_FOCUS:-}" == "upgrade" ]]; then | |
if [[ "${GINKGO_FOCUS:-}" == *"upgrade"* ]]; then |
I'm not sure it is really worth it to have the if statement here at all. It does not cost much to add the passwords regardless if we use them or not.
hack/ci-e2e.sh
Outdated
echo "'upgrade' found in env var GINKGO_FOCUS. Will ONLY run upgrade tests." | ||
export GINKGO_FOCUS="upgrade" |
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.
We should not change the variable. If someone has a reason to set this to something more than just upgrade, we just accept it.
echo "'upgrade' found in env var GINKGO_FOCUS. Will ONLY run upgrade tests." | |
export GINKGO_FOCUS="upgrade" | |
echo "'upgrade' found in env var GINKGO_FOCUS. Will ONLY deploy 'upgrade from' versions of BMO and Ironic." |
f48d684
to
90e3930
Compare
/metal3-bmo-e2e-test |
1 similar comment
/metal3-bmo-e2e-test |
90e3930
to
bcd28f5
Compare
Signed-off-by: Huy Mai <huy.mai@est.tech>
bcd28f5
to
a252707
Compare
/metal3-bmo-e2e-test |
2 similar comments
/metal3-bmo-e2e-test |
/metal3-bmo-e2e-test |
/metal3-bmo-e2e-optional-test |
/metal3-bmo-e2e-test |
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.
Awesome job @mquhuy !
Thanks for the patience through all those comments 😅
/lgtm
/cc @kashifest |
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.
Few questions/comments in line
@@ -0,0 +1,10 @@ | |||
HTTP_PORT=6180 |
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.
The name of the directory could be ironic-release-0.4 or e2e-ironic-release-0.4, otherwise having e2e- in one and fixture- in another seems a bit out of convention.
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.
The name of these directories are to aligned with the equivalent from main
: fixtures
and e2e
. I think your naming convention makes senses, but do you think we should change these original names (fixtures
and e2e
) as well? What about you @lentzi90 ?
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.
I don't have a strong preference. It looks alright like this to me.
My reasoning when I named the folders e2e
and fixture
is that they are used for the e2e and fixture tests respectively. So I think it also makes sense then with e2e-release-04 and fixture-release-0.4
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.
Ok. I dont have a strong opinion either. This is certainly not a blocker for the tests to go in.
/approve
@@ -0,0 +1,24 @@ | |||
apiVersion: kustomize.config.k8s.io/v1beta1 |
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 comment, the name of the directory could be e2e-fixture-release-0.4 or we can keep it as it is and then add ironic- in the other directory name
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.
My answer to this one would be the same as in the other kustomization, so let's continue the discussion there.
case "${GINKGO_FOCUS:-}" in | ||
*upgrade*) | ||
export DEPLOY_IRONIC="false" | ||
export DEPLOY_BMO="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.
I dont understand this, by default we dont deploy BMO even in upgrade test?
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.
The upgrade test has its own logic to install all these components, and they follow the UPGRADE_DEPLOY_*
configs instead. The reason for that is because we use a different BMO version for upgrade test than the rest of the suite.
In the fixtures
setup, we use two separate clusters for this purpose, so we could run all tests in the same run, but in ci-e2e.sh
, we have only 1 cluster, so if we run upgrade test, we don't install anything in the BeforeSuite and leave these installations to the test setup.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test-centos-e2e-integration-main |
Fixes #1430