Merge pull request #4206 from QiWang19/4.13-anno

[release-4.13] OCPBUGS-29721: Add existing kubeletconfig/ctrcfg mc-name-suffix annotation

Author: openshift-merge-bot[bot]

pkg/controller/container-runtime-config/container_runtime_config_controller.go

@@ -627,23 +627,23 @@ func (ctrl *Controller) syncContainerRuntimeConfig(key string) error {
 			if err != nil {
 				return ctrl.syncStatusOnly(cfg, err, "could not create MachineConfig from new Ignition config: %v", err)
 			}
-			_, ok := cfg.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
-			arr := strings.Split(managedKey, "-")
-			// the first managed key value 99-poolname-generated-containerruntime does not have a suffix
-			// set "" as suffix annotation to the containerruntime config object
-			if _, err := strconv.Atoi(arr[len(arr)-1]); err != nil && !ok {
-				if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, ""); err != nil {
-					return ctrl.syncStatusOnly(cfg, err, "could not update annotation for containerruntimeConfig")
-				}
+		}
+		_, ok := cfg.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
+		arr := strings.Split(managedKey, "-")
+		// the first managed key value 99-poolname-generated-containerruntime does not have a suffix
+		// set "" as suffix annotation to the containerruntime config object
+		if _, err := strconv.Atoi(arr[len(arr)-1]); err != nil && !ok {
+			if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, ""); err != nil {
+				return ctrl.syncStatusOnly(cfg, err, "could not update annotation for containerruntimeConfig")
 			}
-			// If the MC name suffix annotation does not exist and the managed key value returned has a suffix, then add the MC name
-			// suffix annotation and suffix value to the ctrcfg object
-			if len(arr) > 4 && !ok {
-				_, err := strconv.Atoi(arr[len(arr)-1])
-				if err == nil {
-					if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, arr[len(arr)-1]); err != nil {
-						return ctrl.syncStatusOnly(cfg, err, "could not update annotation for containerRuntimeConfig")
-					}
+		}
+		// If the MC name suffix annotation does not exist and the managed key value returned has a suffix, then add the MC name
+		// suffix annotation and suffix value to the ctrcfg object
+		if len(arr) > 4 && !ok {
+			_, err := strconv.Atoi(arr[len(arr)-1])
+			if err == nil {
+				if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, arr[len(arr)-1]); err != nil {
+					return ctrl.syncStatusOnly(cfg, err, "could not update annotation for containerRuntimeConfig")
 				}
 			}
 		}

pkg/controller/container-runtime-config/container_runtime_config_controller_test.go

