Skip to content
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

Merged

Conversation

kartikjoshi21
Copy link
Contributor

Update node label from node-role.kubernetes.io to node.kubernetes.io

Fixes: #194

@fidencio
Copy link
Member

fidencio commented Apr 6, 2023

@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?
"""
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).
"""

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.

@kartikjoshi21
Copy link
Contributor Author

@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? """ 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). """

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.

Thanks @fidencio, I Have updated the commit message.

@stevenhorsman
Copy link
Member

/test

@@ -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-"
Copy link
Member

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?

Copy link
Member

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 the node-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.

Copy link
Contributor Author

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.

@katexochen
Copy link

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

@stevenhorsman
Copy link
Member

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

@stevenhorsman
Copy link
Member

/test

@kartikjoshi21
Copy link
Contributor Author

Test failing due to error: taint "node.kubernetes.io/master:NoSchedule" not found

@stevenhorsman
Copy link
Member

Test failing due to error: taint "node.kubernetes.io/master:NoSchedule" not found

When I raised this previously Wainer said:

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;

@katexochen
Copy link

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

@wainersm
Copy link
Member

wainersm commented Jul 7, 2023

hi @katexochen , sorry long delay to reply your msgs...

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

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 node-role.kubernetes.io but also ensure that node.kubernetes.io exists on the worker node.

tests/e2e/cluster/up.sh Outdated Show resolved Hide resolved
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>
@stevenhorsman
Copy link
Member

/test

@katexochen
Copy link

tests-e2e-ubuntu-20.04_snp-x86_64-containerd_kata-qemu-snp

11:30:14 Cloning into '/tmp/tmp.nWm0FY5wVU/src/github.com/kata-containers/tests'...
11:30:16 ERROR: no known tests for runtime class kata-qemu-snp

@stevenhorsman
Copy link
Member

tests-e2e-ubuntu-20.04_snp-x86_64-containerd_kata-qemu-snp

11:30:14 Cloning into '/tmp/tmp.nWm0FY5wVU/src/github.com/kata-containers/tests'...
11:30:16 ERROR: no known tests for runtime class kata-qemu-snp

SNP isn't supported at the moment, so that's an expected failure.

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

Copy link
Member

@fidencio fidencio left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update node label prefix
5 participants