address code review comments

Signed-off-by: zhujian <jiazhu@redhat.com>
open-cluster-management-io · Dec 22, 2021 · 4ca76bb · 4ca76bb
1 parent fe286ee
commit 4ca76bb
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 18 deletions.
diff --git a/manifests/klusterlet/management/klusterlet-registration-deployment.yaml b/manifests/klusterlet/management/klusterlet-registration-deployment.yaml
@@ -51,7 +51,7 @@ spec:
           {{if .ExternalServerURL}}
           - "--spoke-external-server-urls={{ .ExternalServerURL }}"
           {{end}}
-          {{if .DetachedMode}}
+          {{if eq .InstallMode "Detached"}}
           - "--spoke-kubeconfig=/spoke/config/kubeconfig"
           {{end}}
         securityContext:
@@ -67,7 +67,7 @@ spec:
           readOnly: true
         - name: hub-kubeconfig
           mountPath: "/spoke/hub-kubeconfig"
-        {{if .DetachedMode}}
+        {{if eq .InstallMode "Detached"}}
         - name: spoke-kubeconfig-secret
           mountPath: "/spoke/config"
           readOnly: true
@@ -96,7 +96,7 @@ spec:
       - name: hub-kubeconfig
         emptyDir:
           medium: Memory
-      {{if .DetachedMode}}
+      {{if eq .InstallMode "Detached"}}
       - name: spoke-kubeconfig-secret
         secret:
           secretName: {{ .ExternalManagedKubeConfigRegistrationSecret }}

diff --git a/manifests/klusterlet/management/klusterlet-work-deployment.yaml b/manifests/klusterlet/management/klusterlet-work-deployment.yaml
@@ -47,7 +47,7 @@ spec:
           - "agent"
           - "--spoke-cluster-name={{ .ClusterName }}"
           - "--hub-kubeconfig=/spoke/hub-kubeconfig/kubeconfig"
-          {{if .DetachedMode}}
+          {{if eq .InstallMode "Detached"}}
           - "--spoke-kubeconfig=/spoke/config/kubeconfig"
           {{end}}
         securityContext:
@@ -61,7 +61,7 @@ spec:
         - name: hub-kubeconfig-secret
           mountPath: "/spoke/hub-kubeconfig"
           readOnly: true
-        {{if .DetachedMode}}
+        {{if eq .InstallMode "Detached"}}
         - name: spoke-kubeconfig-secret
           mountPath: "/spoke/config"
           readOnly: true
@@ -87,7 +87,7 @@ spec:
       - name: hub-kubeconfig-secret
         secret:
           secretName: {{ .HubKubeConfigSecret }}
-      {{if .DetachedMode}}
+      {{if eq .InstallMode "Detached"}}
       - name: spoke-kubeconfig-secret
         secret:
           secretName: {{ .ExternalManagedKubeConfigWorkSecret }}

diff --git a/pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go b/pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
@@ -42,6 +42,7 @@ const (
 	klusterletFinalizer          = "operator.open-cluster-management.io/klusterlet-cleanup"
 	imagePullSecret              = "open-cluster-management-image-pull-credentials"
 	klusterletApplied            = "Applied"
+	klusterletReadyToApply       = "ReadyToApply"
 	appliedManifestWorkFinalizer = "cluster.open-cluster-management.io/applied-manifest-work-cleanup"
 	defaultReplica               = 3
 	singleReplica                = 1
@@ -147,7 +148,7 @@ type klusterletConfig struct {
 	ExternalManagedKubeConfigSecret             string
 	ExternalManagedKubeConfigRegistrationSecret string
 	ExternalManagedKubeConfigWorkSecret         string
-	DetachedMode                                bool
+	InstallMode                                 operatorapiv1.InstallMode
 }
 
 // managedClusterClients holds variety of kube client for managed cluster
@@ -173,7 +174,6 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 	}
 	klusterlet = klusterlet.DeepCopy()
 
-	detachedMode := klusterlet.Spec.DeployOption.Mode == operatorapiv1.InstallModeDetached
 	config := klusterletConfig{
 		KlusterletName:            klusterlet.Name,
 		KlusterletNamespace:       helpers.KlusterletNamespace(klusterlet.Spec.DeployOption.Mode, klusterletName, klusterlet.Spec.Namespace),
@@ -189,7 +189,7 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 		ExternalManagedKubeConfigSecret:             helpers.ExternalManagedKubeConfig,
 		ExternalManagedKubeConfigRegistrationSecret: helpers.ExternalManagedKubeConfigRegistration,
 		ExternalManagedKubeConfigWorkSecret:         helpers.ExternalManagedKubeConfigWork,
-		DetachedMode:                                detachedMode,
+		InstallMode:                                 klusterlet.Spec.DeployOption.Mode,
 	}
 
 	managedClusterClients := &managedClusterClients{
@@ -198,10 +198,19 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 		appliedManifestWorkClient: n.appliedManifestWorkClient,
 	}
 
