Fix 1867783 Charm deployment hangs with 2.8

Fixes stateful set creation to no longer immediately update the newly created statefulset. Fixes OperatorExists method to check for all created resources. Fixes config map RAW/WAW race condition. Adds tests for OperatorExists which was missing before.
sdg0919 · Mar 24, 2020 · e329c7b · e329c7b
1 parent 47c9cf5
commit e329c7b
Show file tree

Hide file tree

Showing 12 changed files with 301 additions and 75 deletions.
diff --git a/api/caasoperatorprovisioner/client.go b/api/caasoperatorprovisioner/client.go
@@ -146,8 +146,11 @@ type OperatorCertificate struct {
 
 // IssueOperatorCertificate issues an x509 certificate for use by the specified application operator.
 func (c *Client) IssueOperatorCertificate(applicationName string) (OperatorCertificate, error) {
+	if !names.IsValidApplication(applicationName) {
+		return OperatorCertificate{}, errors.NotValidf("application name %q", applicationName)
+	}
 	args := params.Entities{[]params.Entity{
-		{Tag: applicationName},
+		{Tag: names.NewApplicationTag(applicationName).String()},
 	}}
 	var result params.IssueOperatorCertificateResults
 	if err := c.facade.FacadeCall("IssueOperatorCertificate", args, &result); err != nil {

diff --git a/api/caasoperatorprovisioner/client_test.go b/api/caasoperatorprovisioner/client_test.go
@@ -200,7 +200,7 @@ func (s *provisionerSuite) TestIssueOperatorCertificate(c *gc.C) {
 		c.Check(objType, gc.Equals, "CAASOperatorProvisioner")
 		c.Check(id, gc.Equals, "")
 		c.Assert(request, gc.Equals, "IssueOperatorCertificate")
-		c.Assert(a, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "appymcappface"}}})
+		c.Assert(a, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "application-appymcappface"}}})
 		c.Assert(result, gc.FitsTypeOf, &params.IssueOperatorCertificateResults{})
 		*(result.(*params.IssueOperatorCertificateResults)) = params.IssueOperatorCertificateResults{
 			Results: []params.IssueOperatorCertificateResult{{
@@ -225,7 +225,7 @@ func (s *provisionerSuite) TestIssueOperatorCertificateArity(c *gc.C) {
 		c.Check(objType, gc.Equals, "CAASOperatorProvisioner")
 		c.Check(id, gc.Equals, "")
 		c.Assert(request, gc.Equals, "IssueOperatorCertificate")
-		c.Assert(a, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "appymcappface"}}})
+		c.Assert(a, jc.DeepEquals, params.Entities{Entities: []params.Entity{{Tag: "application-appymcappface"}}})
 		c.Assert(result, gc.FitsTypeOf, &params.IssueOperatorCertificateResults{})
 		return nil
 	})

diff --git a/caas/broker.go b/caas/broker.go
@@ -383,4 +383,9 @@ type OperatorConfig struct {
 
 	// ResourceTags is a set of tags to set on the operator pod.
 	ResourceTags map[string]string
+
+	// ConfigMapGeneration is set when updating the operator config
+	// map for consistency in Read after Write and Write after Write.
+	// A value of 0 is ignored.
+	ConfigMapGeneration int64
 }
diff --git a/caas/kubernetes/provider/admissionregistration_test.go b/caas/kubernetes/provider/admissionregistration_test.go
@@ -101,7 +101,7 @@ func (s *K8sBrokerSuite) assertMutatingWebhookConfigurations(c *gc.C, cfgs map[s
 		s.mockStatefulSets.EXPECT().Get("app-name", metav1.GetOptions{}).
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
-			Return(nil, nil),
+			Return(nil, s.k8sAlreadyExistsError()),
 		s.mockStatefulSets.EXPECT().Get("app-name", metav1.GetOptions{}).
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
@@ -325,10 +325,6 @@ func (s *K8sBrokerSuite) assertValidatingWebhookConfigurations(c *gc.C, cfgs map
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", metav1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	}...)
 	gomock.InOrder(assertCalls...)
 

diff --git a/caas/kubernetes/provider/customresourcedefinitions_test.go b/caas/kubernetes/provider/customresourcedefinitions_test.go
@@ -102,10 +102,6 @@ func (s *K8sBrokerSuite) assertCustomerResourceDefinitions(c *gc.C, crds []k8ssp
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	}...)
 	gomock.InOrder(assertCalls...)
 
@@ -428,10 +424,6 @@ func (s *K8sBrokerSuite) assertCustomerResources(c *gc.C, crs map[string][]unstr
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	}...)
 	gomock.InOrder(assertCalls...)
 

diff --git a/caas/kubernetes/provider/ingress_test.go b/caas/kubernetes/provider/ingress_test.go
@@ -98,10 +98,6 @@ func (s *K8sBrokerSuite) assertIngressResources(c *gc.C, IngressResources []k8ss
 				Return(statefulSetArg, nil),
 			s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 				Return(nil, nil),
-			s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-				Return(statefulSetArg, nil),
-			s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-				Return(nil, nil),
 		}...)
 	}
 	gomock.InOrder(assertCalls...)

