Fix issue with concurrent CR creation 04/95104/3
authorDileep Ranganathan <dileep.ranganathan@intel.com>
Fri, 6 Sep 2019 04:46:59 +0000 (21:46 -0700)
committerMarco Platania <platania@research.att.com>
Fri, 6 Sep 2019 12:45:28 +0000 (12:45 +0000)
The collectd operator is going into deadlock when concurrent update
operations happen within the same controller trying to update the
resource. Fixed this by adding Mutex.

Deleted the old build_image.sh which is replaced by new script which
builds and pushes from inside a builder docker container. This helps in
tackling the dependency issues for image build.

Updated the README for build image script usage.

Issue-ID: ONAPARC-461
Signed-off-by: Dileep Ranganathan <dileep.ranganathan@intel.com>
Change-Id: Ib3c2d1edd266e70bb713885de7ad046ebf5ad086

vnfs/DAaaS/README.md
vnfs/DAaaS/microservices/collectd-operator/Makefile
vnfs/DAaaS/microservices/collectd-operator/build/build_image.sh [deleted file]
vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1/zz_generated.openapi.go
vnfs/DAaaS/microservices/collectd-operator/pkg/controller/collectdglobal/collectdglobal_controller.go
vnfs/DAaaS/microservices/collectd-operator/pkg/controller/collectdplugin/collectdplugin_controller.go
vnfs/DAaaS/microservices/collectd-operator/pkg/controller/utils/collectdutils.go
vnfs/DAaaS/microservices/collectd-operator/pkg/controller/utils/dsutils.go

index 80f0d86..68d0401 100644 (file)
@@ -84,11 +84,11 @@ helm install -n rook . -f values.yaml --namespace=rook-ceph-system
 ### Build docker images
 #### collectd-operator
 ```bash
-cd $DA_WORKING_DIR/../microservices/collectd-operator
+cd $DA_WORKING_DIR/../microservices
 
 ## Note: The image tag and respository in the Collectd-operator helm charts needs to match the IMAGE_NAME
 IMAGE_NAME=dcr.cluster.local:32644/collectd-operator:latest
-./build/build_image.sh $IMAGE_NAME
+./build_image.sh collectd-operator $IMAGE_NAME
 ```
 #### visualization-operator
 ```bash
index af2ac66..c09f4c2 100644 (file)
@@ -24,7 +24,7 @@ export GO111MODULE=on
 .PHONY: clean plugins
 
 ## build: Generate the k8s and openapi artifacts using operator-sdk
-build: clean
+build: clean format
        GOOS=linux GOARCH=amd64
        operator-sdk generate k8s --verbose
        operator-sdk generate openapi --verbose
diff --git a/vnfs/DAaaS/microservices/collectd-operator/build/build_image.sh b/vnfs/DAaaS/microservices/collectd-operator/build/build_image.sh
deleted file mode 100755 (executable)
index 9cda67d..0000000
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/bash
-set -e
-set -x
-
-sudo rm -rf /usr/local/go
-sudo apt-get install make mercurial
-wget https://dl.google.com/go/go1.12.9.linux-amd64.tar.gz
-sudo tar -xvf go1.12.9.linux-amd64.tar.gz
-sudo mv go /usr/local
-export GOROOT=/usr/local/go
-export PATH=$PATH:/usr/local/go/bin
-export GO111MODULE=on
-
-RELEASE_VERSION=v0.9.0
-curl -OJL https://github.com/operator-framework/operator-sdk/releases/download/${RELEASE_VERSION}/operator-sdk-${RELEASE_VERSION}-x86_64-linux-gnu
-
-chmod +x operator-sdk-${RELEASE_VERSION}-x86_64-linux-gnu && sudo cp operator-sdk-${RELEASE_VERSION}-x86_64-linux-gnu /usr/local/bin/operator-sdk && rm operator-sdk-${RELEASE_VERSION}-x86_64-linux-gnu
-IMAGE_NAME=$1
-if [ -z "$IMAGE_NAME" ]
-then
-    echo "Building Collectd-Operator image with default image name"
-    make
-else
-    echo "Building Collectd-Operator image $IMAGE_NAME"
-    make IMAGE_NAME=$IMAGE_NAME
-fi
-rm -rf go1.12.9.linux-amd64.tar.gz
index 85e49f4..0c80396 100644 (file)
@@ -11,12 +11,12 @@ import (
 
 func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition {
        return map[string]common.OpenAPIDefinition{
-               "./pkg/apis/onap/v1alpha1.CollectdGlobal":       schema_pkg_apis_onap_v1alpha1_CollectdGlobal(ref),
-               "./pkg/apis/onap/v1alpha1.CollectdGlobalSpec":   schema_pkg_apis_onap_v1alpha1_CollectdGlobalSpec(ref),
-               "./pkg/apis/onap/v1alpha1.CollectdGlobalStatus": schema_pkg_apis_onap_v1alpha1_CollectdGlobalStatus(ref),
-               "./pkg/apis/onap/v1alpha1.CollectdPlugin":       schema_pkg_apis_onap_v1alpha1_CollectdPlugin(ref),
-               "./pkg/apis/onap/v1alpha1.CollectdPluginSpec":   schema_pkg_apis_onap_v1alpha1_CollectdPluginSpec(ref),
-               "./pkg/apis/onap/v1alpha1.CollectdPluginStatus": schema_pkg_apis_onap_v1alpha1_CollectdPluginStatus(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobal":       schema_pkg_apis_onap_v1alpha1_CollectdGlobal(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalSpec":   schema_pkg_apis_onap_v1alpha1_CollectdGlobalSpec(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalStatus": schema_pkg_apis_onap_v1alpha1_CollectdGlobalStatus(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPlugin":       schema_pkg_apis_onap_v1alpha1_CollectdPlugin(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginSpec":   schema_pkg_apis_onap_v1alpha1_CollectdPluginSpec(ref),
+               "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginStatus": schema_pkg_apis_onap_v1alpha1_CollectdPluginStatus(ref),
        }
 }
 
@@ -47,19 +47,19 @@ func schema_pkg_apis_onap_v1alpha1_CollectdGlobal(ref common.ReferenceCallback)
                                        },
                                        "spec": {
                                                SchemaProps: spec.SchemaProps{
-                                                       Ref: ref("./pkg/apis/onap/v1alpha1.CollectdGlobalSpec"),
+                                                       Ref: ref("demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalSpec"),
                                                },
                                        },
                                        "status": {
                                                SchemaProps: spec.SchemaProps{
-                                                       Ref: ref("./pkg/apis/onap/v1alpha1.CollectdGlobalStatus"),
+                                                       Ref: ref("demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalStatus"),
                                                },
                                        },
                                },
                        },
                },
                Dependencies: []string{
-                       "./pkg/apis/onap/v1alpha1.CollectdGlobalSpec", "./pkg/apis/onap/v1alpha1.CollectdGlobalStatus", "k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta"},
+                       "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalSpec", "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdGlobalStatus", "k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta"},
        }
 }
 
@@ -151,19 +151,19 @@ func schema_pkg_apis_onap_v1alpha1_CollectdPlugin(ref common.ReferenceCallback)
                                        },
                                        "spec": {
                                                SchemaProps: spec.SchemaProps{
-                                                       Ref: ref("./pkg/apis/onap/v1alpha1.CollectdPluginSpec"),
+                                                       Ref: ref("demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginSpec"),
                                                },
                                        },
                                        "status": {
                                                SchemaProps: spec.SchemaProps{
-                                                       Ref: ref("./pkg/apis/onap/v1alpha1.CollectdPluginStatus"),
+                                                       Ref: ref("demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginStatus"),
                                                },
                                        },
                                },
                        },
                },
                Dependencies: []string{
-                       "./pkg/apis/onap/v1alpha1.CollectdPluginSpec", "./pkg/apis/onap/v1alpha1.CollectdPluginStatus", "k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta"},
+                       "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginSpec", "demo/vnfs/DAaaS/microservices/collectd-operator/pkg/apis/onap/v1alpha1.CollectdPluginStatus", "k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta"},
        }
 }
 
index 0c4064e..a43afdc 100644 (file)
@@ -149,38 +149,32 @@ func (r *ReconcileCollectdGlobal) Reconcile(request reconcile.Request) (reconcil
 
 // handleCollectdGlobal regenerates the collectd conf on CR Create, Update, Delete events
 func (r *ReconcileCollectdGlobal) handleCollectdGlobal(reqLogger logr.Logger, cr *onapv1alpha1.CollectdGlobal, isDelete bool) error {
+       var collectdConf string
 
-       rmap, err := collectdutils.FindResourceMapForCR(r.client, reqLogger, cr.Namespace)
-       if err != nil {
-               reqLogger.Info(":::: Skip current reconcile:::: Resources not found. Cache might be stale. Requeue")
-               return err
-       }
+       retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+               cm, err := collectdutils.GetConfigMap(r.client, reqLogger, cr.Namespace)
+               if err != nil {
+                       reqLogger.Info(":::: Skip current reconcile:::: ConfigMap not found. Cache might be stale. Requeue")
+                       return err
+               }
 
-       cm := rmap.ConfigMap
-       reqLogger.V(1).Info("Found ResourceMap")
-       reqLogger.V(1).Info(":::: ConfigMap Info ::::", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
+               reqLogger.V(1).Info(":::: ConfigMap Info ::::", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
 
-       collectdConf, err := collectdutils.RebuildCollectdConf(r.client, cr.Namespace, isDelete, "")
-       if err != nil {
-               reqLogger.Error(err, "Skip reconcile: Rebuild conf failed")
-               return err
-       }
+               collectdConf, err := collectdutils.RebuildCollectdConf(r.client, cr.Namespace, isDelete, "")
+               if err != nil {
+                       reqLogger.Error(err, "Skip reconcile: Rebuild conf failed")
+                       return err
+               }
 
-       cm.SetAnnotations(map[string]string{
-               "daaas-random": collectdutils.ComputeSHA256([]byte(collectdConf)),
-       })
+               cm.SetAnnotations(map[string]string{
+                       "daaas-random": collectdutils.ComputeSHA256([]byte(collectdConf)),
+               })
 
-       cm.Data["collectd.conf"] = collectdConf
-       retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+               cm.Data["collectd.conf"] = collectdConf
                // Update the ConfigMap with new Spec and reload DaemonSets
                reqLogger.Info("Updating the ConfigMap", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
-               log.V(1).Info("ConfigMap Data", "Map: ", cm.Data)
-               err = r.client.Update(context.TODO(), cm)
-               if err != nil {
-                       reqLogger.Error(err, "Update the ConfigMap failed", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
-                       return err
-               }
-               return nil
+               updateErr := r.client.Update(context.TODO(), cm)
+               return updateErr
        })
        if retryErr != nil {
                panic(fmt.Errorf("Update failed: %v", retryErr))
@@ -221,7 +215,7 @@ func (r *ReconcileCollectdGlobal) handleCollectdGlobal(reqLogger logr.Logger, cr
                panic(fmt.Errorf("Update failed: %v", retryErr))
        }
 
-       err = r.updateStatus(cr)
+       err := r.updateStatus(cr)
        if err != nil {
                reqLogger.Error(err, "Unable to update status")
                return err
index 9329c55..644a6bb 100644 (file)
@@ -5,6 +5,7 @@ import (
        "fmt"
        "reflect"
        "strings"
+       "sync"
 
        "github.com/go-logr/logr"
        "github.com/operator-framework/operator-sdk/pkg/predicate"
@@ -28,6 +29,8 @@ import (
 
 var log = logf.Log.WithName("controller_collectdplugin")
 
+var reconcileLock sync.Mutex
+
 // Add creates a new CollectdPlugin Controller and adds it to the Manager. The Manager will set fields on the Controller
 // and Start it when the Manager is Started.
 func Add(mgr manager.Manager) error {
@@ -142,45 +145,43 @@ func (r *ReconcileCollectdPlugin) Reconcile(request reconcile.Request) (reconcil
        }
        // Handle the reconciliation for CollectdPlugin.
        // At this stage the Status of the CollectdPlugin should NOT be ""
+       reconcileLock.Lock()
        err = r.handleCollectdPlugin(reqLogger, instance, false)
+       reconcileLock.Unlock()
        return reconcile.Result{}, err
 }
 
 // handleCollectdPlugin regenerates the collectd conf on CR Create, Update, Delete events
 func (r *ReconcileCollectdPlugin) handleCollectdPlugin(reqLogger logr.Logger, cr *onapv1alpha1.CollectdPlugin, isDelete bool) error {
+       var collectdConf string
+       retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+               cm, err := collectdutils.GetConfigMap(r.client, reqLogger, cr.Namespace)
+               if err != nil {
+                       reqLogger.Error(err, "Skip reconcile: ConfigMap not found")
+                       return err
+               }
+               reqLogger.V(1).Info(":::: ConfigMap Info ::::", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
 
-       rmap, err := collectdutils.FindResourceMapForCR(r.client, reqLogger, cr.Namespace)
-       if err != nil {
-               reqLogger.Error(err, "Skip reconcile: Resources not found")
-               return err
-       }
-
-       cm := rmap.ConfigMap
-       reqLogger.V(1).Info("Found ResourceMap")
-       reqLogger.V(1).Info(":::: ConfigMap Info ::::", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
+               collectdConf, err := collectdutils.RebuildCollectdConf(r.client, cr.Namespace, isDelete, cr.Spec.PluginName)
+               if err != nil {
+                       reqLogger.Error(err, "Skip reconcile: Rebuild conf failed")
+                       return err
+               }
 
-       collectdConf, err := collectdutils.RebuildCollectdConf(r.client, cr.Namespace, isDelete, cr.Spec.PluginName)
-       if err != nil {
-               reqLogger.Error(err, "Skip reconcile: Rebuild conf failed")
-               return err
-       }
+               cm.SetAnnotations(map[string]string{
+                       "daaas-random": collectdutils.ComputeSHA256([]byte(collectdConf)),
+               })
+               cm.Data["collectd.conf"] = collectdConf
 
-       cm.SetAnnotations(map[string]string{
-               "daaas-random": collectdutils.ComputeSHA256([]byte(collectdConf)),
+               // Update the ConfigMap with new Spec and reload DaemonSets
+               reqLogger.Info("Updating the ConfigMap", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
+               updateErr := r.client.Update(context.TODO(), cm)
+               return updateErr
        })
-
-       cm.Data["collectd.conf"] = collectdConf
-
-       // Update the ConfigMap with new Spec and reload DaemonSets
-       reqLogger.Info("Updating the ConfigMap", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
-       log.V(1).Info("ConfigMap Data", "Map: ", cm.Data)
-       err = r.client.Update(context.TODO(), cm)
-       if err != nil {
-               reqLogger.Error(err, "Update the ConfigMap failed", "ConfigMap.Namespace", cm.Namespace, "ConfigMap.Name", cm.Name)
-               return err
+       if retryErr != nil {
+               panic(fmt.Errorf("Update failed: %v", retryErr))
        }
-
-       retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+       retryErr = retry.RetryOnConflict(retry.DefaultRetry, func() error {
                // Retrieve the latest version of Daemonset before attempting update
                // RetryOnConflict uses exponential backoff to avoid exhausting the apiserver
                // Select DaemonSets with label
@@ -214,7 +215,7 @@ func (r *ReconcileCollectdPlugin) handleCollectdPlugin(reqLogger logr.Logger, cr
                panic(fmt.Errorf("Update failed: %v", retryErr))
        }
 
-       err = r.updateStatus(cr)
+       err := r.updateStatus(cr)
        if err != nil {
                reqLogger.Error(err, "Unable to update status")
                return err
index 9560483..6a85103 100644 (file)
@@ -72,14 +72,28 @@ func GetWatchLabels() (string, error) {
        return labelSelector, nil
 }
 
-// FindResourceMapForCR returns the configMap, collectd Daemonset and list of Collectd Plugins
-func FindResourceMapForCR(rc client.Client, reqLogger logr.Logger, ns string) (*ResourceMap, error) {
+// GetCollectdPluginList returns the list of CollectdPlugin instances in the namespace ns
+func GetCollectdPluginList(rc client.Client, ns string) (*onapv1alpha1.CollectdPluginList, error) {
+       // Get all collectd plugins in the current namespace to rebuild conf.
+       collectdPlugins := &onapv1alpha1.CollectdPluginList{}
+       cpOpts := &client.ListOptions{}
+       cpOpts.InNamespace(ns)
+       err := rc.List(context.TODO(), cpOpts, collectdPlugins)
+       if err != nil {
+               return nil, err
+       }
+       return collectdPlugins, nil
+}
+
+// GetConfigMap returns the GetConfigMap in the namespace ns
+func GetConfigMap(rc client.Client, reqLogger logr.Logger, ns string) (*corev1.ConfigMap, error) {
        lock.Lock()
        defer lock.Unlock()
+
+       reqLogger.Info("Get ConfigMap for collectd.conf")
+       // Get all collectd plugins in the current namespace to rebuild conf.
        cmList := &corev1.ConfigMapList{}
        opts := &client.ListOptions{}
-       rmap := &ResourceMap{}
-
        // Select ConfigMaps with label
        labelSelector, err := GetWatchLabels()
        if err != nil {
@@ -90,41 +104,15 @@ func FindResourceMapForCR(rc client.Client, reqLogger logr.Logger, ns string) (*
 
        err = rc.List(context.TODO(), opts, cmList)
        if err != nil {
-               return rmap, err
+               return nil, err
        }
 
        if cmList.Items == nil || len(cmList.Items) == 0 {
-               return rmap, errors.NewNotFound(corev1.Resource("configmap"), "ConfigMap")
-       }
-
-       // Select DaemonSets with label
-       dsList := &appsv1.DaemonSetList{}
-       err = rc.List(context.TODO(), opts, dsList)
-       if err != nil {
-               return rmap, err
+               return nil, errors.NewNotFound(corev1.Resource("configmap"), "ConfigMap")
        }
 
-       if dsList.Items == nil || len(dsList.Items) == 0 {
-               return rmap, errors.NewNotFound(corev1.Resource("daemonset"), "DaemonSet")
-       }
-
-       rmap.ConfigMap = &cmList.Items[0]
-       rmap.DaemonSet = &dsList.Items[0]
-
-       return rmap, err
-}
-
-// GetCollectdPluginList returns the list of CollectdPlugin instances in the namespace ns
-func GetCollectdPluginList(rc client.Client, ns string) (*onapv1alpha1.CollectdPluginList, error) {
-       // Get all collectd plugins in the current namespace to rebuild conf.
-       collectdPlugins := &onapv1alpha1.CollectdPluginList{}
-       cpOpts := &client.ListOptions{}
-       cpOpts.InNamespace(ns)
-       err := rc.List(context.TODO(), cpOpts, collectdPlugins)
-       if err != nil {
-               return nil, err
-       }
-       return collectdPlugins, nil
+       cm := &cmList.Items[0]
+       return cm, nil
 }
 
 // GetCollectdGlobal returns the CollectdGlobal instance in the namespace ns
index d52a68c..c5a44c4 100644 (file)
@@ -2,8 +2,8 @@ package utils
 
 import (
        "path/filepath"
-       "strings"
        "strconv"
+       "strings"
 
        onapv1alpha1 "collectd-operator/pkg/apis/onap/v1alpha1"
 
@@ -26,7 +26,7 @@ const (
 // RemoveTypesDB - removes TypesDB volumes and volume mounts from collectd pods.
 func RemoveTypesDB(ds *appsv1.DaemonSet) {
        vols := &ds.Spec.Template.Spec.Volumes
-       for i:=0; i < len(*vols); i++ {
+       for i := 0; i < len(*vols); i++ {
                if (*vols)[i].Name == typesDB {
                        *vols = append((*vols)[:i], (*vols)[i+1:]...)
                        i--
@@ -37,7 +37,7 @@ func RemoveTypesDB(ds *appsv1.DaemonSet) {
        for j, container := range *containers {
                if container.Name == collectdContainerName {
                        vms := &(*containers)[j].VolumeMounts
-                       for i:=0; i < len(*vms); i++ {
+                       for i := 0; i < len(*vms); i++ {
                                if (*vms)[i].Name == typesDB {
                                        *vms = append((*vms)[:i], (*vms)[i+1:]...)
                                        i--
@@ -81,7 +81,7 @@ func UpsertTypesDB(ds *appsv1.DaemonSet, cm *corev1.ConfigMap, cr *onapv1alpha1.
        for j, container := range *containers {
                if container.Name == collectdContainerName {
                        vms := &(*containers)[j].VolumeMounts
-                       for i:=0; i < len(*vms); i++ {
+                       for i := 0; i < len(*vms); i++ {
                                // Update case (Equivalent to remove and add)
                                if (*vms)[i].Name == typesDB {
                                        *vms = append((*vms)[:i], (*vms)[i+1:]...)
@@ -104,7 +104,7 @@ func findMountInfo(cr *onapv1alpha1.CollectdGlobal) *[]corev1.VolumeMount {
                s := strings.Fields(globalOpt)
                log.V(1).Info(":::::s:::::", "s:", s)
                if s != nil && len(s) != 0 && s[0] == "TypesDB" {
-                       path,_ := strconv.Unquote(s[1])
+                       path, _ := strconv.Unquote(s[1])
                        _, file := filepath.Split(path)
                        log.V(1).Info(":::::file:::::", "s[1]:", path, "file:", file)
                        vm := corev1.VolumeMount{Name: typesDB, MountPath: path, SubPath: file}