Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rollouts: add unit test and fake client for simple reconcileRollouts case #3894

Merged
merged 2 commits into from
Mar 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using this style of assertions in other parts of kpt project ? otherwise I have found omega matchers to be very good for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of places in kpt repo that we are using the testify/assert and testify/require packages to do similar assertions in unit tests. require is a subtly different flavor of assert, except that require exits the test immediately, while assert continues running the rest of the test even when it encounters an error.

That said, I am happy to convert to omega matchers if you prefer!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see usage of require before. But the failure behavior of require does make more sense.

I am fine using require here, so let's ship it.

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