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

Update Taints field comment in the machine spec #651

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

vikaschoudhary16
Copy link
Contributor

What this PR does / why we need it:
This PR fixes description of a field, Taints, in the machine spec. Current description mentions that the list of taints in the machine spec should be applied to the node in a "authoritative" manner and should overwrite any other existing taints on the node with this list of the taints. This is not correct because it conflicts with existing features such as taint-nodes-by-condition and upcoming features like shutdown taint

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 #

Release note:

Update expected handling of Taints field in machine spec 

/cc @roberthbailey @derekwaynecarr @enxebre

@k8s-ci-robot
Copy link
Contributor

@vikaschoudhary16: GitHub didn't allow me to request PR reviews from the following users: enxebre.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:
This PR fixes description of a field, Taints, in the machine spec. Current description mentions that the list of taints in the machine spec should be applied to the node in a "authoritative" manner and should overwrite any other existing taints on the node with this list of the taints. This is not correct because it conflicts with existing features such as taint-nodes-by-condition and upcoming features like shutdown taint

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 #

Release note:

Update expected handling of Taints field in machine spec 

/cc @roberthbailey @derekwaynecarr @enxebre

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2018
@derekwaynecarr
Copy link
Contributor

+1 on the change. The doc as described is in conflict with other usages of taints in Kubernetes and other planned work coming in sig-node.

@roberthbailey roberthbailey self-assigned this Dec 21, 2018
@roberthbailey
Copy link
Contributor

Thanks, this was also my understanding of how we should handle taints after talking to folks on api machinery and node.

The only question I have is whether it is possible for our controller to know when a user removes a taint from the machine spec so that it can be removed from the node, or if the additive nature of the machine controller means that we will only apply new taints and never be able to remove them. The Reconcile method in the machine controller gets the current spec for the machine, but doesn't see the delta between old and new. It's simple to apply additive taints, but if a taint is removed from the spec, how would the machine controller know that a taint on the node was previously applied by the machine controller as opposed to a taint manually applied by a user or a taint applied by a different automated system?

/approve
/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 21, 2018
@derekwaynecarr
Copy link
Contributor

Is it sufficient for taints on a machine object to only reflect initial state? In practice, the node once actualized should be the source of truth.

@detiber
Copy link
Member

detiber commented Dec 21, 2018

I'm with @derekwaynecarr here and I also think that we should rename the field InitialTaints.

@roberthbailey
Copy link
Contributor

Is it sufficient for taints on a machine object to only reflect initial state?

I think so...

One downside is that it means that if you want to manage taints on nodes you need to think about it in two places, the machine object for not yet created nodes and the node object for ones that exist. And we provide automation that will constantly churning nodes (cluster autoscaler).

Another is that we will need to keep track somewhere of whether the taints have been applied to the node yet or not (since the new name would imply that it should only be done once).

If we keep the field mutable, we also need to decide what the behavior should be if it's changed. If new taints are added, should they be applied retroactively? Or just to new nodes? If a user had previously removed one of the initial taints, should it be replaced?

We should also think about changing the intended behavior of labels as well, since they should behave the same way as nodes.

@detiber
Copy link
Member

detiber commented Dec 21, 2018

Is it sufficient for taints on a machine object to only reflect initial state?

I think so...

One downside is that it means that if you want to manage taints on nodes you need to think about it in two places, the machine object for not yet created nodes and the node object for ones that exist. And we provide automation that will constantly churning nodes (cluster autoscaler).

Another is that we will need to keep track somewhere of whether the taints have been applied to the node yet or not (since the new name would imply that it should only be done once).

Do we though? I would assume that we could consider that a bootstrapping step that is applied as part of the node bringup (exposed through kubeadm or other bootstrapping tool).

If we keep the field mutable, we also need to decide what the behavior should be if it's changed. If new taints are added, should they be applied retroactively? Or just to new nodes? If a user had previously removed one of the initial taints, should it be replaced?

I'd be okay with us updating the field to be immutable. I'm not sure how we would enforce that, though. Is there some mechanism exposed by kubebuilder or CRDs for doing so?

We should also think about changing the intended behavior of labels as well, since they should behave the same way as nodes.

+1 for treating labels the same way.

@vikaschoudhary16
Copy link
Contributor Author

+1 for keeping the field immutable. Doing this, management of taints wont be required at two places rather at just nodes.
+1 for treating labels also the same way.

@enxebre
Copy link
Member

enxebre commented Jan 2, 2019

Another is that we will need to keep track somewhere of whether the taints have been applied to the node yet or not (since the new name would imply that it should only be done once).

Do we though? I would assume that we could consider that a bootstrapping step that is applied as part of the node bringup (exposed through kubeadm or other bootstrapping tool).

Having the field immutable and considering the node the source of truth for farther updates sounds reasonable. The initialTaints setup could be considered a part of the node controller "linking" #658

Keeping consistency with labels sounds good.

@roberthbailey
Copy link
Contributor

This came up during the subproject meeting today.

/cc @dlipovetsky

@k8s-ci-robot
Copy link
Contributor

@roberthbailey: GitHub didn't allow me to request PR reviews from the following users: dlipovetsky.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This came up during the subproject meeting today.

/cc @dlipovetsky

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.

@roberthbailey
Copy link
Contributor

One thing we discussed was having the set of taints that are specified on the machine be actively reconciled (e.g. if you ask the machine controller to apply a taint and then manually remove the taint the machine controller will put it back) but not have the machine controller remove any taints. The same would go for labels and annotations. This would make the behavior both more understandable and also allow us to build complementary automation around taint/label management.

@dlipovetsky
Copy link
Contributor

One thing we discussed was having the set of taints that are specified on the machine be actively reconciled ... but not have the machine controller remove any taints.

This seems to work with what I think is a common use case of the Machine.Spec.Taints field, namely to generate the kubeadm configuration (to be precise, the InitConfiguration.NodeRegistration.Taints field) passed to kubeadm init/join. kubeadm taints a master node unless the user explicitly specifies an empty set of taints.

In the design your described, an empty set of taints means "add no taints, but remove no taints," and this matches the semantics of the use case I describe.

@enxebre
Copy link
Member

enxebre commented Jan 25, 2019

Related to #658. Also for reference kubernetes/kubernetes#73097

@roberthbailey
Copy link
Contributor

This PR needs a rebase.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 21, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 21, 2019
@detiber
Copy link
Member

detiber commented Mar 21, 2019

/lgtm

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

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roberthbailey, vikaschoudhary16, vincepri

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:
  • OWNERS [roberthbailey,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vincepri
Copy link
Member

Leaving it on hold until next week's community meeting so we can give folks a heads up :)

@vikaschoudhary16
Copy link
Contributor Author

thanks @vincepri !!!
sounds good.

@detiber
Copy link
Member

detiber commented Mar 28, 2019

Removing hold since it's been a week
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit 9f67deb into kubernetes-sigs:master Mar 28, 2019
serbrech pushed a commit to serbrech/cluster-api that referenced this pull request Apr 8, 2019
jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants