feat: refine the orchestration code (Azure#237)

* feat: refine the orchestration code * fixing lint and format * address the comments and integrate new work-api version * fix gh action naming and mc update bug * fix e2e test * address comments and fix some bugs Co-authored-by: Ryan Zhang <[email protected]>
michaelawyu · Aug 22, 2022 · 8b41895 · 8b41895
1 parent 2933825
commit 8b41895
Show file tree

Hide file tree

Showing 33 changed files with 1,715 additions and 748 deletions.
diff --git a/.github/workflows/workflow.yml → .github/workflows/ci.yml b/.github/workflows/workflow.yml → .github/workflows/ci.yml
@@ -1,4 +1,4 @@
-name: fleet-ci
+name: unit and e2e test
 
 on:
   push:

diff --git a/.github/workflows/go.yml → .github/workflows/code-lint.yml b/.github/workflows/go.yml → .github/workflows/code-lint.yml
@@ -1,4 +1,4 @@
-name: Go
+name: code lint
 
 on:
   push:

diff --git a/.github/workflows/commit-lint.yml → .github/workflows/pr-title-lint.yml b/.github/workflows/commit-lint.yml → .github/workflows/pr-title-lint.yml
diff --git a/Makefile b/Makefile
@@ -26,7 +26,7 @@ CLUSTER_CONFIG := $(abspath test/e2e/kind-config.yaml)
 # Binaries
 # Note: Need to use abspath so we can invoke these from subdirectories
 
-CONTROLLER_GEN_VER := v0.7.0
+CONTROLLER_GEN_VER := v0.8.0
 CONTROLLER_GEN_BIN := controller-gen
 CONTROLLER_GEN := $(abspath $(TOOLS_BIN_DIR)/$(CONTROLLER_GEN_BIN)-$(CONTROLLER_GEN_VER))
 

diff --git a/apis/v1alpha1/membercluster_types.go b/apis/v1alpha1/membercluster_types.go
@@ -72,7 +72,7 @@ const (
 	// its conditionStatus can be "True" == Joined, "Unknown" == Joining/Leaving, "False" == Left
 	ConditionTypeMemberClusterJoin string = "Joined"
 
-	// ConditionTypeMemberClusterHealthy is used to track the Health state of the MemberCluster.
+	// ConditionTypeMemberClusterHealth is used to track the Health state of the MemberCluster.
 	// its conditionStatus can be "True" == Healthy, "Unknown" == Health degraded, "False" == UnHealthy
 	ConditionTypeMemberClusterHealth string = "Healthy"
 )

diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go
@@ -207,8 +207,7 @@ func Start(ctx context.Context, hubCfg *rest.Config, hubOpts, memberOpts ctrl.Op
 		memberMgr.GetClient(),
 		restMapper,
 		hubMgr.GetEventRecorderFor("work_status_controller"),
-		3,
-	).SetupWithManager(hubMgr); err != nil {
+		1, true).SetupWithManager(hubMgr); err != nil {
 		klog.ErrorS(err, "unable to create controller", "controller", "WorkStatus")
 		return err
 	}
@@ -219,21 +218,11 @@ func Start(ctx context.Context, hubCfg *rest.Config, hubOpts, memberOpts ctrl.Op
 		memberMgr.GetClient(),
 		restMapper,
 		hubMgr.GetEventRecorderFor("work_controller"),
-		3,
-	).SetupWithManager(hubMgr); err != nil {
+		5, true).SetupWithManager(hubMgr); err != nil {
 		klog.ErrorS(err, "unable to create controller", "controller", "Work")
 		return err
 	}
 
-	if err = workcontrollers.NewFinalizeWorkReconciler(
-		hubMgr.GetClient(),
-		memberMgr.GetClient(),
-		hubMgr.GetEventRecorderFor("WorkFinalizer_controller"),
-	).SetupWithManager(hubMgr); err != nil {
-		klog.ErrorS(err, "unable to create controller", "controller", "WorkFinalize")
-		return err
-	}
-
 	if err = internalmembercluster.NewReconciler(hubMgr.GetClient(), memberMgr.GetClient()).SetupWithManager(hubMgr); err != nil {
 		return errors.Wrap(err, "unable to create controller hub_member")
 	}

diff --git a/config/crd/bases/fleet.azure.com_clusterresourceplacements.yaml b/config/crd/bases/fleet.azure.com_clusterresourceplacements.yaml
@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   name: clusterresourceplacements.fleet.azure.com
 spec:
@@ -223,13 +222,12 @@ spec:
                   description: "Condition contains details for one aspect of the current
                     state of this API Resource. --- This struct is intended for direct
                     use as an array at the field path .status.conditions.  For example,
-                    type FooStatus struct{     // Represents the observations of a
-                    foo's current state.     // Known .status.conditions.type are:
-                    \"Available\", \"Progressing\", and \"Degraded\"     // +patchMergeKey=type
-                    \    // +patchStrategy=merge     // +listType=map     // +listMapKey=type
-                    \    Conditions []metav1.Condition `json:\"conditions,omitempty\"
-                    patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`
-                    \n     // other fields }"
+                    type FooStatus struct{ // Represents the observations of a foo's
+                    current state. // Known .status.conditions.type are: \"Available\",
+                    \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                    // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                    `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                    protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }"
                   properties:
                     lastTransitionTime:
                       description: lastTransitionTime is the last time the condition

diff --git a/config/crd/bases/fleet.azure.com_internalmemberclusters.yaml b/config/crd/bases/fleet.azure.com_internalmemberclusters.yaml
@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   name: internalmemberclusters.fleet.azure.com
 spec:
@@ -72,13 +71,13 @@ spec:
                           the current state of this API Resource. --- This struct
                           is intended for direct use as an array at the field path
                           .status.conditions.  For example, type FooStatus struct{
-                          \    // Represents the observations of a foo's current state.
-                          \    // Known .status.conditions.type are: \"Available\",
-                          \"Progressing\", and \"Degraded\"     // +patchMergeKey=type
-                          \    // +patchStrategy=merge     // +listType=map     //
-                          +listMapKey=type     Conditions []metav1.Condition `json:\"conditions,omitempty\"
-                          patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`
-                          \n     // other fields }"
+                          // Represents the observations of a foo's current state.
+                          // Known .status.conditions.type are: \"Available\", \"Progressing\",
+                          and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                          // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                          `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                          protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields
+                          }"
                         properties:
                           lastTransitionTime:
                             description: lastTransitionTime is the last time the condition

diff --git a/config/crd/bases/fleet.azure.com_memberclusters.yaml b/config/crd/bases/fleet.azure.com_memberclusters.yaml
@@ -1,10 +1,9 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.7.0
+    controller-gen.kubebuilder.io/version: v0.8.0
   creationTimestamp: null
   name: memberclusters.fleet.azure.com
 spec:
@@ -105,13 +104,13 @@ spec:
                           the current state of this API Resource. --- This struct
                           is intended for direct use as an array at the field path
                           .status.conditions.  For example, type FooStatus struct{
-                          \    // Represents the observations of a foo's current state.
-                          \    // Known .status.conditions.type are: \"Available\",
-                          \"Progressing\", and \"Degraded\"     // +patchMergeKey=type
-                          \    // +patchStrategy=merge     // +listType=map     //
-                          +listMapKey=type     Conditions []metav1.Condition `json:\"conditions,omitempty\"
-                          patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`
-                          \n     // other fields }"
+                          // Represents the observations of a foo's current state.
+                          // Known .status.conditions.type are: \"Available\", \"Progressing\",
+                          and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                          // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                          `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                          protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields
+                          }"
                         properties:
                           lastTransitionTime:
                             description: lastTransitionTime is the last time the condition
@@ -190,13 +189,12 @@ spec:
                   description: "Condition contains details for one aspect of the current
                     state of this API Resource. --- This struct is intended for direct
                     use as an array at the field path .status.conditions.  For example,
-                    type FooStatus struct{     // Represents the observations of a
-                    foo's current state.     // Known .status.conditions.type are:
-                    \"Available\", \"Progressing\", and \"Degraded\"     // +patchMergeKey=type
-                    \    // +patchStrategy=merge     // +listType=map     // +listMapKey=type
-                    \    Conditions []metav1.Condition `json:\"conditions,omitempty\"
-                    patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`
-                    \n     // other fields }"
+                    type FooStatus struct{ // Represents the observations of a foo's
+                    current state. // Known .status.conditions.type are: \"Available\",
+                    \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                    // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                    `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                    protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }"
                   properties:
                     lastTransitionTime:
                       description: lastTransitionTime is the last time the condition

diff --git a/go.mod b/go.mod
@@ -16,15 +16,13 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.7.0
 	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e
-	golang.org/x/net v0.0.0-20220225172249-27dd8689420f
 	golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
 	golang.org/x/time v0.0.0-20220210224613-90d013bbcef8
 	k8s.io/api v0.24.2
 	k8s.io/apiextensions-apiserver v0.24.2
 	k8s.io/apimachinery v0.24.2
 	k8s.io/client-go v0.24.2
 	k8s.io/component-base v0.24.2
-	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.70.0
 	k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9
 	sigs.k8s.io/controller-runtime v0.12.2
@@ -73,6 +71,7 @@ require (
 	go.uber.org/multierr v1.6.0 // indirect
 	go.uber.org/zap v1.19.1 // indirect
 	golang.org/x/crypto v0.0.0-20220214200702-86341886e292 // indirect
+	golang.org/x/net v0.0.0-20220225172249-27dd8689420f // indirect
 	golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8 // indirect
 	golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8 // indirect
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 // indirect
@@ -91,5 +90,5 @@ require (
 
 replace (
 	k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.24.2 // weird bug that the goland won't compile without this
-	sigs.k8s.io/work-api => github.com/Azure/k8s-work-api v0.2.0
+	sigs.k8s.io/work-api => github.com/Azure/k8s-work-api v0.3.0
 )
diff --git a/go.sum b/go.sum
@@ -52,8 +52,8 @@ github.com/Azure/go-autorest/autorest/date v0.3.0/go.mod h1:BI0uouVdmngYNUzGWeSY
 github.com/Azure/go-autorest/autorest/mocks v0.4.1/go.mod h1:LTp+uSrOhSkaKrUy935gNZuuIPPVsHlr9DSOxSayd+k=
 github.com/Azure/go-autorest/logger v0.2.1/go.mod h1:T9E3cAhj2VqvPOtCYAvby9aBXkZmbF5NWuPV8+WeEW8=
 github.com/Azure/go-autorest/tracing v0.6.0/go.mod h1:+vhtPC754Xsa23ID7GlGsrdKBpUA79WCAKPPZVC2DeU=
-github.com/Azure/k8s-work-api v0.2.0 h1:g6uE+C91NtpdeA/0cgPjpwFERGjMFhkmIXS5i9Limzk=
-github.com/Azure/k8s-work-api v0.2.0/go.mod h1:SFrmPheoFe7g8nNmNVclG2pAEP65GVeWdXpfUTYrcpY=
+github.com/Azure/k8s-work-api v0.3.0 h1:U6tU64Ku0B9Ru7Ymc9Q9NZvJO2BdKwLn9tMxxz0Ke6M=
+github.com/Azure/k8s-work-api v0.3.0/go.mod h1:KBriGSqVKS/h8FkfJT2ketfKB7F2lEjKVvdmy9T/jQY=
 github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0 h1:WVsrXCnHlDDX8ls+tootqRE87/hL9S/g4ewig9RsD/c=
 github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0/go.mod h1:Vt9sXTKwMyGcOxSmLDMnGPgqsUg7m8pe215qMLrDXw4=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
@@ -975,8 +975,6 @@ k8s.io/component-base v0.24.2 h1:kwpQdoSfbcH+8MPN4tALtajLDfSfYxBDYlXobNWI6OU=
 k8s.io/component-base v0.24.2/go.mod h1:ucHwW76dajvQ9B7+zecZAP3BVqvrHoOxm8olHEg0nmM=
 k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/gengo v0.0.0-20211129171323-c02415ce4185/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
-k8s.io/klog v1.0.0 h1:Pt+yjF5aB1xDSVbau4VsWe+dQNzA0qv1LlXdC2dF6Q8=
-k8s.io/klog v1.0.0/go.mod h1:4Bi6QPql/J/LkTDqv7R/cd3hPo4k2DG6Ptcz060Ez5I=
 k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
 k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
 k8s.io/klog/v2 v2.60.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=

diff --git a/pkg/authtoken/token_refresher.go b/pkg/authtoken/token_refresher.go
@@ -1,3 +1,8 @@
+/*
+Copyright (c) Microsoft Corporation.
+Licensed under the MIT license.
+*/
+
 package authtoken
 
 import (

diff --git a/pkg/controllers/clusterresourceplacement/cluster_selector.go b/pkg/controllers/clusterresourceplacement/cluster_selector.go
@@ -36,12 +36,13 @@ func (r *Reconciler) selectClusters(placement *fleetv1alpha1.ClusterResourcePlac
 		}
 		klog.V(4).InfoS("we select all the available clusters in the fleet without a policy",
 			"placement", placement.Name, "clusters", clusterNames)
-		return clusterNames, nil
+		return
 	}
 	// a fix list of clusters set
 	if len(placement.Spec.Policy.ClusterNames) != 0 {
 		klog.V(4).InfoS("use the cluster names provided as the list of cluster we select",
 			"placement", placement.Name, "clusters", placement.Spec.Policy.ClusterNames)
+		// TODO: filter by cluster health
 		return placement.Spec.Policy.ClusterNames, nil
 	}
 
@@ -53,7 +54,7 @@ func (r *Reconciler) selectClusters(placement *fleetv1alpha1.ClusterResourcePlac
 		}
 		klog.V(4).InfoS("we select all the available clusters in the fleet without a cluster affinity",
 			"placement", placement.Name, "clusters", clusterNames)
-		return clusterNames, nil
+		return
 	}
 
 	selectedClusters := make(map[string]bool)
@@ -62,15 +63,15 @@ func (r *Reconciler) selectClusters(placement *fleetv1alpha1.ClusterResourcePlac
 		if err != nil {
 			return nil, errors.Wrap(err, "cannot convert the label clusterSelector to a clusterSelector")
 		}
-		clusterNames, err := r.listClusters(selector)
+		matchClusters, err := r.listClusters(selector)
 		if err != nil {
 			return nil, errors.Wrap(err, fmt.Sprintf("selector = %v", clusterSelector.LabelSelector))
 		}
-		for _, clusterName := range clusterNames {
+		klog.V(4).InfoS("selector matches some cluster", "clusterNum", len(matchClusters), "placement", placement.Name, "selector", clusterSelector.LabelSelector)
+		for _, clusterName := range matchClusters {
 			selectedClusters[clusterName] = true
 		}
 	}
-
 	for cluster := range selectedClusters {
 		klog.V(4).InfoS("matched a cluster", "cluster", cluster, "placement", placement.Name)
 		clusterNames = append(clusterNames, cluster)
@@ -85,18 +86,19 @@ func (r *Reconciler) listClusters(labelSelector labels.Selector) ([]string, erro
 		return nil, errors.Wrap(err, "failed to list the clusters according to obj label selector")
 	}
 
-	clusterNames := make([]string, len(objs))
-	for i, obj := range objs {
+	clusterNames := make([]string, 0)
+	for _, obj := range objs {
 		uObj := obj.DeepCopyObject().(*unstructured.Unstructured)
 		var clusterObj fleetv1alpha1.MemberCluster
 		err = runtime.DefaultUnstructuredConverter.FromUnstructured(uObj.Object, &clusterObj)
 		if err != nil {
 			return nil, errors.Wrap(err, "cannot decode the member cluster object")
 		}
-		// only schedule the resource to a joined cluster
-		// TODO: check the health/condition of the cluster too
-		if clusterObj.Spec.State == fleetv1alpha1.ClusterStateJoin {
-			clusterNames[i] = clusterObj.GetName()
+		// only schedule the resource to an eligible cluster
+		// TODO: check the health condition of the cluster when its aggregated
+		joinCond := clusterObj.GetCondition(fleetv1alpha1.ConditionTypeMemberClusterJoin)
+		if joinCond != nil && joinCond.Status == metav1.ConditionTrue && joinCond.ObservedGeneration == clusterObj.Generation {
+			clusterNames = append(clusterNames, clusterObj.GetName())
 		}
 	}
 	return clusterNames, nil