diff --git a/caas/kubernetes/provider/k8s_test.go b/caas/kubernetes/provider/k8s_test.go
@@ -2814,10 +2814,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceNoStorageStateful(c *gc.C) {
 			Return(nil, s.k8sNotFoundError()),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -2902,10 +2898,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceCustomType(c *gc.C) {
 			Return(statefulSetArg, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -4555,10 +4547,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceWithStorage(c *gc.C) {
 			Return(&storagev1.StorageClass{ObjectMeta: v1.ObjectMeta{Name: "workload-storage"}}, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -4957,10 +4945,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceForStatefulSetWithDevices(c *gc.C) {
 			Return(&storagev1.StorageClass{ObjectMeta: v1.ObjectMeta{Name: "workload-storage"}}, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(statefulSetArg, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -5042,10 +5026,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceWithConstraints(c *gc.C) {
 			Return(&storagev1.StorageClass{ObjectMeta: v1.ObjectMeta{Name: "workload-storage"}}, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -5134,10 +5114,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceWithNodeAffinity(c *gc.C) {
 			Return(&storagev1.StorageClass{ObjectMeta: v1.ObjectMeta{Name: "workload-storage"}}, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{
@@ -5218,10 +5194,6 @@ func (s *K8sBrokerSuite) TestEnsureServiceWithZones(c *gc.C) {
 			Return(&storagev1.StorageClass{ObjectMeta: v1.ObjectMeta{Name: "workload-storage"}}, nil),
 		s.mockStatefulSets.EXPECT().Create(statefulSetArg).
 			Return(nil, nil),
-		s.mockStatefulSets.EXPECT().Get("app-name", v1.GetOptions{}).
-			Return(statefulSetArg, nil),
-		s.mockStatefulSets.EXPECT().Update(statefulSetArg).
-			Return(nil, nil),
 	)
 
 	params := &caas.ServiceParams{

diff --git a/caas/kubernetes/provider/operator.go b/caas/kubernetes/provider/operator.go
@@ -295,19 +295,159 @@ func (k *kubernetesClient) validateOperatorStorage() (string, error) {
 // OperatorExists indicates if the operator for the specified
 // application exists, and whether the operator is terminating.
 func (k *kubernetesClient) OperatorExists(appName string) (caas.OperatorState, error) {
-	var result caas.OperatorState
 	operatorName := k.operatorName(appName)
+	exists, terminating, err := k.operatorStatefulSetExists(appName, operatorName)
+	if err != nil {
+		return caas.OperatorState{}, errors.Trace(err)
+	}
+	if exists || terminating {
+		if terminating {
+			logger.Tracef("operator %q exists and is terminating")
+		} else {
+			logger.Tracef("operator %q exists")
+		}
+		return caas.OperatorState{Exists: exists, Terminating: terminating}, nil
+	}
+	checks := []struct {
+		label string
+		check func(appName string, operatorName string) (bool, error)
+	}{
+		{"rbac", k.operatorRBACResourcesRemaining},
+		{"config map", k.operatorConfigMapExists},
+		{"configurations config map", k.operatorConfigurationsConfigMapExists},
+		{"service", k.operatorServiceExists},
+		{"secret", k.operatorSecretExists},
+		{"deployment", k.operatorDeploymentExists},
+		{"pods", k.operatorPodExists},
+	}
+	for _, c := range checks {
+		exists, err := c.check(appName, operatorName)
+		if err != nil {
+			return caas.OperatorState{}, errors.Annotatef(err, "%s resource check", c.label)
+		}
+		if exists {
+			logger.Debugf("operator %q exists and is terminating due to dangling %s resource(s)", c.label)
+			return caas.OperatorState{Exists: true, Terminating: true}, nil
+		}
+	}
+	return caas.OperatorState{}, nil
+}
+
+func (k *kubernetesClient) operatorStatefulSetExists(appName string, operatorName string) (exists bool, terminating bool, err error) {
 	statefulSets := k.client().AppsV1().StatefulSets(k.namespace)
 	operator, err := statefulSets.Get(operatorName, v1.GetOptions{})
 	if k8serrors.IsNotFound(err) {
-		return result, nil
+		return false, false, nil
+	}
+	if err != nil {
+		return false, false, errors.Trace(err)
+	}
+	return true, operator.DeletionTimestamp != nil, nil
+}
+
+func (k *kubernetesClient) operatorRBACResourcesRemaining(appName string, operatorName string) (bool, error) {
+	_, err := k.getServiceAccount(operatorName)
+	if errors.IsNotFound(err) {
+		// continue
+	} else if err != nil {
+		return false, errors.Trace(err)
+	} else {
+		return true, nil
+	}
+	_, err = k.getRole(operatorName)
+	if errors.IsNotFound(err) {
+		// continue
+	} else if err != nil {
+		return false, errors.Trace(err)
+	} else {
+		return true, nil
+	}
+	_, err = k.getRoleBinding(operatorName)
+	if errors.IsNotFound(err) {
+		// continue
+	} else if err != nil {
+		return false, errors.Trace(err)
+	} else {
+		return true, nil
+	}
+	return false, nil
+}
+
+func (k *kubernetesClient) operatorConfigMapExists(appName string, operatorName string) (bool, error) {
+	configMaps := k.client().CoreV1().ConfigMaps(k.namespace)
+	configMapName := operatorConfigMapName(operatorName)
+	_, err := configMaps.Get(configMapName, v1.GetOptions{})
+	if k8serrors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, errors.Trace(err)
+	}
+	return true, nil
+}
+
+func (k *kubernetesClient) operatorConfigurationsConfigMapExists(appName string, operatorName string) (bool, error) {
+	legacy := isLegacyName(operatorName)
+	configMaps := k.client().CoreV1().ConfigMaps(k.namespace)
+	configMapName := appName + "-configurations-config"
+	if legacy {
+		configMapName = "juju-" + configMapName
+	}
+	_, err := configMaps.Get(configMapName, v1.GetOptions{})
+	if k8serrors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, errors.Trace(err)
+	}
+	return true, nil
+}
+
+func (k *kubernetesClient) operatorServiceExists(appName string, operatorName string) (bool, error) {
+	services := k.client().CoreV1().Services(k.namespace)
+	_, err := services.Get(operatorName, v1.GetOptions{})
+	if k8serrors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, errors.Trace(err)
+	}
+	return true, nil
+}
+
+func (k *kubernetesClient) operatorSecretExists(appName string, operatorName string) (bool, error) {
+	legacy := isLegacyName(operatorName)
+	deploymentName := appName
+	if legacy {
+		deploymentName = "juju-" + appName
+	}
+	secretName := appSecretName(deploymentName, operatorContainerName)
+	_, err := k.getSecret(secretName)
+	if errors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, errors.Trace(err)
+	}
+	return true, nil
+}
+
+func (k *kubernetesClient) operatorDeploymentExists(appName string, operatorName string) (bool, error) {
+	deployments := k.client().AppsV1().Deployments(k.namespace)
+	_, err := deployments.Get(operatorName, v1.GetOptions{})
+	if k8serrors.IsNotFound(err) {
+		return false, nil
+	} else if err != nil {
+		return false, errors.Trace(err)
 	}
+	return true, nil
+}
+
+func (k *kubernetesClient) operatorPodExists(appName string, operatorName string) (bool, error) {
+	pods := k.client().CoreV1().Pods(k.namespace)
+	podList, err := pods.List(v1.ListOptions{
+		LabelSelector: operatorSelector(appName),
+	})
 	if err != nil {
-		return result, errors.Trace(err)
+		return false, errors.Trace(err)
 	}
-	result.Exists = true
-	result.Terminating = operator.DeletionTimestamp != nil
-	return result, nil
+	return len(podList.Items) != 0, nil
 }
 
 // DeleteOperator deletes the specified operator.
@@ -454,6 +594,7 @@ func (k *kubernetesClient) Operator(appName string) (*caas.Operator, error) {
 		return nil, errors.Trace(err)
 	}
 	if configMap != nil {
+		cfg.ConfigMapGeneration = configMap.Generation
 		if agentConf, ok := configMap.Data[operatorConfigMapAgentConfKey(appName)]; ok {
 			cfg.AgentConf = []byte(agentConf)
 		}
@@ -590,6 +731,7 @@ func operatorConfigMap(appName, name string, labels, annotations map[string]stri
 			Name:        name,
 			Labels:      labels,
 			Annotations: annotations,
+			Generation:  config.ConfigMapGeneration,
 		},
 		Data: map[string]string{
 			operatorConfigMapAgentConfKey(appName): string(config.AgentConf),