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
migrate --register-with-taints to KubeletConfiguration #105437
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/kind cleanup |
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. |
/ok-to-test |
/label api-review |
@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 |
|
it looks like your branch is ~1100 commits behind |
06d7b7a
to
4b401e8
Compare
eea9fb4
to
740213b
Compare
@@ -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 { |
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.
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
740213b
to
bad4faf
Compare
API bits lgtm /approve /hold for node lgtm |
[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 |
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.
/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")) | ||
} | ||
} |
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 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.
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.
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 |
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.
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"
}
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.
Effectively identical.
@@ -287,3 +288,62 @@ func TestReservedMemoryVar(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
func TestTaintsVar(t *testing.T) { |
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.
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 {
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.
Just a type change + swap for RegisterWithTaintsVar
, lgtm.
{ | ||
name: "valid taint", | ||
taintsToCheck: v1.Taint{Key: "foo_5", Value: "bar_5"}, | ||
expectedErr: false, |
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.
Test cases look good.
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: