Repository: GoogleContainerTools/kpt
Pull request #3894 (2023-03-23 20:48:27)

rollouts: add unit test and fake client for simple reconcileRollouts case
This PR adds a unit test for a simple case for `reconcileRollouts`. To support the unit test, this PR adds a fake client and refactors a few things out of `reconcileRollout`. I'd like to add the remaining unit tests in a followup, so that the review here can focus on structure of the test, fake client code, and refactoring, and the followup PR can focus on code coverage.

I looked at using https://github.com/golang/mock for the fake client, but felt that this fake client is simple enough that another framework wasn't needed. Happy to revisit this idea though.

Related issue: https://github.com/GoogleContainerTools/kpt/issues/3856


Patch:
diff --git a/rollouts/controllers/fakeclient.go b/rollouts/controllers/fakeclient.go
new file mode 100644
index 00000000..be1e1eb2
--- /dev/null
+++ b/rollouts/controllers/fakeclient.go
@@ -0,0 +1,108 @@
+/*
+Copyright 2022.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+	"fmt"
+
+	gitopsv1alpha1 "github.com/GoogleContainerTools/kpt/rollouts/api/v1alpha1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var _ client.Client = &fakeRemoteSyncClient{}
+
+type fakeRemoteSyncClient struct {
+	client.Client
+
+	remotesyncs map[types.NamespacedName]gitopsv1alpha1.RemoteSync
+	actions     []string
+}
+
+func newFakeRemoteSyncClient() *fakeRemoteSyncClient {
+	return &fakeRemoteSyncClient{
+		remotesyncs: make(map[types.NamespacedName]gitopsv1alpha1.RemoteSync),
+	}
+}
+
+func (fc *fakeRemoteSyncClient) Create(ctx context.Context, obj client.Object, _ ...client.CreateOption) error {
+	fc.actions = append(fc.actions, fmt.Sprintf("creating object named %q", obj.GetName()))
+
+	namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
+	if err := fc.Get(ctx, namespacedName, obj); err == nil {
+		return fmt.Errorf("object %q already exists", obj.GetName())
+	}
+	fc.remotesyncs[namespacedName] = *obj.(*gitopsv1alpha1.RemoteSync)
+
+	return nil
+}
+
+func (fc *fakeRemoteSyncClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error {
+	fc.actions = append(fc.actions, fmt.Sprintf("deleting object named %q", obj.GetName()))
+
+	namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
+	delete(fc.remotesyncs, namespacedName)
+
+	return nil
+}
+
+func (fc *fakeRemoteSyncClient) Update(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+	fc.actions = append(fc.actions, fmt.Sprintf("updating object named %q", obj.GetName()))
+
+	namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}
+	fc.remotesyncs[namespacedName] = *obj.(*gitopsv1alpha1.RemoteSync)
+
+	return nil
+}
+
+func (fc *fakeRemoteSyncClient) List(_ context.Context, obj client.ObjectList, _ ...client.ListOption) error {
+	fc.actions = append(fc.actions, fmt.Sprintf("listing objects"))
+
+	var remoteSyncList []gitopsv1alpha1.RemoteSync
+	for _, rs := range fc.remotesyncs {
+		remoteSyncList = append(remoteSyncList, rs)
+	}
+	*obj.(*gitopsv1alpha1.RemoteSyncList) = gitopsv1alpha1.RemoteSyncList{Items: remoteSyncList}
+
+	return nil
+}
+
+func (fc *fakeRemoteSyncClient) Get(_ context.Context, namespacedName types.NamespacedName, obj client.Object, _ ...client.GetOption) error {
+	fc.actions = append(fc.actions, fmt.Sprintf("getting object named %q", namespacedName.Name))
+
+	rs, found := fc.remotesyncs[namespacedName]
+	if found {
+		*obj.(*gitopsv1alpha1.RemoteSync) = rs
+		return nil
+	}
+
+	return &errors.StatusError{ErrStatus: metav1.Status{Reason: metav1.StatusReasonNotFound}}
+}
+
+func (fc *fakeRemoteSyncClient) setSyncStatus(namespacedName types.NamespacedName, syncStatus string) error {
+	rs, found := fc.remotesyncs[namespacedName]
+	if found {
+		rs.Status.SyncStatus = syncStatus
+		fc.remotesyncs[namespacedName] = rs
+		return nil
+	}
+
+	return &errors.StatusError{ErrStatus: metav1.Status{Reason: metav1.StatusReasonNotFound}}
+}
diff --git a/rollouts/controllers/rollout_controller.go b/rollouts/controllers/rollout_controller.go
index deeed051..93865b1e 100644
--- a/rollouts/controllers/rollout_controller.go
+++ b/rollouts/controllers/rollout_controller.go
@@ -154,10 +154,28 @@ func (r *RolloutReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 
 	packageDiscoveryClient := r.getPackageDiscoveryClient(req.NamespacedName)
 
-	err = r.reconcileRollout(ctx, &rollout, strategy, packageDiscoveryClient)
+	targetClusters, err := r.store.ListClusters(ctx, &rollout.Spec.Clusters, rollout.Spec.Targets.Selector)
+	if err != nil {
+		logger.Error(err, "Failed to list clusters")
+		return ctrl.Result{}, client.IgnoreNotFound(err)
+	}
+
+	discoveredPackages, err := packageDiscoveryClient.GetPackages(ctx, rollout.Spec.Packages)
+	if err != nil {
+		logger.Error(err, "Failed to discover packages")
+		return ctrl.Result{}, client.IgnoreNotFound(err)
+	}
+	logger.Info("Discovered packages", "packagesCount", len(discoveredPackages), "packages", packagediscovery.ToStr(discoveredPackages))
+
+	allClusterStatuses, waveStatuses, err := r.reconcileRollout(ctx, &rollout, strategy, targetClusters, discoveredPackages)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+
+	if err := r.updateStatus(ctx, &rollout, waveStatuses, allClusterStatuses); err != nil {
+		return ctrl.Result{}, err
+	}
+
 	if rollout.Spec.Clusters.SourceType == gitopsv1alpha1.GCPFleet &&
 		(rollout.Status.Overall == "Completed" || rollout.Status.Overall == "Stalled") {
 		// TODO (droot): The rollouts in completed/stalled state will not be reconciled
@@ -288,31 +306,18 @@ func (r *RolloutReconciler) getPackageDiscoveryClient(rolloutNamespacedName type
 	return client
 }
 
-func (r *RolloutReconciler) reconcileRollout(ctx context.Context, rollout *gitopsv1alpha1.Rollout, strategy *gitopsv1alpha1.ProgressiveRolloutStrategy, packageDiscoveryClient *packagediscovery.PackageDiscovery) error {
-	logger := klog.FromContext(ctx)
-
-	targetClusters, err := r.store.ListClusters(ctx, &rollout.Spec.Clusters, rollout.Spec.Targets.Selector)
-	if err != nil {
-		logger.Error(err, "Failed to list clusters")
-		return client.IgnoreNotFound(err)
-	}
-
-	discoveredPackages, err := packageDiscoveryClient.GetPackages(ctx, rollout.Spec.Packages)
-	if err != nil {
-		logger.Error(err, "Failed to discover packages")
-		return client.IgnoreNotFound(err)
-	}
-	logger.Info("Discovered packages", "packagesCount", len(discoveredPackages), "packages", packagediscovery.ToStr(discoveredPackages))
+func (r *RolloutReconciler) reconcileRollout(ctx context.Context, rollout *gitopsv1alpha1.Rollout, strategy *gitopsv1alpha1.ProgressiveRolloutStrategy, targetClusters []clusterstore.Cluster,
+	discoveredPackages []packagediscovery.DiscoveredPackage) ([]gitopsv1alpha1.ClusterStatus, []gitopsv1alpha1.WaveStatus, error) {
 
 	packageClusterMatcherClient := packageclustermatcher.NewPackageClusterMatcher(targetClusters, discoveredPackages)
 	clusterPackages, err := packageClusterMatcherClient.GetClusterPackages(rollout.Spec.PackageToTargetMatcher)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
 	targets, err := r.computeTargets(ctx, rollout, clusterPackages)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
 	allClusterStatuses := []gitopsv1alpha1.ClusterStatus{}
@@ -320,7 +325,7 @@ func (r *RolloutReconciler) reconcileRollout(ctx context.Context, rollout *gitop
 
 	allWaveTargets, err := r.getWaveTargets(ctx, rollout, targets, targetClusters, strategy.Spec.Waves)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
 	pauseFutureWaves := false
@@ -338,7 +343,7 @@ func (r *RolloutReconciler) reconcileRollout(ctx context.Context, rollout *gitop
 
 		thisWaveInProgress, clusterStatuses, err := r.rolloutTargets(ctx, rollout, wave, waveTargets, pauseFutureWaves)
 		if err != nil {
-			return err
+			return nil, nil, err
 		}
 
 		if thisWaveInProgress {
@@ -357,10 +362,7 @@ func (r *RolloutReconciler) reconcileRollout(ctx context.Context, rollout *gitop
 
 	sortClusterStatuses(allClusterStatuses)
 
-	if err := r.updateStatus(ctx, rollout, waveStatuses, allClusterStatuses); err != nil {
-		return err
-	}
-	return nil
+	return allClusterStatuses, waveStatuses, nil
 }
 
 func (r *RolloutReconciler) updateStatus(ctx context.Context, rollout *gitopsv1alpha1.Rollout, waveStatuses []gitopsv1alpha1.WaveStatus, clusterStatuses []gitopsv1alpha1.ClusterStatus) error {
diff --git a/rollouts/go.mod b/rollouts/go.mod
index 7d233ab5..64f154b1 100644
--- a/rollouts/go.mod
+++ b/rollouts/go.mod
@@ -14,8 +14,10 @@ require (
 	github.com/stretchr/testify v1.8.1
 	github.com/xanzy/go-gitlab v0.80.2
 	golang.org/x/oauth2 v0.3.0
+	golang.org/x/sync v0.1.0
 	google.golang.org/api v0.103.0
 	google.golang.org/genproto v0.0.0-20221201164419-0e50fba7f41c
+	gopkg.in/yaml.v2 v2.4.0
 	k8s.io/api v0.25.3
 	k8s.io/apimachinery v0.25.4
 	k8s.io/client-go v0.25.3
@@ -89,7 +91,6 @@ require (
 	google.golang.org/grpc v1.50.1 // indirect
 	google.golang.org/protobuf v1.28.1 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
-	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/apiextensions-apiserver v0.25.3 // indirect
 	k8s.io/component-base v0.25.3 // indirect
diff --git a/rollouts/go.sum b/rollouts/go.sum
index 14ccd983..ce405a04 100644
--- a/rollouts/go.sum
+++ b/rollouts/go.sum
@@ -462,6 +462,8 @@ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
+golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20181116152217-5ac8a444bdc5/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=


Test patch:
diff --git a/rollouts/controllers/rollout_controller_test.go b/rollouts/controllers/rollout_controller_test.go
index e479d4ba..3feba826 100644
--- a/rollouts/controllers/rollout_controller_test.go
+++ b/rollouts/controllers/rollout_controller_test.go
@@ -16,12 +16,18 @@ package controllers
 
 import (
 	"context"
+	"fmt"
+	"testing"
 	"time"
 
 	gitopsv1alpha1 "github.com/GoogleContainerTools/kpt/rollouts/api/v1alpha1"
 	e2eclusters "github.com/GoogleContainerTools/kpt/rollouts/e2e/clusters"
+	"github.com/GoogleContainerTools/kpt/rollouts/pkg/clusterstore"
+	"github.com/GoogleContainerTools/kpt/rollouts/pkg/packagediscovery"
+	"github.com/google/go-github/v48/github"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -371,3 +377,159 @@ var _ = Describe("Rollout", func() {
 		})
 	})
 })
+
+func TestReconcileRollout(t *testing.T) {
+	t.Run("maxconcurrent", func(t *testing.T) {
+		// This tests that if "maxConcurrent" is set to 1, only one cluster is synced at
+		// a time. This test calls `reconcileRollout` twice. After the first call,
+		// we check that only the first cluster starts progressing while the second is
+		// waiting. Then, we manually update the sync status of the first remotesync
+		// to "Synced", call `reconcileRollout` again, and verify that the second cluster
+		// starts progressing.
+
+		rollout := &gitopsv1alpha1.Rollout{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "Rollout",
+				APIVersion: "gitops.kpt.dev/v1alpha1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "sample",
+			},
+
+			Spec: gitopsv1alpha1.RolloutSpec{
+				PackageToTargetMatcher: gitopsv1alpha1.PackageToClusterMatcher{
+					Type: "AllClusters",
+				},
+				Strategy: gitopsv1alpha1.RolloutStrategy{
+					Type: "RollingUpdate",
+					RollingUpdate: &gitopsv1alpha1.StrategyRollingUpdate{
+						MaxConcurrent: 1,
+					},
+				},
+				Targets: gitopsv1alpha1.ClusterTargetSelector{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{"foo": "bar"},
+					},
+				},
+			},
+		}
+
+		// create 2 target clusters with arbitrary names
+		targetClusters := make([]clusterstore.Cluster, 2)
+		for i := 0; i < 2; i++ {
+			targetClusters[i] = clusterstore.Cluster{
+				Ref:    gitopsv1alpha1.ClusterRef{Name: fmt.Sprintf("foo/%d", i)},
+				Labels: map[string]string{"foo": "bar"},
+			}
+		}
+
+		// create an arbitrary package
+		testURL := "https://test.com/git"
+		discoveredPackage := packagediscovery.DiscoveredPackage{
+			Directory: "dir",
+			Revision:  "v0",
+			Branch:    "main",
+			GitHubRepo: &github.Repository{
+				CloneURL: &testURL,
+			},
+		}
+
+		fc := newFakeRemoteSyncClient()
+		reconciler := (&RolloutReconciler{Client: fc})
+
+		strategy, err := reconciler.getStrategy(context.Background(), rollout)
+		require.NoError(t, err)
+
+		// first call to reconcileRollout - only one cluster should start progressing
+		_, waveStatus, err := reconciler.reconcileRollout(
+			context.Background(),
+			rollout,
+			strategy,
+			targetClusters,
+			[]packagediscovery.DiscoveredPackage{discoveredPackage},
+		)
+
+		require.NoError(t, err)
+		require.Equal(t, []string{
+			"listing objects",
+			"getting object named \"github-0-dir-0\"",
+			"getting object named \"github-0-dir-1\"",
+			"creating object named \"github-0-dir-0\"",
+			"getting object named \"github-0-dir-0\"",
+		}, fc.actions)
+		require.Equal(t, 1, len(fc.remotesyncs))
+		require.Equal(t, []gitopsv1alpha1.WaveStatus{
+			{
+				Name:   "",
+				Status: "Progressing",
+				Paused: false,
+				ClusterStatuses: []gitopsv1alpha1.ClusterStatus{
+					{
+						Name: "foo/0",
+						PackageStatus: gitopsv1alpha1.PackageStatus{
+							PackageID:  "github-0-dir-0",
+							SyncStatus: "",
+							Status:     "Progressing",
+						},
+					},
+					{
+						Name: "foo/1",
+						PackageStatus: gitopsv1alpha1.PackageStatus{
+							PackageID:  "github-0-dir-1",
+							SyncStatus: "",
+							Status:     "Waiting",
+						},
+					},
+				},
+			},
+		}, waveStatus)
+
+		// reset actions and set sync status of remote sync to "synced"
+		fc.actions = nil
+		require.NoError(t, fc.setSyncStatus(types.NamespacedName{Name: "github-0-dir-0", Namespace: ""}, "Synced"))
+
+		// second call to reconcileRollout - the second cluster should now progress
+		_, waveStatus, err = reconciler.reconcileRollout(
+			context.Background(),
+			rollout,
+			strategy,
+			targetClusters,
+			[]packagediscovery.DiscoveredPackage{discoveredPackage},
+		)
+
+		require.NoError(t, err)
+		require.Equal(t, []string{
+			"listing objects",
+			"getting object named \"github-0-dir-0\"",
+			"getting object named \"github-0-dir-1\"",
+			"creating object named \"github-0-dir-1\"",
+			"getting object named \"github-0-dir-1\"",
+		}, fc.actions)
+		require.Equal(t, 2, len(fc.remotesyncs))
+		require.Equal(t, []gitopsv1alpha1.WaveStatus{
+			{
+				Name:   "",
+				Status: "Progressing",
+				Paused: false,
+				ClusterStatuses: []gitopsv1alpha1.ClusterStatus{
+					{
+						Name: "foo/0",
+						PackageStatus: gitopsv1alpha1.PackageStatus{
+							PackageID:  "github-0-dir-0",
+							SyncStatus: "Synced",
+							Status:     "Synced",
+						},
+					},
+					{
+						Name: "foo/1",
+						PackageStatus: gitopsv1alpha1.PackageStatus{
+							PackageID:  "github-0-dir-1",
+							SyncStatus: "",
+							Status:     "Progressing",
+						},
+					},
+				},
+			},
+		}, waveStatus)
+	})
+}