From 6e1c4f9a5e97bf947b38c3b57023162b054cd4a5 Mon Sep 17 00:00:00 2001 From: Ryan Phillips Date: Thu, 2 Sep 2021 13:28:07 -0500 Subject: [PATCH 1/3] fix 1 to 1 relation to kubelet configs and machine config pools --- pkg/controller/kubelet-config/helpers.go | 25 ++++++++-- .../kubelet_config_controller_test.go | 48 +++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/pkg/controller/kubelet-config/helpers.go b/pkg/controller/kubelet-config/helpers.go index 7a1a10af53..f21a89ab1b 100644 --- a/pkg/controller/kubelet-config/helpers.go +++ b/pkg/controller/kubelet-config/helpers.go @@ -12,6 +12,7 @@ import ( "github.com/vincent-petithory/dataurl" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -145,22 +146,36 @@ func findKubeletConfig(mc *mcfgv1.MachineConfig) (*ign3types.File, error) { // nolint: dupl func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclientset.Interface, cfg *mcfgv1.KubeletConfig) (string, error) { // Get all the kubelet config CRs - kcList, err := client.MachineconfigurationV1().KubeletConfigs().List(context.TODO(), metav1.ListOptions{}) + kcListAll, err := client.MachineconfigurationV1().KubeletConfigs().List(context.TODO(), metav1.ListOptions{}) if err != nil { return "", fmt.Errorf("error listing kubelet configs: %v", err) } + // If there is no kubelet config in the list, return the default MC name with no suffix - if kcList == nil || len(kcList.Items) == 0 { + if kcListAll == nil || len(kcListAll.Items) == 0 { return ctrlcommon.GetManagedKey(pool, client, "99", "kubelet", getManagedKubeletConfigKeyDeprecated(pool)) } - for _, kc := range kcList.Items { + + var kcList []mcfgv1.KubeletConfig + for _, kc := range kcListAll.Items { + selector, err := metav1.LabelSelectorAsSelector(kc.Spec.MachineConfigPoolSelector) + if err != nil { + return "", fmt.Errorf("invalid label selector: %v", err) + } + if selector.Empty() || !selector.Matches(labels.Set(pool.Labels)) { + continue + } + kcList = append(kcList, kc) + } + + for _, kc := range kcList { if kc.Name != cfg.Name { continue } val, ok := kc.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey] // If we find a matching kubelet config and it is the only one in the list, then return the default MC name with no suffix // add check len(kcList.Items) < 2, mc name should not suffixed if cfg is the first kubelet config to be updated/created - if !ok && len(kcList.Items) < 2 { + if !ok && len(kcList) < 2 { return ctrlcommon.GetManagedKey(pool, client, "99", "kubelet", getManagedKubeletConfigKeyDeprecated(pool)) } // Otherwise if an MC name suffix exists, append it to the default MC name and return that as this kubelet config exists and @@ -173,7 +188,7 @@ func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclien // If we are here, this means that a new kubelet config was created, so we have to calculate the suffix value for its MC name suffixNum := 0 // Go through the list of kubelet config objects created and get the max suffix value currently created - for _, item := range kcList.Items { + for _, item := range kcList { val, ok := item.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey] if ok { // Convert the suffix value to int so we can look through the list and grab the max suffix created so far diff --git a/pkg/controller/kubelet-config/kubelet_config_controller_test.go b/pkg/controller/kubelet-config/kubelet_config_controller_test.go index c9c7483b7d..3ffe36ffae 100644 --- a/pkg/controller/kubelet-config/kubelet_config_controller_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_controller_test.go @@ -383,6 +383,54 @@ func TestKubeletConfigCreate(t *testing.T) { } } +func TestKubeletConfigMultiCreate(t *testing.T) { + for _, platform := range []osev1.PlatformType{osev1.AWSPlatformType, osev1.NonePlatformType, "unrecognized"} { + t.Run(string(platform), func(t *testing.T) { + f := newFixture(t) + f.newController() + + cc := newControllerConfig(ctrlcommon.ControllerConfigName, platform) + f.ccLister = append(f.ccLister, cc) + + kcCount := 30 + for i := 0; i < kcCount; i++ { + f.resetActions() + + poolName := fmt.Sprintf("subpool%v", i) + poolLabelName := fmt.Sprintf("pools.operator.machineconfiguration.openshift.io/%s", poolName) + labelSelector := metav1.AddLabelToSelector(&metav1.LabelSelector{}, poolLabelName, "") + + mcp := helpers.NewMachineConfigPool(poolName, nil, labelSelector, "v0") + mcp.ObjectMeta.Labels[poolLabelName] = "" + + kc := newKubeletConfig(poolName, &kubeletconfigv1beta1.KubeletConfiguration{MaxPods: 100}, labelSelector) + + f.mcpLister = append(f.mcpLister, mcp) + f.mckLister = append(f.mckLister, kc) + f.objects = append(f.objects, kc) + + mcs := helpers.NewMachineConfig(generateManagedKey(kc, 1), labelSelector.MatchLabels, "dummy://", []ign3types.File{{}}) + mcsDeprecated := mcs.DeepCopy() + mcsDeprecated.Name = getManagedKubeletConfigKeyDeprecated(mcp) + + expectedPatch := fmt.Sprintf("{\"metadata\":{\"finalizers\":[\"99-%v-generated-kubelet-1\"]}}", poolName) + + f.expectGetMachineConfigAction(mcs) + f.expectGetMachineConfigAction(mcsDeprecated) + f.expectGetMachineConfigAction(mcs) + f.expectCreateMachineConfigAction(mcs) + f.expectPatchKubeletConfig(kc, []byte(expectedPatch)) + f.expectUpdateKubeletConfig(kc) + f.run(poolName) + } + }) + } +} + +func generateManagedKey(kcfg *mcfgv1.KubeletConfig, generation uint64) string { + return fmt.Sprintf("99-%s-generated-kubelet-%v", kcfg.Name, generation) +} + func TestKubeletConfigAutoSizingReserved(t *testing.T) { for _, platform := range []osev1.PlatformType{osev1.AWSPlatformType, osev1.NonePlatformType, "unrecognized"} { t.Run(string(platform), func(t *testing.T) { From c1fc3dbfcfcfd047c59962284fc1f0b9b0471dcc Mon Sep 17 00:00:00 2001 From: Ryan Phillips Date: Thu, 2 Sep 2021 13:28:43 -0500 Subject: [PATCH 2/3] add more debug functionality to the test suite --- .../kubelet_config_controller_test.go | 39 +++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/pkg/controller/kubelet-config/kubelet_config_controller_test.go b/pkg/controller/kubelet-config/kubelet_config_controller_test.go index 3ffe36ffae..40b8d01101 100644 --- a/pkg/controller/kubelet-config/kubelet_config_controller_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_controller_test.go @@ -56,7 +56,8 @@ type fixture struct { featLister []*osev1.FeatureGate apiserverLister []*osev1.APIServer - actions []core.Action + actions []core.Action + skipActionsValidation bool objects []runtime.Object oseobjects []runtime.Object @@ -78,7 +79,23 @@ func newFixture(t *testing.T) *fixture { } func (f *fixture) validateActions() { + if f.skipActionsValidation { + f.t.Log("Skipping actions validation") + return + } actions := filterInformerActions(f.client.Actions()) + if len(f.actions) != len(actions) { + f.t.Log("Expected Actions:") + for i := range f.actions { + f.t.Logf("\t%v %#v", f.actions[i].GetVerb(), f.actions[i]) + } + f.t.Log("Seen Actions:") + for i := range actions { + f.t.Logf("\t%v %#v", actions[i].GetVerb(), actions[i]) + } + f.t.Errorf("number of seen actions do not match expected actions count") + return + } for i, action := range actions { glog.Infof(" Action: %v", action) @@ -88,7 +105,7 @@ func (f *fixture) validateActions() { } expectedAction := f.actions[i] - checkAction(expectedAction, action, f.t) + checkAction(expectedAction, action, f.t, i) } if len(f.actions) > len(actions) { @@ -287,9 +304,14 @@ func filterOSEActions(actions []core.Action) []core.Action { // checkAction verifies that expected and actual actions are equal and both have // same attached resources -func checkAction(expected, actual core.Action, t *testing.T) { +func checkAction(expected, actual core.Action, t *testing.T, index int) { if !(expected.Matches(actual.GetVerb(), actual.GetResource().Resource) && actual.GetSubresource() == expected.GetSubresource()) { - t.Errorf("Expected\n\t%#v\ngot\n\t%#v", expected, actual) + if actual.GetVerb() == "patch" { + actual := actual.(core.PatchAction) + t.Errorf("Expected(index=%v)\n\t%#v\ngot\n\t%#v\npatch\t%#v", index, expected, actual, string(actual.GetPatch())) + } else { + t.Errorf("Expected(index=%v)\n\t%#v\ngot\n\t%#v", index, expected, actual) + } return } @@ -350,6 +372,15 @@ func (f *fixture) expectUpdateKubeletConfig(config *mcfgv1.KubeletConfig) { f.actions = append(f.actions, core.NewRootUpdateSubresourceAction(schema.GroupVersionResource{Version: "v1", Group: "machineconfiguration.openshift.io", Resource: "kubeletconfigs"}, "status", config)) } +func (f *fixture) expectUpdateKubeletConfigRoot(config *mcfgv1.KubeletConfig) { + f.actions = append(f.actions, core.NewRootUpdateAction(schema.GroupVersionResource{Version: "v1", Group: "machineconfiguration.openshift.io", Resource: "kubeletconfigs"}, config)) +} + +func (f *fixture) resetActions() { + f.actions = []core.Action{} + f.client.ClearActions() +} + func TestKubeletConfigCreate(t *testing.T) { for _, platform := range []osev1.PlatformType{osev1.AWSPlatformType, osev1.NonePlatformType, "unrecognized"} { t.Run(string(platform), func(t *testing.T) { From b0139eaf9c1f2c4c225e6019e0b14e70ea6f199f Mon Sep 17 00:00:00 2001 From: Ryan Phillips Date: Thu, 2 Sep 2021 19:19:27 -0500 Subject: [PATCH 3/3] fix comment in kubelet-config/helpers.go Co-authored-by: Kirsten --- pkg/controller/kubelet-config/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/kubelet-config/helpers.go b/pkg/controller/kubelet-config/helpers.go index f21a89ab1b..1e3aab5bb0 100644 --- a/pkg/controller/kubelet-config/helpers.go +++ b/pkg/controller/kubelet-config/helpers.go @@ -174,7 +174,7 @@ func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclien } val, ok := kc.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey] // If we find a matching kubelet config and it is the only one in the list, then return the default MC name with no suffix - // add check len(kcList.Items) < 2, mc name should not suffixed if cfg is the first kubelet config to be updated/created + // add check len(kcList) < 2, mc name should not suffixed if cfg is the first kubelet config to be updated/created if !ok && len(kcList) < 2 { return ctrlcommon.GetManagedKey(pool, client, "99", "kubelet", getManagedKubeletConfigKeyDeprecated(pool)) }