-	if detachedMode {
+	if config.InstallMode == operatorapiv1.InstallModeDetached {
 		managedClusterClients, err = n.buildManagedClusterClientsDetachedMode(n.kubeClient, config.KlusterletNamespace, config.ExternalManagedKubeConfigSecret)
 		if err != nil {
+			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.UpdateKlusterletConditionFn(metav1.Condition{
+				Type: klusterletReadyToApply, Status: metav1.ConditionFalse, Reason: "KlusterletPrepareFailed",
+				Message: fmt.Sprintf("Failed to build managed cluster clients: %v", err),
+			}))
 			return err
+		} else {
+			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.UpdateKlusterletConditionFn(metav1.Condition{
+				Type: klusterletReadyToApply, Status: metav1.ConditionTrue, Reason: "KlusterletPrepared",
+				Message: "Klusterlet is ready to apply",
+			}))
 		}
 	}
 
@@ -245,7 +254,7 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 		return err
 	}
 
-	if detachedMode {
+	if config.InstallMode == operatorapiv1.InstallModeDetached {
 		// In detached mode, we should ensure the namespace on the managed cluster since
 		// some resources(eg:service account) are still deployed on managed cluster.
 		err := n.ensureNamespace(ctx, managedClusterClients.kubeClient, klusterletName, config.KlusterletNamespace)
@@ -296,7 +305,7 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 	}
 	relatedResources = append(relatedResources, statuses...)
 
-	if detachedMode {
+	if config.InstallMode == operatorapiv1.InstallModeDetached {
 		// create managed config secret for registration and work.
 		err = n.createManagedClusterKubeconfig(ctx, klusterletName, config.KlusterletNamespace, registrationServiceAccountName(klusterletName), config.ExternalManagedKubeConfigRegistrationSecret,
 			managedClusterClients.kubeconfig, managedClusterClients.kubeClient, n.kubeClient.CoreV1(), controllerContext.Recorder())
@@ -580,7 +589,7 @@ func (n *klusterletController) cleanUp(
 
 	// Remove secrets
 	secrets := []string{config.HubKubeConfigSecret}
-	if config.DetachedMode {
+	if config.InstallMode == operatorapiv1.InstallModeDetached {
 		// In Detached mod, also need to remove the external-managed-kubeconfig-registration and external-managed-kubeconfig-work
 		secrets = append(secrets, []string{config.ExternalManagedKubeConfigRegistrationSecret, config.ExternalManagedKubeConfigWorkSecret}...)
 	}
@@ -626,7 +635,7 @@ func (n *klusterletController) cleanUp(
 			return err
 		}
 	}
-	if config.DetachedMode {
+	if config.InstallMode == operatorapiv1.InstallModeDetached {
 		// remove the klusterlet namespace on the management cluster
 		err = n.kubeClient.CoreV1().Namespaces().Delete(ctx, config.KlusterletNamespace, metav1.DeleteOptions{})
 		if err != nil && !errors.IsNotFound(err) {

diff --git a/pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go b/pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go
@@ -522,15 +522,25 @@ func TestSyncDeployDetached(t *testing.T) {
 	}
 
 	operatorAction := controller.operatorClient.Actions()
-	if len(operatorAction) != 2 {
-		t.Errorf("Expect 2 actions in the sync loop, actual %#v", operatorAction)
+	for _, action := range operatorAction {
+		klog.Infof("operator actions, verb:%v \t resource:%v \t namespace:%v", action.GetVerb(), action.GetResource(), action.GetNamespace())
+	}
+
+	if len(operatorAction) != 4 {
+		t.Errorf("Expect 4 actions in the sync loop, actual %#v", len(operatorAction))
 	}
 
 	testinghelper.AssertGet(t, operatorAction[0], "operator.open-cluster-management.io", "v1", "klusterlets")
 	testinghelper.AssertAction(t, operatorAction[1], "update")
+	testinghelper.AssertGet(t, operatorAction[2], "operator.open-cluster-management.io", "v1", "klusterlets")
+	testinghelper.AssertAction(t, operatorAction[3], "update")
+
+	conditionReady := testinghelper.NamedCondition(klusterletReadyToApply, "KlusterletPrepared", metav1.ConditionTrue)
+	conditionApplied := testinghelper.NamedCondition(klusterletApplied, "KlusterletApplied", metav1.ConditionTrue)
 	testinghelper.AssertOnlyConditions(
-		t, operatorAction[1].(clienttesting.UpdateActionImpl).Object,
-		testinghelper.NamedCondition(klusterletApplied, "KlusterletApplied", metav1.ConditionTrue))
+		t, operatorAction[1].(clienttesting.UpdateActionImpl).Object, conditionReady)
+	testinghelper.AssertOnlyConditions(
+		t, operatorAction[3].(clienttesting.UpdateActionImpl).Object, conditionReady, conditionApplied)
 }
 
 // TestSyncDelete test cleanup hub deploy