-
Notifications
You must be signed in to change notification settings - Fork 717
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
[wip]remove the KubeletVersion from ignorePreflightErrors for v1.29 #2944
Conversation
@pacoxu: GitHub didn't allow me to request PR reviews from the following users: lalitc375. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacoxu 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 |
…t as skew policy is now n-3
e953d19
to
e7452d4
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.
/hold for review
kinder/ci/tools/update-workflows/templates/workflows/skew-kubelet-x-on-y.yaml
Show resolved
Hide resolved
ignorePreflightErrors: "KubeletVersion" | ||
ignorePreflightErrors: "{{.IgnorePreflightErrors}}" |
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.
can we just remove this old line:
ignorePreflightErrors: "KubeletVersion"
i don't remember why we added it.
the kubelet version should match the k8s version and the k8s version is N-1 of the kubeadm version.
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.
kubeadm/kinder/ci/tools/update-workflows/templates/workflows/skew-x-on-y-tasks.yaml
Lines 68 to 69 in e7452d4
- --ignore-preflight-errors={{ .vars.defaultIgnorePreflightErrors }}{{ .vars.ignorePreflightErrors }} | |
- --loglevel=debug |
We will appendignorePreflightErrors
after the defaultIgnorePreflightErrors
.
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 can remove ignorePreflightErrors
suport and add KubeletVersion
to the defaultIgnorePreflightErrors
list then. And remove KubeletVersion
from defaultIgnorePreflightErrors
in v1.32.
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 see what the problem here is.
this file:
https://github.com/kubernetes/kubeadm/blob/e7452d437493fc9345cd2e243d7fba960af71fdb/kinder/ci/tools/update-workflows/templates/workflows/skew-kubelet-x-on-y.yaml
imports:
skew-x-on-y-tasks.yaml
so if you want to make changes to the kubelet skew it must be in skew-kubelet-x-on-y.yaml, not in skew-x-on-y-tasks.yaml. skew-x-on-y-tasks.yaml also applies to the kubeadm / control plane skew workflows.
if you add variables in skew-kubelet-x-on-y.yaml, they will act as overrides.
then again, this
kubeadm/kinder/ci/tools/update-workflows/templates/workflows/skew-kubelet-x-on-y.yaml
Line 10 in e7452d4
ignorePreflightErrors: "{{.IgnorePreflightErrors}}" |
ignorePreflightErrors: KubeletVersion |
so as long as all config entries have the KubeletVersion preflight error ignored, it will work.
and there is no need to change skew-kubelet-x-on-y.yaml or skew-x-on-y-tasks.yaml.
does this make sense to you?
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.
what is the status of this @pacoxu ?
if you don't have time i can send the test / docs prs?
2357afc
to
e7452d4
Compare
I would suggest just changing the comments only in v1.29 if we want to keep it simple for maintenance. I opened #2950. |
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.
@pacoxu
test freeze is in two days.. you are probably busy with other things.
i will close this PR, send a new PR from me and ping you for review there.
Thanks @neolit123 |
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.
@pacoxu so after looking at this change and our comments from earlier again, we don't need to do changes to the tests. you already added the doc changes, those are ok.
we established that we can continue passing --ignore-preflight-errors=KubeletVesion
even in cases where it's not needed.
that was because we don't want to update the config.yaml every release to target only specific versions with --ignore-preflight-errors=KubeletVesion
, this is annoying. instead we can just leave the test jobs they way they are and in 1 year when all supported kubeadm versions supported the new skew drop the --ignore-preflight-errors=KubeletVesion
for all of the jobs.
/cc @neolit123 @lalitc375
xref #2924