improve node patch code

remove unnecessary functions (write configmap to disk, convert configmap to string, ...)

Signed-off-by: Karel Simon <[email protected]>

pkg/virt-handler/node-labeller/config.go

@@ -21,6 +21,7 @@ package nodelabeller
 import (
 	"strings"
 
+	"gopkg.in/yaml.v2"
 	v1 "k8s.io/api/core/v1"
 )
 
@@ -60,15 +61,11 @@ func (n *NodeLabeller) loadConfig() (Config, error) {
 	}
 
 	if value, ok := cm.Data["cpu-plugin-configmap.yaml"]; ok {
-		err := writeConfigFile(configPath, value)
+		err := yaml.Unmarshal([]byte(value), &config)
 		if err != nil {
 			return config, err
 		}
 	}
-	err = getStructureFromYamlFile(configPath, &config)
-	if err != nil {
-		return Config{}, err
-	}
 
 	return config, nil
 }

pkg/virt-handler/node-labeller/cpu_plugin.go

@@ -3,8 +3,6 @@ package nodelabeller
 import (
 	"encoding/xml"
 	"io/ioutil"
-
-	yaml "gopkg.in/yaml.v2"
 )
 
 const (
@@ -141,27 +139,3 @@ func getStructureFromXMLFile(path string, structure interface{}) error {
 	}
 	return nil
 }
