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

migrate --register-with-taints to KubeletConfiguration #105437

Merged
merged 1 commit into from Nov 17, 2021

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Oct 4, 2021

What type of PR is this?

/kind cleanup
/area kubelet
/area kubelet-api

What this PR does / why we need it:

Moving Kubelet --register-with-taints flags to Kubelet configuration file.

Which issue(s) this PR fixes:

Refs #105298

Special notes for your reviewer:

move implemention of the pflag.Value interface to pkg/util/flag/flags.go.

Does this PR introduce a user-facing change?

The Kubelet's `--register-with-taints` option is now available via the Kubelet config file field registerWithTaints

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 4, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @cmssczy. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 4, 2021
@nayihz
Copy link
Contributor Author

nayihz commented Oct 4, 2021

/kind cleanup
/area kubelet
/area kubelet-api

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/kubelet-api labels Oct 4, 2021
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@ehashman ehashman added this to Triage in SIG Node PR Triage Oct 4, 2021
@ehashman
Copy link
Member

ehashman commented Oct 6, 2021

/ok-to-test
/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 6, 2021
@ehashman ehashman moved this from Triage to Needs Reviewer in SIG Node PR Triage Oct 6, 2021
@ehashman
Copy link
Member

ehashman commented Oct 6, 2021

/label api-review

@nayihz nayihz requested a review from ehashman October 18, 2021 09:46
@liggitt liggitt moved this from Changes requested to In progress in API Reviews Oct 28, 2021
@nayihz
Copy link
Contributor Author

nayihz commented Nov 6, 2021

@ehashman maybe the test case failed because we set the default value? Could you give me some hints about this faliled test case?

// pkg/kubelet/apis/config/v1beta1/defaults.go
if obj.RegisterNode == nil {
	obj.RegisterNode = utilpointer.BoolPtr(true)
}

But I think RegisterNode should be set to true by default. So is it possible that we should update the test case? Unfortunately I can't find this test case in k/k.

@liggitt
Copy link
Member

liggitt commented Nov 11, 2021

@ehashman maybe the test case failed because we set the default value? Could you give me some hints about this faliled test case?

  1. Update the input test data for TestSetDefaultsKubeletConfiguration/all_positive to set the field to &true
  2. Update the input test data for TestSetDefaultsKubeletConfiguration/all_negative to set the field to &false
  3. Update the expected result for the field to &true for:
    • TestSetDefaultsKubeletConfiguration/empty_config
    • TestSetDefaultsKubeletConfiguration/all_positive

pkg/kubelet/kubelet_node_status.go Outdated Show resolved Hide resolved
pkg/kubelet/apis/config/types.go Show resolved Hide resolved
@liggitt liggitt moved this from In progress to Changes requested in API Reviews Nov 11, 2021
@liggitt liggitt added this to the v1.23 milestone Nov 11, 2021
@liggitt
Copy link
Member

liggitt commented Nov 12, 2021

it looks like your branch is ~1100 commits behind master... the test was added since you started this PR. Rebase this on master and you'll be able to locate the test to update it

@k8s-ci-robot k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Nov 13, 2021
@nayihz nayihz force-pushed the update-kubelet-configuration branch from 06d7b7a to 4b401e8 Compare November 13, 2021 01:48
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Nov 13, 2021
@nayihz nayihz force-pushed the update-kubelet-configuration branch 2 times, most recently from eea9fb4 to 740213b Compare November 15, 2021 01:59
@@ -126,6 +127,16 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error
if kc.TopologyManagerPolicy != kubeletconfig.NoneTopologyManagerPolicy && !localFeatureGate.Enabled(features.TopologyManager) {
allErrors = append(allErrors, fmt.Errorf("invalid configuration: topologyManagerPolicy %v requires feature gate TopologyManager", kc.TopologyManagerPolicy))
}
if kc.RegisterNode && len(kc.RegisterWithTaints) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

drop if kc.RegisterNode && len(kc.RegisterWithTaints) > 0 {

we want to check any provided RegisterWithTaints values regardless of whether RegisterNode was specified, and range will no-op on a nil or zero-length list

@nayihz nayihz force-pushed the update-kubelet-configuration branch from 740213b to bad4faf Compare November 16, 2021 11:10
@liggitt
Copy link
Member

liggitt commented Nov 16, 2021

API bits lgtm

/approve

/hold for node lgtm

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2021
@liggitt liggitt moved this from Changes requested to API review completed, 1.23 in API Reviews Nov 16, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cmssczy, liggitt

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 Nov 16, 2021
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

/hold cancel
/lgtm

Thanks for your responsiveness on this PR, I think this looks great.

if nodeTaint.TimeAdded != nil {
allErrors = append(allErrors, fmt.Errorf("taint TimeAdded is not nil"))
}
}
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 missing a test... I know CheckTaintValidation appears to have some below.

We didn't have the coverage before, so I don't want to block this PR just on this one thing.

#105360 will make adding one much easier.

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 follow this pr if it doesn't add CheckTaintValidation test function. Thanks for your remind.

