Skip to content

Commit

Permalink
rollouts: add unit test and fake client for simple reconcileRollouts …
Browse files Browse the repository at this point in the history
…case (#3894)
  • Loading branch information
natasha41575 committed Mar 25, 2023
1 parent ce0ff27 commit 4cee890
Show file tree
Hide file tree
Showing 5 changed files with 300 additions and 25 deletions.
108 changes: 108 additions & 0 deletions rollouts/controllers/fakeclient.go
Original file line number Diff line number Diff line change
@@ -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}}
}
50 changes: 26 additions & 24 deletions rollouts/controllers/rollout_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -288,39 +306,26 @@ 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{}
waveStatuses := []gitopsv1alpha1.WaveStatus{}

allWaveTargets, err := r.getWaveTargets(ctx, rollout, targets, targetClusters, strategy.Spec.Waves)
if err != nil {
return err
return nil, nil, err
}

pauseFutureWaves := false
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
162 changes: 162 additions & 0 deletions rollouts/controllers/rollout_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
})
}
Loading

0 comments on commit 4cee890

Please sign in to comment.