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.

Sign up for GitHub

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

Jump to bottom

rollouts: check entire spec when patching remotesync #3871

Merged
merged 2 commits into from
Mar 10, 2023
Merged

rollouts: check entire spec when patching remotesync #3871

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d254380
rollouts: check entire spec when patching rootsync
natasha41575 Mar 10, 2023
c2aaedc
code review
natasha41575 Mar 10, 2023
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
49 changes: 20 additions & 29 deletions rollouts/controllers/rollout_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import (
"flag"
"fmt"
"math"
"reflect"
"sort"
"strings"
"sync"
"time"

"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -432,7 +432,10 @@ func (r *RolloutReconciler) computeTargets(ctx context.Context,
}
} else {
// remotesync already exists
updated, needsUpdate := pkgNeedsUpdate(ctx, rollout, rs, pkg)
updated, needsUpdate := rsNeedsUpdate(ctx, rollout, &rs, &clusterPackagePair{
cluster: cluster,
packageRef: pkg,
})
if needsUpdate {
targets.ToBeUpdated = append(targets.ToBeUpdated, updated)
} else {
Expand All @@ -448,18 +451,17 @@ func (r *RolloutReconciler) computeTargets(ctx context.Context,
return targets, nil
}

func pkgNeedsUpdate(ctx context.Context, rollout *gitopsv1alpha1.Rollout, rs gitopsv1alpha1.RemoteSync, pkg *packagediscovery.DiscoveredPackage) (*gitopsv1alpha1.RemoteSync, bool) {
// TODO: We need to check other things here besides git.Revision and metadata
metadata := getSpecMetadata(rollout)
if pkg.Revision != rs.Spec.Template.Spec.Git.Revision ||
!reflect.DeepEqual(metadata, rs.Spec.Template.Metadata) ||
rs.Spec.Type != rollout.GetSyncTemplateType() {
// rsNeedsUpdate checks if the underlying remotesync needs to be updated by creating a new RemoteSync object and comparing it to the existing one
func rsNeedsUpdate(ctx context.Context, rollout *gitopsv1alpha1.Rollout, currentRS *gitopsv1alpha1.RemoteSync, target *clusterPackagePair) (*gitopsv1alpha1.RemoteSync, bool) {
desiredRS := newRemoteSync(rollout, target)

rs.Spec.Template.Spec.Git.Revision = pkg.Revision
rs.Spec.Template.Metadata = metadata
rs.Spec.Type = rollout.GetSyncTemplateType()
return &rs, true
// if the spec of the new RemoteSync object is not identical to the existing one, then an update is necessary
if !equality.Semantic.DeepEqual(currentRS.Spec, desiredRS.Spec) {
currentRS.Spec = desiredRS.Spec
return currentRS, true
}

// no update required, return nil
return nil, false
}

Expand Down Expand Up @@ -542,14 +544,7 @@ func (r *RolloutReconciler) rolloutTargets(ctx context.Context, rollout *gitopsv
}

for _, target := range targets.ToBeCreated {
syncSpec := toSyncSpec(target.packageRef)
rs := newRemoteSync(rollout,
gitopsv1alpha1.ClusterRef{Name: target.cluster.Name},
syncSpec,
pkgID(target.packageRef),
wave.Name,
getSpecMetadata(rollout),
)
rs := newRemoteSync(rollout, target)

if maxConcurrent > concurrentUpdates {
if err := r.Create(ctx, rs); err != nil {
Expand Down Expand Up @@ -719,21 +714,17 @@ func isRSErrored(rss *gitopsv1alpha1.RemoteSync) bool {
}

// Given a package identifier and cluster, create a RemoteSync object.
func newRemoteSync(rollout *gitopsv1alpha1.Rollout,
clusterRef gitopsv1alpha1.ClusterRef,
rssSpec *gitopsv1alpha1.SyncSpec,
pkgID string,
waveName string,
metadata *gitopsv1alpha1.Metadata) *gitopsv1alpha1.RemoteSync {
func newRemoteSync(rollout *gitopsv1alpha1.Rollout, target *clusterPackagePair) *gitopsv1alpha1.RemoteSync {
t := true
clusterRef := gitopsv1alpha1.ClusterRef{Name: target.cluster.Name}
clusterName := clusterRef.Name[strings.LastIndex(clusterRef.Name, "/")+1:]

// The RemoteSync object is created in the same namespace as the Rollout
// object. The RemoteSync will create either a RepoSync in the same namespace,
// or a RootSync in the config-management-system namespace.
return &gitopsv1alpha1.RemoteSync{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s-%s", pkgID, clusterName),
Name: fmt.Sprintf("%s-%s", pkgID(target.packageRef), clusterName),
Namespace: rollout.Namespace,
Labels: map[string]string{
rolloutLabel: rollout.Name,
Expand All @@ -753,8 +744,8 @@ func newRemoteSync(rollout *gitopsv1alpha1.Rollout,
Type: rollout.Spec.SyncTemplate.Type,
ClusterRef: clusterRef,
Template: &gitopsv1alpha1.Template{
Spec: rssSpec,
Metadata: metadata,
Spec: toSyncSpec(target.packageRef),
Metadata: getSpecMetadata(rollout),
},
},
}
Expand Down