-
Notifications
You must be signed in to change notification settings - Fork 247
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 Upgrade test: Add more BMO versions to upgrade from #1523
🌱 E2E Upgrade test: Add more BMO versions to upgrade from #1523
Conversation
/metal3-bmo-e2e-test |
/metal3-bmo-e2e-optional-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.
This looks good already, but I have one request: would it be possible to make it visible in the logs what version we test?
Maybe just printing the exact overlay that is used? We could also print the image tag or something like that.
4f3d2fb
to
16e4c8d
Compare
b426eec
to
cd03d66
Compare
Thanks Lennart. Sorry it took a bit long, I almost forgot about it. |
/test metal3-bmo-e2e-optional-test |
@mquhuy: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
/test metal3-bmo-e2e-test-optional-pull |
test/e2e/upgrade_test.go
Outdated
bmoKustomization := e2eConfig.GetVariable("UPGRADE_BMO_KUSTOMIZATION_FROM") | ||
// Get the index of the last "release" word, which typically indicates the start of the BMO version name | ||
lastReleaseWordIndex := strings.LastIndex(bmoKustomization, "release") | ||
BmoFromVer := bmoKustomization[lastReleaseWordIndex:] | ||
By(fmt.Sprintf("Installing BMO with version: %s on the upgrade cluster", BmoFromVer)) |
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 basically adds a requirement on the kustomization folder name. I would rather have something simpler that doesn't care what you name them. Perhaps just print the path?
bmoKustomization := e2eConfig.GetVariable("UPGRADE_BMO_KUSTOMIZATION_FROM") | |
// Get the index of the last "release" word, which typically indicates the start of the BMO version name | |
lastReleaseWordIndex := strings.LastIndex(bmoKustomization, "release") | |
BmoFromVer := bmoKustomization[lastReleaseWordIndex:] | |
By(fmt.Sprintf("Installing BMO with version: %s on the upgrade cluster", BmoFromVer)) | |
bmoKustomization := e2eConfig.GetVariable("UPGRADE_BMO_KUSTOMIZATION_FROM") | |
By(fmt.Sprintf("Installing BMO from %s on the upgrade cluster", bmoKustomization)) |
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.
Hmm. Okay. How about using the Base path instead? Having a full path in the log is fine, but I also want to have a nice way to notice it in the artifacts.
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.
Oh I didn't notice it was used for the artifacts. Thanks for pointing it out!
I think the base path sounds good for artifacts, in the log I would say the more we include the better :D
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.
Sounds good. I'll update it real quick
14b9aad
to
f2aed3e
Compare
Signed-off-by: Huy Mai <huy.mai@est.tech>
f2aed3e
to
4c94b2b
Compare
/test metal3-bmo-e2e-test-pull |
/test metal3-bmo-e2e-test-optional-pull |
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
Thanks for this!
I think that it would be nice eventually to refactor it so that we can run the upgrade test for all different versions, one after the other. But let's do that in a follow up.
The reason I think it would be nice is to reduce the number of jobs so it is easier to keep track of them and also to understand what we test.
Thank you. Yes, I was thinking about that idea the other day. Having a separate job for every upgrade test is too much to handle, but the "problem" with running them in sequence is that to run any upgrade test (including ironic upgrade), the cluster should be cleaned up to its initial state (no ironic nor bmo), so we need to test if cleaning up a cluster is reliable (i.e. if the cluster is really "clean" afterwards). I guess it needs more investigating, and I might start doing it together with the ironic upgrade test. |
/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.
Does the inspector config here work with the #1544 ?
CACHEURL=http://192.168.222.199/images | ||
IRONIC_FAST_TRACK=true | ||
IRONIC_KERNEL_PARAMS=console=ttyS0 | ||
IRONIC_INSPECTOR_VLAN_INTERFACES=all |
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.
Will this consume ironic-image from main? If so, you need to set USE_IRONIC_INSPECTOR=true
.
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.
Yes, we use the same ironic image for all jobs currently and that is the latest. Please add it @mquhuy
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.
Wait a second, this is for BMO. In #1544 you did not touch those, do we need to?
- BMO: https://github.com/metal3-io/baremetal-operator/blob/main/config/overlays/e2e/ironic.env
- Ironic: https://github.com/metal3-io/baremetal-operator/blob/main/ironic-deployment/overlays/e2e/ironic_bmo_configmap.env
Does BMO need to have the variable or is it only relevant for Ironic?
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.
BMO requires Inspector before 0.5.0. Does it answer your question?
CACHEURL=http://192.168.222.199/images | ||
IRONIC_FAST_TRACK=true | ||
IRONIC_KERNEL_PARAMS=console=ttyS0 | ||
IRONIC_INSPECTOR_VLAN_INTERFACES=all |
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 re INSPECTOR
@tuminoid I cannot say for sure, but if inspector removal doesn't work with these kustomization, then it will fail on e2e as well. I think we can merge this PR, and fix everything later when/if it fails. |
/approve |
[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 |
This PR adds more BMO config versions to the E2E upgrade tests, so that we can test from either of
release-0.3
,release-0.4
andrelease-0.5
tomain
(with bothironic
andfixture
config).It also changes the default "from" version from
release-0.4
torelease-0.5
.e.g. To run upgrade test from
release-0.3
tomain
withironic
config:Edit: The upgrade-from version can now be seen from the test output:
And from the logs