From ae23831cf9c82ab4be75c831a614210f390b0e19 Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Tue, 8 Nov 2022 02:25:46 +0000 Subject: [PATCH] Fix bugs in RootSyncSet controller --- .../api/v1alpha1/rootsyncset_types.go | 2 ++ .../rootsyncsets/config/rbac/role.yaml | 1 + .../pkg/controllers/rootsyncset/controller.go | 13 +++++++++---- .../pkg/controllers/rootsyncset/status.go | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/porch/controllers/rootsyncsets/api/v1alpha1/rootsyncset_types.go b/porch/controllers/rootsyncsets/api/v1alpha1/rootsyncset_types.go index 963e487137..38878f1141 100644 --- a/porch/controllers/rootsyncsets/api/v1alpha1/rootsyncset_types.go +++ b/porch/controllers/rootsyncsets/api/v1alpha1/rootsyncset_types.go @@ -63,6 +63,8 @@ type SecretReference struct { // RootSyncSetStatus defines the observed state of RootSyncSet type RootSyncSetStatus struct { + ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // Conditions describes the reconciliation state of the object. Conditions []metav1.Condition `json:"conditions,omitempty"` diff --git a/porch/controllers/rootsyncsets/config/rbac/role.yaml b/porch/controllers/rootsyncsets/config/rbac/role.yaml index bb23cdea3e..75c8ed6ad4 100644 --- a/porch/controllers/rootsyncsets/config/rbac/role.yaml +++ b/porch/controllers/rootsyncsets/config/rbac/role.yaml @@ -58,6 +58,7 @@ rules: verbs: - get - list + - watch - apiGroups: - container.cnrm.cloud.google.com resources: diff --git a/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go b/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go index d69e0fbdd2..f0e3772f46 100644 --- a/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go +++ b/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/controller.go @@ -100,7 +100,7 @@ type RootSyncSetReconciler struct { //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=rootsyncsets,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=rootsyncsets/status,verbs=get;update;patch //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=rootsyncsets/finalizers,verbs=update -//+kubebuilder:rbac:groups=configcontroller.cnrm.cloud.google.com,resources=configcontrollerinstances,verbs=get;list +//+kubebuilder:rbac:groups=configcontroller.cnrm.cloud.google.com,resources=configcontrollerinstances,verbs=get;list;watch //+kubebuilder:rbac:groups=container.cnrm.cloud.google.com,resources=containerclusters,verbs=get;list;watch //+kubebuilder:rbac:groups=core.cnrm.cloud.google.com,resources=configconnectorcontexts,verbs=get;list;watch //+kubebuilder:rbac:groups=hub.gke.io,resources=memberships,verbs=get;list;watch @@ -242,11 +242,13 @@ func (r *RootSyncSetReconciler) updateStatus(ctx context.Context, rss *v1alpha1. } // Don't update if there are no changes. - if equality.Semantic.DeepEqual(rss.Status.ClusterRefStatuses, crss) { + if equality.Semantic.DeepEqual(rss.Status.ClusterRefStatuses, crss) && + rss.Generation == rss.Status.ObservedGeneration { return nil } rss.Status.ClusterRefStatuses = crss + rss.Status.ObservedGeneration = rss.Generation return r.Client.Status().Update(ctx, rss) } @@ -337,8 +339,11 @@ func (r *RootSyncSetReconciler) setupWatches(ctx context.Context, client dynamic cancelFunc: cancelFunc, client: client, channel: r.channel, - liens: make(map[types.NamespacedName]struct{}), + liens: map[types.NamespacedName]struct{}{ + nn: {}, + }, } + klog.Infof("Creating watcher for %v", clusterRef) go w.watch() r.watchers[clusterRef] = w } @@ -349,6 +354,7 @@ func (r *RootSyncSetReconciler) setupWatches(ctx context.Context, client dynamic func (r *RootSyncSetReconciler) pruneWatches(rssnn types.NamespacedName, clusterRefs []*v1alpha1.ClusterRef) { r.mutex.Lock() defer r.mutex.Unlock() + klog.Infof("Pruning watches for %s which has %v clusterRefs", rssnn.String(), clusterRefs) // Look through all watchers to check if it used to be needed by the RootSyncSet // but is no longer. @@ -368,7 +374,6 @@ func (r *RootSyncSetReconciler) pruneWatches(rssnn types.NamespacedName, cluster delete(w.liens, rssnn) // If no other RootSyncSets need the watch, stop it and remove the watcher from the map. if len(w.liens) == 0 { - klog.Infof("clusterRef %s is no longer needed, so closing watch", clusterRef.Name) w.cancelFunc() delete(r.watchers, clusterRef) } diff --git a/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/status.go b/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/status.go index d66cbadb3e..6f9dec681f 100644 --- a/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/status.go +++ b/porch/controllers/rootsyncsets/pkg/controllers/rootsyncset/status.go @@ -26,11 +26,26 @@ import ( // checkSyncStatus fetches the RootSync using the provided client and computes the sync status. The rules // for computing status here mirrors the one used in the status command in the nomos cli. func checkSyncStatus(ctx context.Context, client dynamic.Interface, rssName string) (string, error) { + // TODO: Change this to use the RootSync type instead of Unstructured. rs, err := client.Resource(rootSyncGVR).Namespace(rootSyncNamespace).Get(ctx, rssName, metav1.GetOptions{}) if err != nil { return "", fmt.Errorf("failed to get RootSync: %w", err) } + generation, _, err := unstructured.NestedInt64(rs.Object, "metadata", "generation") + if err != nil { + return "", fmt.Errorf("failed to read generation from RootSync: %w", err) + } + + observedGeneration, _, err := unstructured.NestedInt64(rs.Object, "status", "observedGeneration") + if err != nil { + return "", fmt.Errorf("failed to read observedGeneration from RootSync: %w", err) + } + + if generation != observedGeneration { + return "Pending", nil + } + conditions, _, err := unstructured.NestedSlice(rs.Object, "status", "conditions") if err != nil { return "", fmt.Errorf("failed to extract conditions from RootSync: %w", err)