-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 periodic job for OCP with realtime workers #7154
Add periodic job for OCP with realtime workers #7154
Conversation
0b71d9a
to
f440c87
Compare
Nice, it seems to work :) In the
But I also see 5 failed tests... are those flaky, or can it be a real issue? |
/test ci/rehearse/release-openshift-ocp-installer-e2e-gcp-rt-4.4 |
/test pj-rehearse |
2nd run had different failures, so I assume they are flakes. |
/test all |
/test pj-rehearse |
looking at other jobs, the |
/approve |
/assign @smarterclayton |
ping @smarterclayton, can I get a review please :) |
openshift-install --dir=/tmp/artifacts/installer/ create manifests | ||
echo "${CLUSTER_NETWORK_MANIFEST}" > /tmp/artifacts/installer/manifests/cluster-network-03-config.yml | ||
|
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.
nit: this should probably be flattened out:
if [[ -n "${CLUSTER_NETWORK_MANIFEST:-}" ]] || has_variant "rt"; then
openshift-install --dir=/tmp/artifacts/installer/ create manifests
fi
if [[ -n "${CLUSTER_NETWORK_MANIFEST:-}" ]]; then
echo ...
fi
if has_variant "rt"; then
cat > ...
fi
or at least untangled:
if [[ -n "${CLUSTER_NETWORK_MANIFEST:-}" ]]; then
openshift-install...
echo ...
fi
if has_variant "rt"; then
openshift-install...
cat >...
fi
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.
Thanks for your review!
About your 1st suggestion: not sure, doesn't it make more sense to deal with both features which need to modify manifests inside the if
branch which creates them?
About the 2nd: that might probably break if both features are used simultaneously, it would call create manifests
twice.
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, keep the variant separate. CLUSTER_NETWORK_MANIFEST is a highly specialized use case and support is limited.
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.
split them, and then insight has_variant "rt" have an error condition that fails if CLUSTER_NETWORK_MANIFEST is set:
if has_variant "rt"; then
if [[ -n "{CLUSTER_NETWORK_MANIFEST:-}" ]]; then
echo 'error: CLUSTER_NETWORK_MANIFEST is incompatible with the `rt` variant'
exit 1
fi
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, done
/lgtm |
cd21e2b
to
4199163
Compare
/test pj-rehearse |
- mountPath: /usr/local/pull-secret | ||
name: pull-secret | ||
- mountPath: /usr/local/pull-secret | ||
name: release-pull-secret |
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 think you got bit by a rebase issue, you should just have release-pull-secret, rename all references to pull-secret in this PR to release-pull-secret and drop the dupes. There was a rename here 91ef459
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 ha, thanks, fixed
24f3532
to
fc3cfd1
Compare
The e2e-gcp-rt-4.4 lane looks good, and surprisingly also the e2e-gcp-rt-4.5 lane. |
For this a new CLUSTER_VARIANT "rt" is introduced
fc3cfd1
to
d83c779
Compare
/test app-ci-config |
/test pj-rehearse |
|
/test all |
/test pj-rehearse |
/retest |
|
Signed-off-by: Marc Sluiter <msluiter@redhat.com>
4cc8ac0
to
05e0471
Compare
/test pj-rehearse |
/lgtm the rt-4.5 job appeared to boot into the right worker kernel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jstuever, slintes, smarterclayton 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 |
@slintes: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
Recently a new feature was introduced for
MachineConfig
s to install a realtime kernel on nodes, see [0] and [1]. To detect regressions with the rt kernel early, this PR introduces a new periodic job.Todos (AFAIK):
CLUSTER_VARIANT
"rt"
The job needs to run on GCP, as the rt kernel does not work on all AWS machine types.cluster-launch-installer-e2e.yaml
release-ocp-4.4.json
[0] openshift/enhancements#166
[1] openshift/machine-config-operator#1330
FYI @MarSik