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.

Sign up for GitHub

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

Jump to bottom

⚠️ apply node.cluster.x-k8s.io/uninitialized during machine creation #7993

Merged
merged 1 commit into from
Mar 8, 2023
Merged

⚠️ apply node.cluster.x-k8s.io/uninitialized during machine creation #7993

merged 1 commit into from
Mar 8, 2023

Conversation

ykakarap
Copy link
Contributor

@ykakarap ykakarap commented Jan 25, 2023 •
edited
Loading

What this PR does / why we need it:

This PR adds the taint logic solve the delay problem in node label sync.
Does the following

  • Add the node.cluster.x-k8s.io/uninitialized:NoSchedule taint to node at creation
  • Drop the taint after the labels are synced from the Machine to the Node for the first time.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Part of #7730

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2023
@ykakarap ykakarap mentioned this pull request Jan 25, 2023
Closed
4 tasks
@ykakarap ykakarap force-pushed the node-label-sync-taint branch from 2a08de5 to 13927b3 Compare January 26, 2023 00:28
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2023
@ykakarap
Copy link
Contributor Author

/milestone v1.4

@k8s-ci-robot
Copy link
Contributor

@ykakarap: You must be a member of the kubernetes-sigs/cluster-api-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.4

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.

@sbueringer sbueringer added this to the v1.4 milestone Jan 26, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 1, 2023
sbueringer
sbueringer reviewed Feb 7, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Looks good overall

api/v1beta1/common_types.go Outdated Show resolved Hide resolved
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go Show resolved Hide resolved
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go Outdated Show resolved Hide resolved
internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved
internal/controllers/machine/machine_controller_noderef_test.go Show resolved Hide resolved
internal/controllers/machine/machine_controller_noderef_test.go Outdated Show resolved Hide resolved
internal/controllers/machine/machine_controller_noderef_test.go Show resolved Hide resolved
@ykakarap ykakarap force-pushed the node-label-sync-taint branch from e7fe3e1 to eb18a8e Compare March 2, 2023 06:37
@ykakarap ykakarap changed the title ✨ [DO_NOT_REVEW][WIP] apply node.cluster.x-k8s.io/uninitialized during machine creation ✨ apply node.cluster.x-k8s.io/uninitialized during machine creation Mar 2, 2023
@ykakarap
Copy link
Contributor Author

ykakarap commented Mar 2, 2023

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

@sbueringer
Copy link
Member

/retest

@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2023 •
edited
Loading

@ykakarap Just fyi. Looks like the upgrade test tells us that removing the taint doesn't work at the moment

Mar 2 17:08:20.280: INFO: ==== node wait: 3 out of 5 nodes are ready, max notReady allowed 0. Need 2 more before starting.
Mar 2 17:08:50.287: INFO: Unschedulable nodes= 2, maximum value for starting tests= 0
Mar 2 17:08:50.287: INFO: -> Node k8s-upgrade-and-conformance-jg0iwk-worker-q05w6x [[[ Ready=true, Network(available)=false, Taints=[{node.cluster.x-k8s.io/uninitialized NoSchedule }], NonblockingTaints=node-role.kubernetes.io/control-plane,node-role.kubernetes.io/master ]]]

@ykakarap
Copy link
Contributor Author

ykakarap commented Mar 2, 2023

@ykakarap Just fyi. Looks like the upgrade test tells us that removing the taint doesn't work at the moment

Mar 2 17:08:20.280: INFO: ==== node wait: 3 out of 5 nodes are ready, max notReady allowed 0. Need 2 more before starting.
Mar 2 17:08:50.287: INFO: Unschedulable nodes= 2, maximum value for starting tests= 0
Mar 2 17:08:50.287: INFO: -> Node k8s-upgrade-and-conformance-jg0iwk-worker-q05w6x [[[ Ready=true, Network(available)=false, Taints=[{node.cluster.x-k8s.io/uninitialized NoSchedule }], NonblockingTaints=node-role.kubernetes.io/control-plane,node-role.kubernetes.io/master ]]]

Looking into it now.

@ykakarap ykakarap force-pushed the node-label-sync-taint branch from eb18a8e to c96d608 Compare March 3, 2023 06:25
@sbueringer
Copy link
Member

changes lgtm so far

@ykakarap ykakarap force-pushed the node-label-sync-taint branch from c96d608 to 23b35b8 Compare March 3, 2023 22:28
@ykakarap
Copy link
Contributor Author

ykakarap commented Mar 3, 2023

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

@ykakarap
Copy link
Contributor Author

ykakarap commented Mar 5, 2023

/retest

@ykakarap ykakarap changed the title ✨ apply node.cluster.x-k8s.io/uninitialized during machine creation ⚠️ apply node.cluster.x-k8s.io/uninitialized during machine creation Mar 5, 2023
sbueringer
sbueringer reviewed Mar 6, 2023
View reviewed changes
exp/internal/controllers/machinepool_controller_noderef.go Outdated
log.V(2).Info("Failed patch node to set annotations", "err", err, "node name", node.Name)
return ctrl.Result{}, err
// Add annotations.
annotations.AddAnnotations(node, desired)
Copy link
Member

@sbueringer sbueringer Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Q: Would it make sense to bring up the topic of in-place propagation on the Machine Pool machines via an issue?
(I guess it depends on MachinePool Machines being implemented first)

But fine for me to add the unitialized taint to MachinePool nodes as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open an issue and link it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened and issue: #8258 and also pinged on the MachinePool Machine PR: #7938 (comment)

@sbueringer
Copy link
Member

/lgtm
/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 51a2c47088cc92a547f7877074445e07fff9e8b9

fabriziopandini
fabriziopandini reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

great work, only two nits from my side

exp/internal/controllers/machinepool_controller_noderef.go Outdated Show resolved Hide resolved
internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved
@ykakarap ykakarap force-pushed the node-label-sync-taint branch from 23b35b8 to 446253a Compare March 6, 2023 20:12
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2023
sbueringer
sbueringer reviewed Mar 7, 2023
View reviewed changes
util/taints/taints.go Outdated Show resolved Hide resolved
Apply uninitialized taint to nodes at creation
a142ca0 
and delete after labels are synced
@ykakarap ykakarap force-pushed the node-label-sync-taint branch from 446253a to a142ca0 Compare March 7, 2023 17:18
@sbueringer
Copy link
Member

Thank you!!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3d884520d485cc8355aa5cd8e30b62d25feb6099

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2023
@sbueringer
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 1c15a20 into kubernetes-sigs:main Mar 8, 2023
@ykakarap ykakarap mentioned this pull request Mar 9, 2023
Open
3 tasks
@guettli guettli mentioned this pull request Jul 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

@ykakarap ykakarap

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
4 participants
@ykakarap @k8s-ci-robot @sbueringer @fabriziopandini