Skip to content

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

Closed
wants to merge 1 commit into from
Closed

WIP: Add baremetalds-e2e-upgrade-workflow #12779

wants to merge 1 commit into from

Conversation

eisraeli
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2020
@openshift-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@eisraeli
Copy link
Contributor Author

/assign @andfasano @vrutkovs

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eisraeli
To complete the pull request process, please assign andfasano after the PR has been reviewed.
You can assign the PR to them by writing /assign @andfasano in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

test:
- ref: baremetalds-e2e-conf
- ref: baremetalds-e2e-test
- ref: baremetalds-e2e-upgrade-upgrade
Copy link
Contributor

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:

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
Copy link
Contributor

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

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
Copy link
Contributor

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 (

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 -
) conditional

Copy link
Contributor Author

@eisraeli eisraeli Oct 14, 2020

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:

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.

Copy link
Member

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?

Copy link
Contributor

@andfasano andfasano Oct 14, 2020

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.

@openshift-ci-robot
Copy link
Contributor

@eisraeli: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/rehearse/openshift-metal3/dev-scripts/master/e2e-metal-ipi-upgrade e44f5e7 link /test pj-rehearse
ci/prow/pj-rehearse e44f5e7 link /test pj-rehearse

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.

@gbenhaim
Copy link
Member

I couldn't update @eisraeli PR so I've created a new one #13120

@openshift-merge-robot
Copy link
Contributor

@eisraeli: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/release-config e44f5e7 link /test release-config
ci/prow/boskos-config-generation e44f5e7 link /test boskos-config-generation
ci/prow/secret-generator-config-valid e44f5e7 link /test secret-generator-config-valid

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.

@openshift-ci-robot
Copy link
Contributor

@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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2020
Copy link
Member

@stbenjam stbenjam left a 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?

@stbenjam
Copy link
Member

stbenjam commented Nov 4, 2020

It looks like #13120 supersedes this. Let's keep just one PR open at a time...

/close

@openshift-ci-robot
Copy link
Contributor

@stbenjam: Closed this PR.

In response to this:

It looks like #13120 supersedes this. Let's keep just one PR open at a time...

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants