Repository: odahu/odahu-flow (language: python)

Issue:
`roleName` ODAHU deployment changes doesn't take effect. 
**Description**
Deployment `roleName` field update doesn't actually trigger changes.

**Steps to Reproduce**
1. Create ODAHU deployment
2. Go to deployment page
3. Click `YAML` tab
4. Change `roleName` field value
5. Click `Submit` 

**Actual result**
Knative configuration doesn't change, revision is not created, pod doesn't restart, Authentication works via role that was previously in configuration.

**Expected result**
Knative configuration entity should change, then new model deployment revision should be created and new pod should be deployed. The new pod should authenticate via provided role. UI shows that deployment is ready and the new role name is displayed in the deployment configuration window and YAML.

**Environment:**
 - k8s version: 1.17
 - ODAHU version: 1.5.0-rc2


Discussion / hints:

Fix patch:
diff --git a/packages/operator/controllers/modeldeployment_controller.go b/packages/operator/controllers/modeldeployment_controller.go
index 29f0be45d..06c994436 100644
--- a/packages/operator/controllers/modeldeployment_controller.go
+++ b/packages/operator/controllers/modeldeployment_controller.go
@@ -27,6 +27,7 @@ import (
 	"github.com/odahu/odahu-flow/packages/operator/pkg/odahuflow"
 	"github.com/odahu/odahu-flow/packages/operator/pkg/repository/util/kubernetes"
 	"github.com/odahu/odahu-flow/packages/operator/pkg/utils"
+	"github.com/odahu/odahu-flow/packages/operator/pkg/utils/hash"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -67,6 +68,7 @@ const (
 	DefaultKnativeAutoscalingClass       = "kpa.autoscaling.knative.dev"
 	ModelNameAnnotationKey               = "modelName"
 	AppliedModelDeploymentSpecKey        = "odahu.org/applied-model-deployment-spec"
+	AppliedPolicyHashKey                 = "odahu.org/applied-policy-hash"
 
 	IstioRewriteHTTPProbesAnnotation = "sidecar.istio.io/rewriteAppHTTPProbers"
 	OdahuAuthorizationLabel          = "odahu-flow-authorization"
@@ -144,7 +146,9 @@ func (r *ModelDeploymentReconciler) ReconcileKnativeService(
 			ModelNameAnnotationKey:  modelDeploymentCR.Name,
 			deploymentIDLabel:       modelDeploymentCR.Name,
 			OdahuAuthorizationLabel: "enabled",
-			podPolicyLabel:          getCMPolicyName(modelDeploymentCR),
+		}
+		if modelDeploymentCR.Spec.RoleName != nil {  // Otherwise default ConfigMap will be mounted to container
+			templateLabelsToAdd[podPolicyLabel] = GetCMPolicyName(modelDeploymentCR)
 		}
 		templateAnnotationsToAdd := map[string]string{
 			KnativeAutoscalingClass:          DefaultKnativeAutoscalingClass,
@@ -153,6 +157,8 @@ func (r *ModelDeploymentReconciler) ReconcileKnativeService(
 			KnativeMaxReplicasKey:            strconv.Itoa(int(*modelDeploymentCR.Spec.MaxReplicas)),
 			KnativeAutoscalingTargetKey:      KnativeAutoscalingTargetDefaultValue,
 			IstioRewriteHTTPProbesAnnotation: "true",
+			// Annotation to trigger pod restart if policy is changed
+			AppliedPolicyHashKey: modelDeploymentCR.Annotations[AppliedPolicyHashKey],
 		}
 		revisionSpec := knservingv1.RevisionSpec{
 			TimeoutSeconds: &DefaultTerminationPeriod,
@@ -240,26 +246,41 @@ func (r *ModelDeploymentReconciler) ReconcileKnativeService(
 		return err
 	}
 
-	if reflect.DeepEqual(*lastAppliedMDSpec, modelDeploymentCR.Spec) {
-		log.Info("Model Deployment version is up to date, skipping")
-		return nil
-	}
+	depSpecChanged := !reflect.DeepEqual(*lastAppliedMDSpec, modelDeploymentCR.Spec)
 
-	log.Info("Knative Service bases on old MD spec", "old", lastAppliedMDSpec,
-		"new", modelDeploymentCR.Spec)
+	oldPolicyHash := found.Spec.ConfigurationSpec.Template.Annotations[AppliedPolicyHashKey]
+	newPolicyHash := modelDeploymentCR.Annotations[AppliedPolicyHashKey]
+	policyIsChanged := newPolicyHash != oldPolicyHash
 
-	err = fulfilKnativeService(found)
-	if err != nil {
-		return err
-	}
-	log.Info(fmt.Sprintf("Updating %s Knative Service, new MD generation: %d",
-		knativeServiceName, modelDeploymentCR.Generation))
-	err = r.Update(context.TODO(), found)
-	if err != nil {
-		return err
-	}
 
+	if depSpecChanged || policyIsChanged {
+		if policyIsChanged {
+			log.Info("Policy hash was changed",
+				"old", oldPolicyHash,
+				"new", newPolicyHash,
+			)
+		}
+		if depSpecChanged {
+			log.Info("ModelDeployment spec was changed","old", lastAppliedMDSpec,
+				"new", modelDeploymentCR.Spec)
+		}
+
+		err = fulfilKnativeService(found)
+		if err != nil {
+			return err
+		}
+		log.Info(fmt.Sprintf("Updating '%s' Knative Service, new MD generation: %d",
+			knativeServiceName, modelDeploymentCR.Generation))
+		err = r.Update(context.TODO(), found)
+		if err != nil {
+			return err
+		}
+
+		return nil
+	}
+	log.Info("ModelDeployment Spec is not changed. Policy is not changed. Skipping.")
 	return nil
+
 }
 
 func (r *ModelDeploymentReconciler) createModelContainer(
@@ -323,7 +344,7 @@ func (r *ModelDeploymentReconciler) reconcileStatus(
 	return nil
 }
 
-func getCMPolicyName(modelDeploymentCR *odahuflowv1alpha1.ModelDeployment) string {
+func GetCMPolicyName(modelDeploymentCR *odahuflowv1alpha1.ModelDeployment) string {
 	return modelDeploymentCR.Name + "-" + cmPolicySuffix
 }
 
@@ -331,20 +352,56 @@ func (r *ModelDeploymentReconciler) reconcilePolicyCM(log logr.Logger,
 	modelDeploymentCR *odahuflowv1alpha1.ModelDeployment, predictor predictors.Predictor) error {
 
 	rn := modelDeploymentCR.Spec.RoleName
-	if rn == nil {
-		log.Info(".Spec.RoleName is nil. Skip creating custom policies for model")
+
+	roleNameIsNotSet := rn == nil
+
+	if roleNameIsNotSet {
+		log.Info(".Spec.RoleName is nil")
+		// We should delete custom policy if roleName is not set
+		cm := &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:     GetCMPolicyName(modelDeploymentCR),
+				Namespace: r.deploymentConfig.Namespace,
+			},
+		}
+		err := r.Delete(context.TODO(), cm)
+		if err != nil && !errors.IsNotFound(err) {
+			return err
+		}
+
+		log.Info("ConfigMap with polices was maybe deleted")
+
+		// Set special hash value to trigger Pod recreation
+		noCustomPolicyHash, err := hash.Hash("Role name is not set")
+		if err != nil {
+			log.Error(err, "Unable to produce configmap hash using policy configmap data")
+			return err
+		}
+		log.Info("Setting no custom policy hash to ModelDeployment Annotation")
+		modelDeploymentCR.Annotations[AppliedPolicyHashKey] = strconv.FormatUint(noCustomPolicyHash, 10)
+
 		return nil
 	}
 
+	// Handle case when roleName is set
+
 	policies, err := deployment.ReadDefaultPoliciesAndRender(*rn, predictor.OpaPolicyFilename)
 	if err != nil {
 		return err
 	}
 
 	cm := deployment.BuildDefaultPolicyConfigMap(
-		getCMPolicyName(modelDeploymentCR), r.deploymentConfig.Namespace, policies,
+		GetCMPolicyName(modelDeploymentCR), r.deploymentConfig.Namespace, policies,
 	)
 
+	policyHash, err := hash.Hash(cm.Data)
+	if err != nil {
+		log.Error(err, "Unable to produce configmap hash using policy configmap data")
+		return err
+	}
+	log.Info("Setting policy hash to ModelDeployment Annotation")
+	modelDeploymentCR.Annotations[AppliedPolicyHashKey] = strconv.FormatUint(policyHash, 10)
+
 	if err := controllerutil.SetControllerReference(modelDeploymentCR, cm, r.scheme); err != nil {
 		return err
 	}
@@ -359,6 +416,11 @@ func (r *ModelDeploymentReconciler) reconcilePolicyCM(log logr.Logger,
 		return r.Create(context.TODO(), cm)
 	}
 
+	if !reflect.DeepEqual(foundCM.Data, cm.Data) {
+		log.Info("Policy was changed. Updating config map")
+		return r.Update(context.TODO(), cm)
+	}
+
 	if err != nil {
 		return err
 	}
@@ -421,6 +483,9 @@ func (r *ModelDeploymentReconciler) Reconcile(request ctrl.Request) (ctrl.Result
 	}
 
 	log.Info("Run reconciling of model deployment")
+	if modelDeploymentCR.Annotations == nil {
+		modelDeploymentCR.Annotations = make(map[string]string)
+	}
 
 	predictor, ok := predictors.Predictors[modelDeploymentCR.Spec.Predictor]
 	if !ok {


Test patch:
diff --git a/packages/operator/controllers/modeldeployment_controller_test.go b/packages/operator/controllers/modeldeployment_controller_test.go
index bd6e5c4cd..93e8e88b1 100644
--- a/packages/operator/controllers/modeldeployment_controller_test.go
+++ b/packages/operator/controllers/modeldeployment_controller_test.go
@@ -22,7 +22,9 @@ import (
 	odahuflowv1alpha1 "github.com/odahu/odahu-flow/packages/operator/api/v1alpha1"
 	. "github.com/odahu/odahu-flow/packages/operator/controllers"
 	"github.com/odahu/odahu-flow/packages/operator/pkg/config"
+	"github.com/odahu/odahu-flow/packages/operator/pkg/deployment"
 	"github.com/odahu/odahu-flow/packages/operator/pkg/repository/util/kubernetes"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -145,7 +147,7 @@ func (s *ModelDeploymentControllerSuite) TestReconcile() {
 	knativeService := s.getKnativeService(md)
 
 	podAnnonations := knativeService.Spec.Template.ObjectMeta.Annotations
-	s.Assertions.Len(podAnnonations, 6)
+	s.Assertions.Len(podAnnonations, 7)
 
 	s.Assertions.Contains(podAnnonations, KnativeMinReplicasKey)
 	s.Assertions.Equal(podAnnonations[KnativeMinReplicasKey], strconv.Itoa(int(mdMinReplicas)))
@@ -268,6 +270,68 @@ func (s *ModelDeploymentControllerSuite) TestDeploymentReconcile_Tolerations() {
 	s.Assertions.Equal(tolerations, knativeConfiguration.Spec.Template.Spec.Tolerations)
 }
 
+func (s *ModelDeploymentControllerSuite) TestPolicyCM() {
+	conf := config.NewDefaultModelDeploymentConfig()
+	conf.Namespace = mdNamespace
+	s.initReconciler(conf)
+
+	firstRole := "first-role"
+
+	md := &odahuflowv1alpha1.ModelDeployment{
+		ObjectMeta: metav1.ObjectMeta{Name: mdName, Namespace: mdNamespace},
+		Spec: odahuflowv1alpha1.ModelDeploymentSpec{
+			RoleName: &firstRole,
+			Image:                      image,
+			Predictor:                  predictors.OdahuMLServer.ID,
+			MinReplicas:                &mdMinReplicas,
+			MaxReplicas:                &mdMaxReplicas,
+			ReadinessProbeInitialDelay: &mdReadinessDelay,
+			LivenessProbeInitialDelay:  &mdLivenessDelay,
+			Resources:                  mdResources,
+			ImagePullConnectionID:      &mdImagePullConnectionID,
+		},
+	}
+
+	cleanF := s.createDeployment(md)
+	defer cleanF()
+
+	cm := v1.ConfigMap{}
+
+	cmName := GetCMPolicyName(md)
+
+	predictor, ok := predictors.Predictors[md.Spec.Predictor]
+	s.Require().True(ok)
+	expectedCMData, err := deployment.ReadDefaultPoliciesAndRender(firstRole, predictor.OpaPolicyFilename)
+	s.Require().NoError(err)
+
+	s.Assertions.Eventually(func() bool {
+		if err := s.k8sClient.Get(
+			context.TODO(), types.NamespacedName{Name: cmName, Namespace: mdNamespace}, &cm); err != nil {
+			return false
+		}
+		return assert.ObjectsAreEqual(expectedCMData, cm.Data)
+	}, 10*time.Second, time.Second)
+
+	// Let's update deployment by changing roleName. We should expect new ConfigMap
+	secondRole := "second-role"
+	md.Spec.RoleName = &secondRole
+	err = s.k8sClient.Update(context.TODO(), md)
+	s.Require().NoError(err)
+
+	expectedCMData, err = deployment.ReadDefaultPoliciesAndRender(secondRole, predictor.OpaPolicyFilename)
+	s.Require().NoError(err)
+
+	s.Assertions.Eventually(func() bool {
+		if err := s.k8sClient.Get(
+			context.TODO(), types.NamespacedName{Name: cmName, Namespace: mdNamespace}, &cm); err != nil {
+			return false
+		}
+		return assert.ObjectsAreEqual(expectedCMData, cm.Data)
+	}, 10*time.Second, time.Second, "ConfigMap should be changed after roleName modification")
+
+
+}
+
 // Test utilities
 
 func (s *ModelDeploymentControllerSuite) createDeployment(md *odahuflowv1alpha1.ModelDeployment) (