-
Notifications
You must be signed in to change notification settings - Fork 64
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
install: Update node label prefix #195
install: Update node label prefix #195
Conversation
@kartikjoshi21, thanks for the contribution and I'd like to have this one merged before the release. May I ask you to add the info you provided in the issue as part of the commit message? This allows folks taking a look at the git history to easily understand why a change was made without having to visit GitHub to do so. |
d8b599c
to
b537a2f
Compare
Thanks @fidencio, I Have updated the commit message. |
/test |
tests/e2e/cluster/up.sh
Outdated
@@ -58,7 +58,7 @@ main() { | |||
# Untaint the node so that pods can be scheduled on it. | |||
for role in master control-plane; do | |||
kubectl taint nodes "$(hostname)" \ | |||
"node-role.kubernetes.io/$role:NoSchedule-" | |||
"node.kubernetes.io/$role:NoSchedule-" |
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.
@kartikjoshi21 - the tests are getting an error:
error: taint "node.kubernetes.io/master:NoSchedule" not found
which I think is related to this change?
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 how I understand the situation:
- the operator CI deploys the test cluster with kubeadm, which labels the node as
node-role.kubernetes.io
. I will need to change the kubeadm configs to use the new label; - the operator CI for enclave-cc deploys the test cluster with kind, which already labels the node as
node.kubernetes.io
. Additionally the workflow script add thenode-role.kubernetes.io
label, that explains why the enclave-cc job didn't fail (?)
This PR fixes #194 that is a duplicated of #130 . On this last issue there is some discussion whether change or not the label. I am in favor of the change btw, but as we are about to release 0.5 and it has potential to break stuffs: @kartikjoshi21 mind if we post-pone this change to after the release? If yes then I will put the "do-not-merge" label on this PR.
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.
@wainersm I think we can postpone it, and CAA one which is related to this one as well. Meanwhile i have a workaround to label node to node-role.* which is already merged.
@stevenhorsman @wainersm @bpradipt What is the current state of this issue? Does it need more discussion? Would be nice if we could include a fix in the next release. |
Hey Paul, thanks for bumping this topic. IIRC there was a bit of risk to it so we postponed it as we were late in the release, but I think as we are early in the cycle if it's re-based and the tests pass then we can get it in and if there are issue still back it out without being too disruptive. @kartikjoshi21 - does that sound okay? |
56ecce8
to
93323d7
Compare
/test |
Test failing due to |
When I raised this previously Wainer said:
|
@wainersm any idea on how we could the CI passing for this PR? Could we temporarily add both labels to the CI kubeamd so both this PR and tests on main pass? |
hi @katexochen , sorry long delay to reply your msgs...
Here is the explanation: https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/create-cluster-kubeadm/#managed-node-labels I think we still need to untaint |
For Kubernetes version greater than or equal to 1.16, node label keys in the 'kubernetes.io' or 'k8s.io' namespace must begin with an allowed prefix (kubelet.kubernetes.io, node.kubernetes.io). Update node label from node-role.kubernetes.io to node.kubernetes.io Fixes: confidential-containers#194 Signed-off-by: Kartik Joshi <kartikjoshi@microsoft.com>
93323d7
to
56b4442
Compare
/test |
tests-e2e-ubuntu-20.04_snp-x86_64-containerd_kata-qemu-snp
|
SNP isn't supported at the moment, so that's an expected failure. |
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
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 @kartikjoshi21 and @katexochen!
Update node label from node-role.kubernetes.io to node.kubernetes.io
Fixes: #194