@@ -137,7 +137,7 @@ func newControllerConfig(name string, platform apicfgv1.PlatformType) *mcfgv1.Co
 func newContainerRuntimeConfig(name string, ctrconf *mcfgv1.ContainerRuntimeConfiguration, selector *metav1.LabelSelector) *mcfgv1.ContainerRuntimeConfig {
 	return &mcfgv1.ContainerRuntimeConfig{
 		TypeMeta:   metav1.TypeMeta{APIVersion: mcfgv1.SchemeGroupVersion.String()},
-		ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(utilrand.String(5)), Generation: 1},
+		ObjectMeta: metav1.ObjectMeta{Name: name, UID: types.UID(utilrand.String(5)), Generation: 1, CreationTimestamp: metav1.Now()},
 		Spec: mcfgv1.ContainerRuntimeConfigSpec{
 			ContainerRuntimeConfig:    ctrconf,
 			MachineConfigPoolSelector: selector,
@@ -1512,6 +1512,63 @@ func TestContainerruntimeConfigResync(t *testing.T) {
 	}
 }
 
+func TestAddAnnotationExistingContainerRuntimeConfig(t *testing.T) {
+	for _, platform := range []apicfgv1.PlatformType{apicfgv1.AWSPlatformType, apicfgv1.NonePlatformType, "unrecognized"} {
+		t.Run(string(platform), func(t *testing.T) {
+			f := newFixture(t)
+			f.newController()
+
+			cc := newControllerConfig(ctrlcommon.ControllerConfigName, platform)
+			mcp := helpers.NewMachineConfigPool("master", nil, helpers.MasterSelector, "v0")
+			mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0")
+
+			ctrMCKey := "99-master-generated-containerruntime"
+			ctr1MCKey := "99-master-generated-containerruntime-1"
+			ctrc := newContainerRuntimeConfig("log-level", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug"}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", ""))
+			ctrc.Finalizers = []string{ctrMCKey}
+			ctrc1 := newContainerRuntimeConfig("log-level-1", &mcfgv1.ContainerRuntimeConfiguration{LogLevel: "debug"}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", ""))
+			ctrc1.SetAnnotations(map[string]string{ctrlcommon.MCNameSuffixAnnotationKey: "1"})
+			ctrc1.Finalizers = []string{ctr1MCKey}
+			ctrcfgMC := helpers.NewMachineConfig(ctrMCKey, map[string]string{"node-role/master": ""}, "dummy://", []ign3types.File{{}})
+
+			f.ccLister = append(f.ccLister, cc)
+			f.mcpLister = append(f.mcpLister, mcp)
+			f.mcpLister = append(f.mcpLister, mcp2)
+			f.mccrLister = append(f.mccrLister, ctrc, ctrc1)
+			f.objects = append(f.objects, ctrc, ctrc1, ctrcfgMC)
+
+			// ctrc created before ctrc1,
+			// make sure ccr does not have annotation machineconfiguration.openshift.io/mc-name-suffix before sync, ccr1 has annotation machineconfiguration.openshift.io/mc-name-suffix
+			_, ok := ctrc.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
+			require.False(t, ok)
+			val := ctrc1.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
+			require.Equal(t, "1", val)
+
+			// no new machine config will be created
+			f.expectGetMachineConfigAction(ctrcfgMC)
+			f.expectGetMachineConfigAction(ctrcfgMC)
+			f.expectUpdateContainerRuntimeConfigRoot(ctrc)
+			f.expectUpdateMachineConfigAction(ctrcfgMC)
+			f.expectPatchContainerRuntimeConfig(ctrc, ctrcfgPatchBytes)
+			f.expectUpdateContainerRuntimeConfig(ctrc)
+
+			c := f.newController()
+			err := c.syncHandler(getKey(ctrc, t))
+			if err != nil {
+				t.Errorf("syncHandler returned: %v", err)
+			}
+			f.validateActions()
+
+			// ctrc annotation machineconfiguration.openshift.io/mc-name-suffix after sync
+			val = ctrc.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
+			require.Equal(t, "", val)
+			ctrcFinalizers := ctrc.GetFinalizers()
+			require.Equal(t, 1, len(ctrcFinalizers))
+			require.Equal(t, ctrMCKey, ctrcFinalizers[0])
+		})
+	}
+}
+
 // TestCleanUpDuplicatedMC test the function removes the MC from the MC list
 // if the MC is of old GeneratedByControllerVersionAnnotationKey.
 func TestCleanUpDuplicatedMC(t *testing.T) {

pkg/controller/container-runtime-config/helpers.go

@@ -33,12 +33,13 @@ import (
 )
 
 const (
-	minLogSize              = 8192
-	minPidsLimit            = 20
-	storageConfigPath       = "/etc/containers/storage.conf"
-	registriesConfigPath    = "/etc/containers/registries.conf"
-	searchRegDropInFilePath = "/etc/containers/registries.conf.d/01-image-searchRegistries.conf"
-	policyConfigPath        = "/etc/containers/policy.json"
+	minLogSize                             = 8192
+	minPidsLimit                           = 20
+	managedContainerRuntimeConfigKeyPrefix = "99"
+	storageConfigPath                      = "/etc/containers/storage.conf"
+	registriesConfigPath                   = "/etc/containers/registries.conf"
+	searchRegDropInFilePath                = "/etc/containers/registries.conf.d/01-image-searchRegistries.conf"
+	policyConfigPath                       = "/etc/containers/policy.json"
 	// CRIODropInFilePathLogLevel is the path at which changes to the crio config for log-level
 	// will be dropped in this is exported so that we can use it in the e2e-tests
 	CRIODropInFilePathLogLevel       = "/etc/crio/crio.conf.d/01-ctrcfg-logLevel"
@@ -221,11 +222,60 @@ func getManagedKeyCtrCfg(pool *mcfgv1.MachineConfigPool, client mcfgclientset.In
 		return ctrlcommon.GetManagedKey(pool, client, "99", "containerruntime", getManagedKeyCtrCfgDeprecated(pool))
 	}
 
-	// If we are here, this means that a new containerruntime config was created, so we have to calculate the suffix value for its MC name
+	// If we are here, this means that
+	// 1. a new containerruntime config was created, so we have to calculate the suffix value for its MC name
+	// 2. or this is an existing containerruntime config did not get ctrlcommon.MCNameSuffixAnnotationKey set, so we have to set the MCNameSuffixAnnotationKey to the machineconfig suffix it was rendered to, assume for existing containerruntime config, cfg.Finalizers with the largest suffix is the machine config the ctrcfg was rendered to
 	// if the containerruntime config is the only one in the list, mc name should not suffixed since cfg is the first containerruntime config to be created
 	if len(ctrcfgList) == 1 {
 		return ctrlcommon.GetManagedKey(pool, client, "99", "containerruntime", getManagedKeyCtrCfgDeprecated(pool))
 	}
+	// if cfg is not a newly created containerruntime config and did not get ctrlcommon.MCNameSuffixAnnotationKey
+	// but has been rendered to a machineconfig, its len(cfg.Finalizers) > 0
+	if notLatestContainerRuntimeConfigInPool(ctrcfgList, cfg) {
+		finalizers := cfg.GetFinalizers()
+		containerRuntimeMCPrefix := fmt.Sprintf("99-%s-generated-containerruntime", pool.Name)
+		latestFinalizerIdx := -1
+		maxFinalizerSuffix := -1
+		for i, f := range finalizers {
+			// skip the finalizer:
+			// the finalizer is not a machineconfig
+			// the finalizer is not generated by containerruntimeconfig controller of this pool
+			if _, err := client.MachineconfigurationV1().MachineConfigs().Get(context.TODO(), f, metav1.GetOptions{}); err != nil {
+				glog.Infof("skipping error: %v", fmt.Errorf("containerruntimeconfig %s has invalid finalizer: %s", cfg.Name, f))
+				continue
+			}
+			if !strings.HasPrefix(f, containerRuntimeMCPrefix) {
+				continue
+			}
+			arr := strings.Split(f, "-")
+			suffix, err := strconv.Atoi(arr[len(arr)-1])
+			// if the finalizer does not end with a number, make sure it is in the format 99-<poolname>-generated-containerruntime
+			// otherwise, the ctrcfg contains invalid finalizer, do not generate managedKey from finalizers
+			if err != nil {
+				key, err := ctrlcommon.GetManagedKey(pool, nil, managedContainerRuntimeConfigKeyPrefix, "containerruntime", getManagedKeyCtrCfgDeprecated(pool))
+				if err != nil {
+					glog.Infof("skipping error: %v", fmt.Errorf("error generating managedKey for suffix %s: %v", key, err))
+					continue
+				}
+				if f != key {
+					glog.Infof("skipping error: %v", fmt.Errorf("finalizer format does not match the managedKubeletConfigKey: %s, want: %s", f, key))
+					continue
+				}
+				// if the suffix is not a number, f is 99-<poolname>-generated-containerruntime
+				suffix = 0
+			}
+			if suffix > maxFinalizerSuffix {
+				maxFinalizerSuffix = suffix
+				latestFinalizerIdx = i
+			}
+		}
+
+		if latestFinalizerIdx != -1 {
+			glog.Infof("return containerruntimeconfig managedKey from finalizers %s", finalizers[latestFinalizerIdx])
+			return finalizers[latestFinalizerIdx], nil
+		}
+		glog.Infof("skipping error generating managedKey for existing containerruntimeconfig %s from finalizers", cfg.Name)
+	}
 	suffixNum := 0
 	// Go through the list of ctrcfg objects created and get the max suffix value currently created
 	for _, item := range ctrcfgList {
@@ -253,6 +303,15 @@ func getManagedKeyCtrCfg(pool *mcfgv1.MachineConfigPool, client mcfgclientset.In
 	return fmt.Sprintf("99-%s-generated-containerruntime-%s", pool.Name, strconv.Itoa(suffixNum+1)), nil
 }
 
+func notLatestContainerRuntimeConfigInPool(ctrcfgList []mcfgv1.ContainerRuntimeConfig, cfg *mcfgv1.ContainerRuntimeConfig) bool {
+	for _, crc := range ctrcfgList {
+		if cfg.CreationTimestamp.Time.Before(crc.CreationTimestamp.Time) {
+			return true
+		}
+	}
+	return false
+}
+
 // Deprecated: use getManagedKeyReg
 func getManagedKeyRegDeprecated(pool *mcfgv1.MachineConfigPool) string {
 	return fmt.Sprintf("99-%s-%s-registries", pool.Name, pool.ObjectMeta.UID)

pkg/controller/kubelet-config/helpers.go

@@ -20,6 +20,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/klog/v2"
 	kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1"
 
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
@@ -258,11 +259,61 @@ func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclien
 		return ctrlcommon.GetManagedKey(pool, client, managedKubeletConfigKeyPrefix, "kubelet", getManagedKubeletConfigKeyDeprecated(pool))
 	}
 
-	// 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
+	// If we are here, this means that
+	// 1. a new kubelet config was created, so we have to calculate the suffix value for its MC name
+	// 2. or this is an existing kubeletconfig did not get ctrlcommon.MCNameSuffixAnnotationKey set, so we have to set the MCNameSuffixAnnotationKey to the machineconfig suffix it was rendered to, assume for existing kubeletconfig, cfg.Finalizers with the largest suffix is the machine config the kcfg was rendered to
 	// if the kubelet config is the only one in the list, mc name should not suffixed since cfg is the first kubelet config to be created
 	if len(kcList) == 1 {
 		return ctrlcommon.GetManagedKey(pool, client, managedKubeletConfigKeyPrefix, "kubelet", getManagedKubeletConfigKeyDeprecated(pool))
 	}
+	// if cfg is not a newly created kubeletconfig and did not get ctrlcommon.MCNameSuffixAnnotationKey
+	// but has been rendered to a machineconfig, its len(cfg.Finalizers) > 0
+	if notLatestKubeletConfigInPool(kcList, cfg) {
+		finalizers := cfg.GetFinalizers()
+		kubeletMCPrefix := fmt.Sprintf("99-%s-generated-kubelet", pool.Name)
+		latestFinalizerIdx := -1
+		maxFinalizerSuffix := -1
+		for i, f := range finalizers {
+			// skip the finalizer:
+			// the finalizer is not a machineconfig
+			// the finalizer is not generated by kubeletconfig controller of this pool
+			if _, err := client.MachineconfigurationV1().MachineConfigs().Get(context.TODO(), f, metav1.GetOptions{}); err != nil {
+				klog.Infof("skipping error: %v", fmt.Errorf("kubeletconfig %s has invalid finalizer: %s", cfg.Name, f))
+				continue
+			}
+			if !strings.HasPrefix(f, kubeletMCPrefix) {
+				continue
+			}
+			arr := strings.Split(f, "-")
+			suffix, err := strconv.Atoi(arr[len(arr)-1])
+			// if the finalizer does not end with a number, make sure it is in the format 99-<poolname>-generated-kubelet
+			// otherwise, the kcfg contains invalid finalizer, do not generate managedKey from finalizers
+			if err != nil {
+				key, err := ctrlcommon.GetManagedKey(pool, nil, managedKubeletConfigKeyPrefix, "kubelet", getManagedKubeletConfigKeyDeprecated(pool))
+				if err != nil {
+					klog.Infof("skipping error: %v", fmt.Errorf("error generating managedKey for suffix %s: %v", key, err))
+					continue
+				}
+				if f != key {
+					klog.Infof("skipping error: %v", fmt.Errorf("finalizer format does not match the managedKubeletConfigKey: %s, want: %s", f, key))
+					continue
+				}
+				// if the suffix is not a number, f is 99-<poolname>-generated-kubelet
+				suffix = 0
+			}
+			if suffix > maxFinalizerSuffix {
+				maxFinalizerSuffix = suffix
+				latestFinalizerIdx = i
+			}
+		}
+
+		if latestFinalizerIdx != -1 {
+			klog.Infof("return kubeletconfig managedKey from finalizers %s", finalizers[latestFinalizerIdx])
+			return finalizers[latestFinalizerIdx], nil
+		}
+		klog.Infof("skipping error generating managedKey for existing kubeletconfig %s from finalizers", cfg.Name)
+	}
+
 	suffixNum := 0
 	// Go through the list of kubelet config objects created and get the max suffix value currently created
 	for _, item := range kcList {
@@ -278,6 +329,7 @@ func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclien
 			}
 		}
 	}
+
 	// The max suffix value that we can go till with this logic is 9 - this means that a user can create up to 10 different kubelet config CRs.
 	// However, if there is a kc-1 mapping to mc-1 and kc-2 mapping to mc-2 and the user deletes kc-1, it will delete mc-1 but
 	// then if the user creates a kc-new it will map to mc-3. This is what we want as the latest kubelet config created should be higher in priority
@@ -290,6 +342,15 @@ func getManagedKubeletConfigKey(pool *mcfgv1.MachineConfigPool, client mcfgclien
 	return fmt.Sprintf("%s-%s-generated-kubelet-%s", managedKubeletConfigKeyPrefix, pool.Name, strconv.Itoa(suffixNum+1)), nil
 }
 
+func notLatestKubeletConfigInPool(kcList []mcfgv1.KubeletConfig, cfg *mcfgv1.KubeletConfig) bool {
+	for _, kc := range kcList {
+		if cfg.CreationTimestamp.Time.Before(kc.CreationTimestamp.Time) {
+			return true
+		}
+	}
+	return false
+}
+
 func getManagedFeaturesKey(pool *mcfgv1.MachineConfigPool, client mcfgclientset.Interface) (string, error) {
 	return ctrlcommon.GetManagedKey(pool, client, managedFeaturesKeyPrefix, "kubelet", getManagedFeaturesKeyDeprecated(pool))
 }

pkg/controller/kubelet-config/kubelet_config_controller.go

@@ -603,23 +603,23 @@ func (ctrl *Controller) syncKubeletConfig(key string) error {
 				return ctrl.syncStatusOnly(cfg, err, "could not create MachineConfig from new Ignition config: %v", err)
 			}
 			mc.ObjectMeta.UID = uuid.NewUUID()
-			_, ok := cfg.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
-			arr := strings.Split(managedKey, "-")
-			// the first managed key value 99-poolname-generated-kubelet does not have a suffix
-			// set "" as suffix annotation to the kubelet config object
-			if _, err := strconv.Atoi(arr[len(arr)-1]); err != nil && !ok {
-				if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, ""); err != nil {
-					return ctrl.syncStatusOnly(cfg, err, "could not update annotation for kubeletConfig")
-				}
+		}
+		_, ok := cfg.GetAnnotations()[ctrlcommon.MCNameSuffixAnnotationKey]
+		arr := strings.Split(managedKey, "-")
+		// the first managed key value 99-poolname-generated-kubelet does not have a suffix
+		// set "" as suffix annotation to the kubelet config object
+		if _, err := strconv.Atoi(arr[len(arr)-1]); err != nil && !ok {
+			if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, ""); err != nil {
+				return ctrl.syncStatusOnly(cfg, err, "could not update annotation for kubeletConfig")
 			}
-			// If the MC name suffix annotation does not exist and the managed key value returned has a suffix, then add the MC name
-			// suffix annotation and suffix value to the kubelet config object
-			if len(arr) > 4 && !ok {
-				_, err := strconv.Atoi(arr[len(arr)-1])
-				if err == nil {
-					if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, arr[len(arr)-1]); err != nil {
-						return ctrl.syncStatusOnly(cfg, err, "could not update annotation for kubeletConfig")
-					}
+		}
+		// If the MC name suffix annotation does not exist and the managed key value returned has a suffix, then add the MC name
+		// suffix annotation and suffix value to the kubelet config object
+		if len(arr) > 4 && !ok {
+			_, err := strconv.Atoi(arr[len(arr)-1])
+			if err == nil {
+				if err := ctrl.addAnnotation(cfg, ctrlcommon.MCNameSuffixAnnotationKey, arr[len(arr)-1]); err != nil {
+					return ctrl.syncStatusOnly(cfg, err, "could not update annotation for kubeletConfig")
 				}
 			}
 		}