@@ -255,3 +257,44 @@ func (v *ReservedMemoryVar) String() string {
func (v *ReservedMemoryVar) Type() string {
return "reserved-memory"
}

// RegisterWithTaintsVar is used for validating a command line option that represents a register with taints. It implements the pflag.Value interface
Copy link
Member

Choose a reason for hiding this comment

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

Reference diff:

-// NewTaintsVar wraps []api.Taint in a struct that implements flag.Value to allow taints to be
-// bound to command line flags.
-func NewTaintsVar(ptr *[]api.Taint) taintsVar {
-       return taintsVar{
-               ptr: ptr,
-       }
-}
-
-type taintsVar struct {
-       ptr *[]api.Taint
+// RegisterWithTaintsVar is used for validating a command line option that represents a register with taints. It implements the pflag.Value interface
+type RegisterWithTaintsVar struct {
+       Value *[]v1.Taint
 }
 
-func (t taintsVar) Set(s string) error {
+// Set sets the flag value
+func (t RegisterWithTaintsVar) Set(s string) error {
        if len(s) == 0 {
-               *t.ptr = nil
+               *t.Value = nil
                return nil
        }
        sts := strings.Split(s, ",")
-       var taints []api.Taint
-       for _, st := range sts {
-               taint, err := parseTaint(st)
-               if err != nil {
-                       return err
-               }
-               taints = append(taints, api.Taint{Key: taint.Key, Value: taint.Value, Effect: api.TaintEffect(taint.Effect)})
+       corev1Taints, _, err := utiltaints.ParseTaints(sts)
+       if err != nil {
+               return err
+       }
+       var taints []v1.Taint
+       for _, ct := range corev1Taints {
+               taints = append(taints, v1.Taint{Key: ct.Key, Value: ct.Value, Effect: v1.TaintEffect(ct.Effect)})
        }
-       *t.ptr = taints
+       *t.Value = taints
        return nil
 }
 
-func (t taintsVar) String() string {
-       if len(*t.ptr) == 0 {
+// String returns the flag value
+func (t RegisterWithTaintsVar) String() string {
+       if len(*t.Value) == 0 {
                return ""
        }
        var taints []string
-       for _, taint := range *t.ptr {
+       for _, taint := range *t.Value {
                taints = append(taints, fmt.Sprintf("%s=%s:%s", taint.Key, taint.Value, taint.Effect))
        }
        return strings.Join(taints, ",")
 }
 
-func (t taintsVar) Type() string {
-       return "[]api.Taint"
+// Type gets the flag type
+func (t RegisterWithTaintsVar) Type() string {
+       return "[]v1.Taint"
 }

Copy link
Member

Choose a reason for hiding this comment

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

Effectively identical.

@@ -287,3 +288,62 @@ func TestReservedMemoryVar(t *testing.T) {
}
}
}

func TestTaintsVar(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Reference diff:

diff --git a/tmp/bar2 b/tmp/bar1
index 756ac87..4fed623 100644
--- a/tmp/bar2
+++ b/tmp/bar1
@@ -2,32 +2,32 @@ func TestTaintsVar(t *testing.T) {
        cases := []struct {
                f   string
                err bool
-               t   []api.Taint
+               t   []v1.Taint
        }{
                {
                        f: "",
-                       t: []api.Taint(nil),
+                       t: []v1.Taint(nil),
                },
                {
                        f: "--t=foo=bar:NoSchedule",
-                       t: []api.Taint{{Key: "foo", Value: "bar", Effect: "NoSchedule"}},
+                       t: []v1.Taint{{Key: "foo", Value: "bar", Effect: "NoSchedule"}},
                },
                {
                        f: "--t=baz:NoSchedule",
-                       t: []api.Taint{{Key: "baz", Value: "", Effect: "NoSchedule"}},
+                       t: []v1.Taint{{Key: "baz", Value: "", Effect: "NoSchedule"}},
                },
                {
                        f: "--t=foo=bar:NoSchedule,baz:NoSchedule,bing=bang:PreferNoSchedule,qux=:NoSchedule",
-                       t: []api.Taint{
-                               {Key: "foo", Value: "bar", Effect: api.TaintEffectNoSchedule},
+                       t: []v1.Taint{
+                               {Key: "foo", Value: "bar", Effect: v1.TaintEffectNoSchedule},
                                {Key: "baz", Value: "", Effect: "NoSchedule"},
-                               {Key: "bing", Value: "bang", Effect: api.TaintEffectPreferNoSchedule},
+                               {Key: "bing", Value: "bang", Effect: v1.TaintEffectPreferNoSchedule},
                                {Key: "qux", Value: "", Effect: "NoSchedule"},
                        },
                },
                {
                        f: "--t=dedicated-for=user1:NoExecute,baz:NoSchedule,foo-bar=:NoSchedule",
-                       t: []api.Taint{
+                       t: []v1.Taint{
                                {Key: "dedicated-for", Value: "user1", Effect: "NoExecute"},
                                {Key: "baz", Value: "", Effect: "NoSchedule"},
                                {Key: "foo-bar", Value: "", Effect: "NoSchedule"},
@@ -38,8 +38,8 @@ func TestTaintsVar(t *testing.T) {
        for i, c := range cases {
                args := append([]string{"test"}, strings.Fields(c.f)...)
                cli := pflag.NewFlagSet("test", pflag.ContinueOnError)
-               var taints []api.Taint
-               cli.Var(NewTaintsVar(&taints), "t", "bar")
+               var taints []v1.Taint
+               cli.Var(RegisterWithTaintsVar{Value: &taints}, "t", "bar")
 
                err := cli.Parse(args)
                if err == nil && c.err {

Copy link
Member

Choose a reason for hiding this comment

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

Just a type change + swap for RegisterWithTaintsVar, lgtm.

{
name: "valid taint",
taintsToCheck: v1.Taint{Key: "foo_5", Value: "bar_5"},
expectedErr: false,
Copy link
Member

Choose a reason for hiding this comment

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

Test cases look good.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1f6d5ca into kubernetes:master Nov 17, 2021
SIG Node PR Triage automation moved this from Waiting on Author to Done Nov 17, 2021
@nayihz nayihz deleted the update-kubelet-configuration branch November 17, 2021 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/kubelet-api cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.23
Development

Successfully merging this pull request may close these issues.

None yet

6 participants