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

Fix bugs in RootSyncSet controller #3651

Merged
merged 1 commit into from
Nov 8, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

Expand Down
1 change: 1 addition & 0 deletions porch/controllers/rootsyncsets/config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ rules:
verbs:
- get
- list
- watch
- apiGroups:
- container.cnrm.cloud.google.com
resources:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
}
Expand All @@ -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.
Expand All @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down