-
Notifications
You must be signed in to change notification settings - Fork 1.9k
WIP: Add baremetalds-e2e-upgrade-workflow #12779
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
Conversation
Skipping CI for Draft Pull Request. |
/assign @andfasano @vrutkovs |
Signed-off-by: Eran Israeli <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eisraeli The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
test: | ||
- ref: baremetalds-e2e-conf | ||
- ref: baremetalds-e2e-test | ||
- ref: baremetalds-e2e-upgrade-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.
Splitting the upgrade step from the test step will cause an unnecessary tests execution during the first e2e-test
step (which is something already exercised in the standard workflow), thus increasing also the overall time for the job. It would be preferred to merge the two steps together with a conditional execution (in a way similar to the other profiles) here:
release/ci-operator/step-registry/baremetalds/e2e/test/baremetalds-e2e-test-commands.sh
Line 41 in 8cb0233
ssh "${SSHOPTS[@]}" "root@${IP}" openshift-tests run "openshift/conformance/parallel" --dry-run \| grep -Ff /tmp/test-list \|openshift-tests run -o /tmp/artifacts/e2e.log --junit-dir /tmp/artifacts/junit -f - |
OPENSHIFT_UPGRADE_RELEASE_IMAGE_OVERRIDE: "release:latest" | ||
env: | ||
DEVSCRIPTS_CONFIG: | | ||
OPENSHIFT_RELEASE_IMAGE=$OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE |
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 think this kind of indirection is supported. In any case it should not be necessary to set DEVSCRIPTS_CONFIG but I think it would be sufficient something like dependencies: RELEASE_IMAGE_LATEST: "release:initial"
(see
Line 94 in 8cb0233
echo "export OPENSHIFT_RELEASE_IMAGE=${RELEASE_IMAGE_LATEST}" >> /root/dev-scripts/config_root.sh |
set -o errexit | ||
set -o pipefail | ||
|
||
openshift-tests run-upgrade --to-image "${OPENSHIFT_UPGRADE_RELEASE_IMAGE_OVERRIDE}" --dry-run 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.
This unfortunately cannot work, since the openshift-tests
binary must be copied on the remote Packet instance where the cluster is currently running. In addition, tests results must be copied back so that they could be published in the job dashboard by the CI.
The baremetalds-e2e-test
it's already taking care of all these task, it would be sufficient to make the openshift-tests
command here (
release/ci-operator/step-registry/baremetalds/e2e/test/baremetalds-e2e-test-commands.sh
Line 41 in 8cb0233
ssh "${SSHOPTS[@]}" "root@${IP}" openshift-tests run "openshift/conformance/parallel" --dry-run \| grep -Ff /tmp/test-list \|openshift-tests run -o /tmp/artifacts/e2e.log --junit-dir /tmp/artifacts/junit -f - |
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.
@andfasano From what I've seen, the openshift-tests
binary is copied to the remote Packet instance on the baremetal-e2e-test
step.
See:
release/ci-operator/step-registry/baremetalds/e2e/test/baremetalds-e2e-test-commands.sh
Line 33 in 8cb0233
scp "${SSHOPTS[@]}" /usr/bin/openshift-tests /usr/bin/kubectl "root@${IP}:/usr/local/bin" |
So therefore, the openshift-tests
binary is available for the upgrade step is the test step is executed beforehand.
We can either merge those 2 steps (as you've suggested) or make sure the upgrade step handles copies the 'openshift-tests` binary itself.
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.
Perhaps we could mimic vSphere approach here. There is a permanent cluster running on vSphere, which reuses the platform to create more clusters. In that case ssh/scp is not necessary as permanent cluster can reach the new one directly via KUBECONFIG.
WDYT?
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'd prefer to merge it within the existing and already working baremetalds-e2e-test
step. Changes we'll be really minimal, and the step it's already taking care of all the rest. In addition, this will let us quickly go on and focus on the openshift-tests
upgrade tests execution itself, which could present some additional difficulties (hopefully not) for the baremetal platform.
@eisraeli: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@eisraeli: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@eisraeli: PR needs rebase. 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. |
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 is going to be IPv4-only, right? Is it possible for us to also create an OVN variant to ensure IPv4 OVN upgrades are tested?
It looks like #13120 supersedes this. Let's keep just one PR open at a time... /close |
@stbenjam: Closed this PR. 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. |
https://issues.redhat.com/browse/KNIDEPLOY-3733
Signed-off-by: Eran Israeli [email protected]