Resolve comments

Signed-off-by: Jian Qiu <jqiu@redhat.com>

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/client_builder.go

@@ -8,7 +8,10 @@ import (
 	"context"
 
 	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
+	corev1 "k8s.io/api/core/v1"
 	apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 
@@ -83,7 +86,15 @@ func (m *managedClusterClientsBuilder) build(ctx context.Context) (*managedClust
 	// Ensure the agent namespace for users to create the external-managed-kubeconfig secret in this
 	// namespace, so that in the next reconcile loop the controller can get the secret successfully after
 	// the secret was created.
-	if err := ensureAgentNamespace(ctx, m.kubeClient, m.secretNamespace, map[string]string{}, m.recorder); err != nil {
+	_, _, err := resourceapply.ApplyNamespace(ctx, m.kubeClient.CoreV1(), m.recorder, &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: m.secretNamespace,
+			Annotations: map[string]string{
+				"workload.openshift.io/allowed": "management",
+			},
+		},
+	})
+	if err != nil {
 		return nil, err
 	}
 

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go

@@ -373,20 +373,6 @@ func getManagedKubeConfig(ctx context.Context, kubeClient kubernetes.Interface,
 	return helpers.LoadClientConfigFromSecret(managedKubeconfigSecret)
 }
 
-// ensureAgentNamespace create agent namespace if it is not exist
-func ensureAgentNamespace(ctx context.Context, kubeClient kubernetes.Interface, namespace string, labels map[string]string, recorder events.Recorder) error {
-	_, _, err := resourceapply.ApplyNamespace(ctx, kubeClient.CoreV1(), recorder, &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: namespace,
-			Annotations: map[string]string{
-				"workload.openshift.io/allowed": "management",
-			},
-			Labels: labels,
-		},
-	})
-	return err
-}
-
 // syncPullSecret will sync pull secret from the sourceClient cluster to the targetClient cluster in desired namespace.
 func syncPullSecret(ctx context.Context, sourceClient, targetClient kubernetes.Interface,
 	klusterlet *operatorapiv1.Klusterlet, operatorNamespace, namespace string, recorder events.Recorder) error {
@@ -411,17 +397,28 @@ func syncPullSecret(ctx context.Context, sourceClient, targetClient kubernetes.I
 	return nil
 }
 
-// ensureKlusterletNamespace is to apply the namespace defined in klusterlet spec to the managed cluster
-func ensureKlusterletNamespace(ctx context.Context, kubeClient kubernetes.Interface, klusterlet *operatorapiv1.Klusterlet,
-	namespace string, recorder events.Recorder) error {
-	if err := ensureAgentNamespace(ctx, kubeClient, namespace, map[string]string{
-		klusterletNamespaceLabelKey: klusterlet.Name,
-	}, recorder); err != nil {
+// ensureNamespace is to apply the namespace defined in klusterlet spec to the managed cluster. The namespace
+// will have a klusterlet label.
+func ensureNamespace(
+	ctx context.Context,
+	kubeClient kubernetes.Interface,
+	klusterlet *operatorapiv1.Klusterlet,
+	namespace string, labels map[string]string, recorder events.Recorder) error {
+	_, _, err := resourceapply.ApplyNamespace(ctx, kubeClient.CoreV1(), recorder, &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: namespace,
+			Annotations: map[string]string{
+				"workload.openshift.io/allowed": "management",
+			},
+			Labels: labels,
+		},
+	})
+	if err != nil {
 		meta.SetStatusCondition(&klusterlet.Status.Conditions, metav1.Condition{
 			Type:    operatorapiv1.ConditionKlusterletApplied,
 			Status:  metav1.ConditionFalse,
 			Reason:  operatorapiv1.ReasonKlusterletApplyFailed,
-			Message: fmt.Sprintf("Failed to ensure namespace %q: %v", namespace, err)})
+			Message: fmt.Sprintf("Failed to ensure namespace of klusterlet %q: %v", namespace, err)})
 		return err
 	}
 	return nil

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_managed_reconcile.go

@@ -68,7 +68,7 @@ func (r *managedReconcile) reconcile(ctx context.Context, klusterlet *operatorap
 	// TODO(zhujian7): In the future, we may consider deploy addons on the management cluster in Hosted mode.
 	addonNamespace := fmt.Sprintf("%s-addon", config.KlusterletNamespace)
 	// Ensure the addon namespace on the managed cluster
-	err := ensureKlusterletNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, addonNamespace, r.recorder)
+	err := ensureNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, addonNamespace, nil, r.recorder)
 	if err != nil {
 		return klusterlet, reconcileStop, err
 	}
@@ -82,7 +82,10 @@ func (r *managedReconcile) reconcile(ctx context.Context, klusterlet *operatorap
 		return klusterlet, reconcileStop, err
 	}
 
-	err = ensureKlusterletNamespace(ctx, r.managedClusterClients.kubeClient, klusterlet, config.KlusterletNamespace, r.recorder)
+	err = ensureNamespace(
+		ctx, r.managedClusterClients.kubeClient, klusterlet, config.KlusterletNamespace, map[string]string{
+			klusterletNamespaceLabelKey: klusterlet.Name,
+		}, r.recorder)
 	if err != nil {
 		return klusterlet, reconcileStop, err
 	}

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_management_recocile.go

@@ -48,7 +48,7 @@ type managementReconcile struct {
 
 func (r *managementReconcile) reconcile(ctx context.Context, klusterlet *operatorapiv1.Klusterlet,
 	config klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) {
-	err := ensureAgentNamespace(ctx, r.kubeClient, config.AgentNamespace, map[string]string{}, r.recorder)
+	err := ensureNamespace(ctx, r.kubeClient, klusterlet, config.AgentNamespace, nil, r.recorder)
 	if err != nil {
 		return klusterlet, reconcileStop, err
 	}

pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_namespace_reconcile.go

@@ -23,10 +23,11 @@ func (r *namespaceReconcile) reconcile(
 	ctx context.Context,
 	klusterlet *operatorapiv1.Klusterlet,
 	config klusterletConfig) (*operatorapiv1.Klusterlet, reconcileState, error) {
-	if !meta.IsStatusConditionTrue(klusterlet.Status.Conditions, operatorapiv1.ConditionKlusterletApplied) {
+	cond := meta.FindStatusCondition(klusterlet.Status.Conditions, operatorapiv1.ConditionKlusterletApplied)
+	if cond == nil || cond.Status == metav1.ConditionFalse || klusterlet.Generation != klusterlet.Status.ObservedGeneration {
 		return klusterlet, reconcileContinue, nil
 	}
-	// filters namespace for klusterlet and not in hosted mo
+	// filters namespace for klusterlet and not in hosted mode
 	namespaces, err := r.managedClusterClients.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{
 		LabelSelector: fmt.Sprintf(
 			"%s=%s",

test/e2e/klusterlet_test.go

@@ -201,10 +201,13 @@ var _ = Describe("Create klusterlet CR", func() {
 
 		By("old namespace should be removed")
 		Eventually(func() error {
-			_, err = t.SpokeKubeClient.CoreV1().Namespaces().Get(context.TODO(), klusterletNamespace, metav1.GetOptions{})
+			ns, err := t.SpokeKubeClient.CoreV1().Namespaces().Get(context.TODO(), klusterletNamespace, metav1.GetOptions{})
 			if errors.IsNotFound(err) {
 				return nil
 			}
+			if err == nil {
+				By(fmt.Sprintf("ns is %s, %v", ns.Name, ns.Labels))
+			}
 			return fmt.Errorf("namespace still exists")
 		}, t.EventuallyTimeout*5, t.EventuallyInterval*5).Should(Succeed())