-
-//GetStructureFromYamlFile load data from yaml file and unmarshals them into given structure
-//Given structure has to be pointer
-func getStructureFromYamlFile(path string, structure interface{}) error {
-	rawFile, err := ioutil.ReadFile(path)
-	if err != nil {
-		return err
-	}
-
-	//unmarshal data into structure
-	err = yaml.Unmarshal(rawFile, structure)
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func writeConfigFile(path, data string) error {
-	err := ioutil.WriteFile(path, []byte(data), 0777)
-	if err != nil {
-		return err
-	}
-	return nil
-}

pkg/virt-handler/node-labeller/node_labeller.go

@@ -1,11 +1,11 @@
 package nodelabeller
 
 import (
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"strconv"
 	"strings"
-	"sync"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
@@ -28,7 +28,6 @@ var nodeLabellerVolumePath = "/var/lib/kubevirt-node-labeller"
 
 //NodeLabeller struct holds informations needed to run node-labeller
 type NodeLabeller struct {
-	lock              sync.Mutex
 	kvmController     *device_manager.DeviceController
 	configMapInformer cache.SharedIndexInformer
 	nodeInformer      cache.SharedIndexInformer
@@ -115,7 +114,6 @@ func (n *NodeLabeller) Execute() bool {
 }
 
 func (n *NodeLabeller) run() error {
-	defer n.lock.Unlock()
 	cpuFeatures := make(map[string]bool)
 	cpuModels := make([]string, 0)
 
@@ -133,16 +131,15 @@ func (n *NodeLabeller) run() error {
 		return err
 	}
 	var (
-		ok   bool
-		node *v1.Node
+		ok           bool
+		originalNode *v1.Node
 	)
-	if node, ok = nodeObj.(*v1.Node); !ok {
+	if originalNode, ok = nodeObj.(*v1.Node); !ok {
 		n.logger.Infof("node-labeller cannot convert node " + n.host)
 		return fmt.Errorf("Could not convert node " + n.host)
 	}
 
-	n.lock.Lock()
-	originalNode := node.DeepCopy()
+	node := originalNode.DeepCopy()
 
 	//prepare new labels
 	newLabels := n.prepareLabels(cpuModels, cpuFeatures)
@@ -151,20 +148,8 @@ func (n *NodeLabeller) run() error {
 	//add new labels
 	n.addNodeLabels(node, newLabels)
 	//patch node only if there is change in labels
-	if !reflect.DeepEqual(node.Labels, originalNode.Labels) {
-		patchTestLabels := fmt.Sprintf(`{ "op": "test", "path": "/metadata/labels", "value": {%s}}`, convertMapToText(originalNode.Labels))
-		patchTestAnnotations := fmt.Sprintf(`{ "op": "test", "path": "/metadata/annotations", "value": {%s}}`, convertMapToText(originalNode.Annotations))
-		patchLabels := fmt.Sprintf(`{ "op": "replace", "path": "/metadata/labels", "value": {%s}}`, convertMapToText(node.Labels))
-		patchAnnotations := fmt.Sprintf(`{ "op": "replace", "path": "/metadata/annotations", "value": {%s}}`, convertMapToText(node.Annotations))
-		data := []byte(fmt.Sprintf("[ %s, %s, %s, %s ]", patchTestLabels, patchLabels, patchTestAnnotations, patchAnnotations))
-		_, err = n.clientset.CoreV1().Nodes().Patch(node.Name, types.JSONPatchType, data)
-		if err != nil {
-			n.logger.Infof("error during node %s update. %s\n", node.Name, err)
-			return err
-		}
-	}
-
-	return nil
+	err = n.patchNode(originalNode, node)
+	return err
 }
 
 func convertMapToText(m map[string]string) string {
@@ -188,6 +173,42 @@ func convertMapToText(m map[string]string) string {
 	return text
 }
 
+func (n *NodeLabeller) patchNode(originalNode, node *v1.Node) error {
+	originalLabelsBytes, err := json.Marshal(originalNode.Labels)
+	if err != nil {
+		return err
+	}
+
+	originalAnnotationsBytes, err := json.Marshal(originalNode.Annotations)
+	if err != nil {
+		return err
+	}
+
+	labelsBytes, err := json.Marshal(node.Labels)
+	if err != nil {
+		return err
+	}
+
+	annotationsBytes, err := json.Marshal(node.Annotations)
+	if err != nil {
+		return err
+	}
+
+	if !reflect.DeepEqual(node.Labels, originalNode.Labels) {
+		patchTestLabels := fmt.Sprintf(`{ "op": "test", "path": "/metadata/labels", "value": %s}`, string(originalLabelsBytes))
+		patchTestAnnotations := fmt.Sprintf(`{ "op": "test", "path": "/metadata/annotations", "value": %s}`, string(originalAnnotationsBytes))
+		patchLabels := fmt.Sprintf(`{ "op": "replace", "path": "/metadata/labels", "value": %s}`, string(labelsBytes))
+		patchAnnotations := fmt.Sprintf(`{ "op": "replace", "path": "/metadata/annotations", "value": %s}`, string(annotationsBytes))
+		data := []byte(fmt.Sprintf("[ %s, %s, %s, %s ]", patchTestLabels, patchLabels, patchTestAnnotations, patchAnnotations))
+		_, err = n.clientset.CoreV1().Nodes().Patch(node.Name, types.JSONPatchType, data)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // prepareLabels converts cpu models + features to map[string]string format
 // e.g. "/cpu-model-Penryn": "true"
 func (n *NodeLabeller) prepareLabels(cpuModels []string, cpuFeatures map[string]bool) map[string]string {

pkg/virt-operator/install-strategy/create.go

@@ -676,6 +676,11 @@ func createOrUpdateConfigMaps(kv *v1.KubeVirt,
 	id := kv.Status.TargetDeploymentID
 
 	for _, cm := range targetStrategy.configMaps {
+
+		if cm.Name == components.KubeVirtCASecretName {
+			continue
+		}
+
 		var cachedCM *corev1.ConfigMap
 
 		cm := cm.DeepCopy()

pkg/virt-operator/install-strategy/strategy.go

@@ -211,6 +211,7 @@ func dumpInstallStrategyToBytes(strategy *InstallStrategy) []byte {
 		marshalutil.MarshallObject(entry, writer)
 	}
 	for _, entry := range strategy.configMaps {
+		fmt.Printf("%#v\n", entry)
 		marshalutil.MarshallObject(entry, writer)
 	}
 	writer.Flush()
@@ -291,7 +292,7 @@ func GenerateCurrentInstallStrategy(config *operatorutil.KubeVirtDeploymentConfi
 	strategy.deployments = append(strategy.deployments, controller)
 
 	nodeLabellerConfigMap := components.NewNodeLabellerConfigMap(config.GetNamespace())
-	strategy.configMaps = append(strategy.configMaps, nodeLabellerConfigMap)
+	strategy.configMaps = append(strategy.configMaps, nodeLabellerConfigMap, components.NewKubeVirtCAConfigMap(operatorNamespace))
 
 	handler, err := components.NewHandlerDaemonSet(config.GetNamespace(), config.GetImageRegistry(), config.GetImagePrefix(), config.GetHandlerVersion(), config.GetLauncherVersion(), config.GetImagePullPolicy(), config.GetVerbosity())
 	if err != nil {
@@ -320,7 +321,6 @@ func GenerateCurrentInstallStrategy(config *operatorutil.KubeVirtDeploymentConfi
 	strategy.apiServices = components.NewVirtAPIAPIServices(config.GetNamespace())
 	strategy.certificateSecrets = components.NewCertSecrets(config.GetNamespace(), operatorNamespace)
 	strategy.certificateSecrets = append(strategy.certificateSecrets, components.NewCACertSecret(operatorNamespace))
-	strategy.configMaps = append(strategy.configMaps, components.NewKubeVirtCAConfigMap(operatorNamespace))
 
 	return strategy, nil
 }

pkg/virt-operator/install-strategy/strategy_test.go

@@ -166,6 +166,10 @@ var _ = Describe("Install Strategy", func() {
 						break
 					}
 				}
+				//delete ManagedByLabel labels from original config map.
+				//dumpInstallStrategyToBytes function deletes it, and then
+				//original and converted configmaps are not the same
+				delete(original.Labels, v1.ManagedByLabel)
 				Expect(reflect.DeepEqual(original, converted)).To(BeTrue())
 			}
 		})

pkg/virt-operator/kubevirt.go

@@ -606,7 +606,7 @@ func (c *KubeVirtController) execute(key string) error {
 		return nil
 	}
 
-	logger.Info("Handling KubeVirt resource " + key)
+	logger.Info("Handling KubeVirt resource")
 
 	// only process the kubevirt deployment if all expectations are satisfied.
 	needsSync := c.kubeVirtExpectations.SatisfiedExpectations(key)
@@ -1119,6 +1119,7 @@ func (c *KubeVirtController) syncDeletion(kv *v1.KubeVirt) error {
 
 		return nil
 	}
+
 	logger.Info("Processed deletion for this round")
 	return nil
 }

pkg/virt-operator/kubevirt_test.go

@@ -142,8 +142,8 @@ var _ = Describe("KubeVirt Operator", func() {
 	var totalDeletions int
 	var resourceChanges map[string]map[string]int
 
-	resourceCount := 49
-	patchCount := 30
+	resourceCount := 50
+	patchCount := 31
 	updateCount := 20
 
 	deleteFromCache := true
@@ -275,9 +275,6 @@ var _ = Describe("KubeVirt Operator", func() {
 		informers.InstallStrategyConfigMap, installStrategyConfigMapSource = testutils.NewFakeInformerFor(&k8sv1.ConfigMap{})
 		stores.InstallStrategyConfigMapCache = informers.InstallStrategyConfigMap.GetStore()
 
-		informers.ConfigMap, configMapSource = testutils.NewFakeInformerFor(&k8sv1.ConfigMap{})
-		stores.ConfigMapCache = informers.ConfigMap.GetStore()
-
 		informers.InstallStrategyJob, installStrategyJobSource = testutils.NewFakeInformerFor(&batchv1.Job{})
 		stores.InstallStrategyJobCache = informers.InstallStrategyJob.GetStore()
 
@@ -308,6 +305,7 @@ var _ = Describe("KubeVirt Operator", func() {
 
 		informers.Secrets, secretsSource = testutils.NewFakeInformerFor(&k8sv1.Secret{})
 		stores.SecretCache = informers.Secrets.GetStore()
+
 		informers.ConfigMap, configMapSource = testutils.NewFakeInformerFor(&k8sv1.ConfigMap{})
 		stores.ConfigMapCache = informers.ConfigMap.GetStore()
 
@@ -478,12 +476,6 @@ var _ = Describe("KubeVirt Operator", func() {
 		mockQueue.Wait()
 	}
 
-	addConfigMap := func(c *k8sv1.ConfigMap) {
-		mockQueue.ExpectAdds(1)
-		configMapSource.Add(c)
-		mockQueue.Wait()
-	}
-
 	addInstallStrategyJob := func(job *batchv1.Job) {
 		mockQueue.ExpectAdds(1)
 		installStrategyJobSource.Add(job)
@@ -515,6 +507,7 @@ var _ = Describe("KubeVirt Operator", func() {
 		} else {
 			configMapSource.Add(configMap)
 		}
+
 		mockQueue.Wait()
 	}
 
@@ -578,14 +571,8 @@ var _ = Describe("KubeVirt Operator", func() {
 			injectMetadata(&obj.(*batchv1.Job).ObjectMeta, config)
 			addInstallStrategyJob(resource)
 		case *k8sv1.ConfigMap:
-			cm := obj.(*k8sv1.ConfigMap)
-			injectMetadata(&cm.ObjectMeta, config)
-
-			if strings.Contains(cm.Name, "kubevirt-cpu-plugin-configmap") {
-				addConfigMap(resource)
-			} else {
-				addInstallStrategyConfigMap(resource)
-			}
+			injectMetadata(&obj.(*k8sv1.ConfigMap).ObjectMeta, config)
+			addConfigMap(resource)
 		case *k8sv1.Pod:
 			injectMetadata(&obj.(*k8sv1.Pod).ObjectMeta, config)
 			addPod(resource)
@@ -865,8 +852,7 @@ var _ = Describe("KubeVirt Operator", func() {
 		controller, _ := components.NewControllerDeployment(NAMESPACE, config.GetImageRegistry(), config.GetImagePrefix(), config.GetControllerVersion(), config.GetLauncherVersion(), config.GetImagePullPolicy(), config.GetVerbosity())
 		controllerPdb := components.NewPodDisruptionBudgetForDeployment(controller)
 		handler, _ := components.NewHandlerDaemonSet(NAMESPACE, config.GetImageRegistry(), config.GetImagePrefix(), config.GetLauncherVersion(), config.GetHandlerVersion(), config.GetImagePullPolicy(), config.GetVerbosity())
-		configMap := components.NewNodeLabellerConfigMap(NAMESPACE)
-		all = append(all, apiDeployment, apiDeploymentPdb, controller, controllerPdb, handler, configMap)
+		all = append(all, apiDeployment, apiDeploymentPdb, controller, controllerPdb, handler)
 
 		all = append(all, rbac.GetAllServiceMonitor(NAMESPACE, config.GetMonitorNamespace(), config.GetMonitorServiceAccount())...)
 		all = append(all, components.NewServiceMonitorCR(NAMESPACE, config.GetMonitorNamespace(), true))
@@ -1091,14 +1077,6 @@ var _ = Describe("KubeVirt Operator", func() {
 		mockQueue.Wait()
 	}
 
-	deleteConfigMap := func(key string) {
-		mockQueue.ExpectAdds(1)
-		if obj, exists, _ := informers.ConfigMap.GetStore().GetByKey(key); exists {
-			configMapSource.Delete(obj.(runtime.Object))
-		}
-		mockQueue.Wait()
-	}
-
 	deletePodDisruptionBudget := func(key string) {
 		mockQueue.ExpectAdds(1)
 		if obj, exists, _ := informers.PodDisruptionBudget.GetStore().GetByKey(key); exists {
@@ -1180,11 +1158,7 @@ var _ = Describe("KubeVirt Operator", func() {
 		case "jobs":
 			deleteInstallStrategyJob(key)
 		case "configmaps":
-			if strings.Contains(key, "kubevirt-cpu-plugin-configmap") {
-				deleteConfigMap(key)
-			} else {
-				deleteInstallStrategyConfigMap(key)
-			}
+			deleteConfigMap(key)
 		case "poddisruptionbudgets":
 			deletePodDisruptionBudget(key)
 		case "secrets":
@@ -1341,7 +1315,6 @@ var _ = Describe("KubeVirt Operator", func() {
 		kubeClient.Fake.PrependReactor("patch", "services", genericPatchFunc)
 		kubeClient.Fake.PrependReactor("patch", "daemonsets", genericPatchFunc)
 		kubeClient.Fake.PrependReactor("patch", "deployments", genericPatchFunc)
-		kubeClient.Fake.PrependReactor("patch", "configmaps", genericPatchFunc)
 		kubeClient.Fake.PrependReactor("patch", "poddisruptionbudgets", genericPatchFunc)
 		secClient.Fake.PrependReactor("update", "securitycontextconstraints", genericUpdateFunc)
 		promClient.Fake.PrependReactor("patch", "servicemonitors", genericPatchFunc)
@@ -1379,7 +1352,6 @@ var _ = Describe("KubeVirt Operator", func() {
 		kubeClient.Fake.PrependReactor("create", "validatingwebhookconfigurations", genericCreateFunc)
 		kubeClient.Fake.PrependReactor("create", "mutatingwebhookconfigurations", genericCreateFunc)
 		kubeClient.Fake.PrependReactor("create", "secrets", genericCreateFunc)
-		kubeClient.Fake.PrependReactor("create", "configmaps", genericCreateFunc)
 		kubeClient.Fake.PrependReactor("create", "poddisruptionbudgets", genericCreateFunc)
 		secClient.Fake.PrependReactor("create", "securitycontextconstraints", genericCreateFunc)
 		promClient.Fake.PrependReactor("create", "servicemonitors", genericCreateFunc)
@@ -1898,7 +1870,7 @@ var _ = Describe("KubeVirt Operator", func() {
 			Expect(len(controller.stores.SCCCache.List())).To(Equal(3))
 			Expect(len(controller.stores.ServiceMonitorCache.List())).To(Equal(1))
 			Expect(len(controller.stores.PrometheusRuleCache.List())).To(Equal(1))
-			Expect(len(controller.stores.ConfigMapCache.List())).To(Equal(1))
+			Expect(len(controller.stores.ConfigMapCache.List())).To(Equal(2))
 
 			Expect(resourceChanges["poddisruptionbudgets"][Added]).To(Equal(1))