address review comments

Author: Yuvaraj Kakaraparthi

exp/runtime/api/v1alpha1/extensionconfig_types.go

@@ -209,5 +209,7 @@ const (
 	InjectCAFromSecretAnnotation string = "runtime.cluster.x-k8s.io/inject-ca-from-secret"
 
 	// PendingHooksAnnotation is the annotation used to keep a track of pending runtime hooks.
-	PendingHooksAnnotation string = "hooks.x-cluster.k8s.io/pending-hooks"
+	// The annotation will be used to track the intent to call a hook as soon as an operation completes;
+	// the intent will be removed as soon as the hook call completes successfully.
+	PendingHooksAnnotation string = "runtime.cluster.x-k8s.io/pending-hooks"
 )

exp/runtime/hooks/api/v1alpha1/lifecyclehooks_types.go

@@ -188,7 +188,7 @@ func init() {
 	catalogBuilder.RegisterHook(AfterControlPlaneInitialized, &runtimecatalog.HookMeta{
 		Tags:        []string{"Lifecycle Hooks"},
 		Summary:     "Called after the Control Plane is available for the first time",
-		Description: "This non-blocking hook is called after the ControlPlane for the Cluster is marked as available for the first time",
+		Description: "This non-blocking hook is called after the ControlPlane for the Cluster reachable for the first time",
 	})
 
 	catalogBuilder.RegisterHook(BeforeClusterUpgrade, &runtimecatalog.HookMeta{

internal/controllers/topology/cluster/desired_state.go

@@ -317,9 +317,10 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 
 		// Call the AfterControlPlaneUpgrade now that the control plane is upgraded.
 		if feature.Gates.Enabled(feature.RuntimeSDK) {
-			// Call the hook only if it is marked. If it is not marked it means we don't need ot call the
+			// Call the hook only if we are tracking the intent to do so. If it is not tracked it means we don't need to call the
 			// hook because we didn't go through an upgrade or we already called the hook after the upgrade.
 			if hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, s.Current.Cluster) {
+				// Call all the registered extension for the hook.
 				hookRequest := &runtimehooksv1.AfterControlPlaneUpgradeRequest{
 					Cluster:           *s.Current.Cluster,
 					KubernetesVersion: desiredVersion,
@@ -328,14 +329,17 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 				if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.AfterControlPlaneUpgrade, s.Current.Cluster, hookRequest, hookResponse); err != nil {
 					return "", errors.Wrapf(err, "error calling the %s hook", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade))
 				}
+				// Add the response to the tracker so we can later update condition or requeue when required.
 				s.HookResponseTracker.Add(runtimehooksv1.AfterControlPlaneUpgrade, hookResponse)
+
+				// If the extension responds to hold off on starting Machine deployments upgrades,
+				// change the UpgradeTracker accordingly, otherwise the hook call is completed and we
+				// can remove this hook from the list of pending-hooks.
 				if hookResponse.RetryAfterSeconds != 0 {
-					// We have to block the upgrade of the Machine deployments.
 					s.UpgradeTracker.MachineDeployments.HoldUpgrades(true)
 				} else {
-					// We are done with the hook for now. We don't need to call it anymore. Unmark it.
 					if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade); err != nil {
-						return "", errors.Wrapf(err, "failed to unmark the %s hook", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade))
+						return "", errors.Wrapf(err, "failed to remove the %s hook from pending hooks tracker", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade))
 					}
 				}
 			}
@@ -377,16 +381,17 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
 		if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.BeforeClusterUpgrade, s.Current.Cluster, hookRequest, hookResponse); err != nil {
 			return "", errors.Wrapf(err, "failed to call %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade))
 		}
+		// Add the response to the tracker so we can later update condition or requeue when required.
 		s.HookResponseTracker.Add(runtimehooksv1.BeforeClusterUpgrade, hookResponse)
 		if hookResponse.RetryAfterSeconds != 0 {
 			// Cannot pickup the new version right now. Need to try again later.
 			return *currentVersion, nil
 		}
 
 		// We are picking up the new version here.
-		// Mark the AfterControlPlaneUpgrade and the AfterClusterUpgrade hooks so that we call them once we are done with the upgrade.
+		// Track the intent of calling the AfterControlPlaneUpgrade and the AfterClusterUpgrade hooks once we are done with the upgrade.
 		if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneUpgrade, runtimehooksv1.AfterClusterUpgrade); err != nil {
-			return "", errors.Wrapf(err, "failed to mark the %s hook", []string{runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade), runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)})
+			return "", errors.Wrapf(err, "failed to mark the %s hook as pending", []string{runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade), runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)})
 		}
 	}
 

internal/controllers/topology/cluster/desired_state_test.go

@@ -908,7 +908,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 			name                string
 			s                   *scope.Scope
 			hookResponse        *runtimehooksv1.AfterControlPlaneUpgradeResponse
-			wantMarked          bool
+			wantIntentToCall    bool
 			wantHookToBeCalled  bool
 			wantAllowMDUpgrades bool
 			wantErr             bool
@@ -937,12 +937,12 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					UpgradeTracker:      scope.NewUpgradeTracker(),
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
-				wantMarked:         false,
+				wantIntentToCall:   false,
 				wantHookToBeCalled: false,
 				wantErr:            false,
 			},
 			{
-				name: "should not call hook if the control plane is provisioning - hook is marked",
+				name: "should not call hook if the control plane is provisioning - there is intent to call hook",
 				s: &scope.Scope{
 					Blueprint: &scope.ClusterBlueprint{
 						Topology: &clusterv1.Topology{
@@ -968,12 +968,12 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					UpgradeTracker:      scope.NewUpgradeTracker(),
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
-				wantMarked:         true,
+				wantIntentToCall:   true,
 				wantHookToBeCalled: false,
 				wantErr:            false,
 			},
 			{
-				name: "should not call hook if the control plane is upgrading - hook is marked",
+				name: "should not call hook if the control plane is upgrading - there is intent to call hook",
 				s: &scope.Scope{
 					Blueprint: &scope.ClusterBlueprint{
 						Topology: &clusterv1.Topology{
@@ -999,12 +999,12 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					UpgradeTracker:      scope.NewUpgradeTracker(),
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
-				wantMarked:         true,
+				wantIntentToCall:   true,
 				wantHookToBeCalled: false,
 				wantErr:            false,
 			},
 			{
-				name: "should call hook if the control plane is at desired version - non blocking response should unmark hook and allow MD upgrades",
+				name: "should call hook if the control plane is at desired version - non blocking response should remove hook from pending hooks list and allow MD upgrades",
 				s: &scope.Scope{
 					Blueprint: &scope.ClusterBlueprint{
 						Topology: &clusterv1.Topology{
@@ -1031,13 +1031,13 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
 				hookResponse:        nonBlockingResponse,
-				wantMarked:          false,
+				wantIntentToCall:    false,
 				wantHookToBeCalled:  true,
 				wantAllowMDUpgrades: true,
 				wantErr:             false,
 			},
 			{
-				name: "should call hook if the control plane is at desired version - blocking response should leave the hook as marked and block MD upgrades",
+				name: "should call hook if the control plane is at desired version - blocking response should leave the hook in pending hooks list and block MD upgrades",
 				s: &scope.Scope{
 					Blueprint: &scope.ClusterBlueprint{
 						Topology: &clusterv1.Topology{
@@ -1064,13 +1064,13 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
 				hookResponse:        blockingResponse,
-				wantMarked:          true,
+				wantIntentToCall:    true,
 				wantHookToBeCalled:  true,
 				wantAllowMDUpgrades: false,
 				wantErr:             false,
 			},
 			{
-				name: "should call hook if the control plane is at desired version - failure response should leave the hook as marked",
+				name: "should call hook if the control plane is at desired version - failure response should leave the hook in pending hooks list",
 				s: &scope.Scope{
 					Blueprint: &scope.ClusterBlueprint{
 						Topology: &clusterv1.Topology{
@@ -1097,7 +1097,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 					HookResponseTracker: scope.NewHookResponseTracker(),
 				},
 				hookResponse:       failureResponse,
-				wantMarked:         true,
+				wantIntentToCall:   true,
 				wantHookToBeCalled: true,
 				wantErr:            true,
 			},
@@ -1124,7 +1124,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 
 				_, err := r.computeControlPlaneVersion(ctx, tt.s)
 				g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.AfterControlPlaneUpgrade) == 1).To(Equal(tt.wantHookToBeCalled))
-				g.Expect(hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, tt.s.Current.Cluster)).To(Equal(tt.wantMarked))
+				g.Expect(hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, tt.s.Current.Cluster)).To(Equal(tt.wantIntentToCall))
 				g.Expect(err != nil).To(Equal(tt.wantErr))
 				if tt.wantHookToBeCalled && !tt.wantErr {
 					g.Expect(tt.s.UpgradeTracker.MachineDeployments.AllowUpgrade()).To(Equal(tt.wantAllowMDUpgrades))
@@ -1133,7 +1133,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 		}
 	})
 
-	t.Run("marking AfterClusterUpgrade and AfterControlPlaneUpgrade hooks", func(t *testing.T) {
+	t.Run("register intent to call AfterClusterUpgrade and AfterControlPlaneUpgrade hooks", func(t *testing.T) {
 		defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
 
 		catalog := runtimecatalog.New()
@@ -1201,7 +1201,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
 		desiredVersion, err := r.computeControlPlaneVersion(ctx, s)
 		g := NewWithT(t)
 		g.Expect(err).To(BeNil())
-		// When successfully picking up the new version the AfterControlPlaneUpgrade and AfterClusterUpgrade hooks should be marked
+		// When successfully picking up the new version the intent to call AfterControlPlaneUpgrade and AfterClusterUpgrade hooks should be registered.
 		g.Expect(desiredVersion).To(Equal("v1.2.3"))
 		g.Expect(hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, s.Current.Cluster)).To(BeTrue())
 		g.Expect(hooks.IsPending(runtimehooksv1.AfterClusterUpgrade, s.Current.Cluster)).To(BeTrue())

internal/controllers/topology/cluster/reconcile_state.go

@@ -202,18 +202,18 @@ func (r *Reconciler) callAfterHooks(ctx context.Context, s *scope.Scope) error {
 }
 
 func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *scope.Scope) error {
-	/*
-		TODO: Working comment - DELETE AFTER:
-		- If the cluster topology is being created then mark the AfterControlPlaneInitialized hook so that we can call it later.
-	*/
+	// If the cluster topology is being created then track to intent to call the AfterControlPlaneInitialized hook so that we can call it later.
 	if s.Current.Cluster.Spec.InfrastructureRef == nil && s.Current.Cluster.Spec.ControlPlaneRef == nil {
 		if err := hooks.MarkAsPending(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterControlPlaneInitialized); err != nil {
-			return errors.Wrapf(err, "failed to mark %s hook", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneInitialized))
+			return errors.Wrapf(err, "failed to remove the %s hook from pending hooks tracker", runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneInitialized))
 		}
 	}
 
+	// Call the hook only if we are tracking the intent to do so. If it is not tracked it means we don't need to call the
+	// hook because already called the hook after the control plane is initialized.
 	if hooks.IsPending(runtimehooksv1.AfterControlPlaneInitialized, s.Current.Cluster) {
 		if isControlPlaneInitialized(s.Current.Cluster) {
+			// The control plane is initialized for the first time. Call all the registered extensions for the hook.
 			hookRequest := &runtimehooksv1.AfterControlPlaneInitializedRequest{
 				Cluster: *s.Current.Cluster,
 			}
@@ -233,8 +233,6 @@ func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *sc
 
 func isControlPlaneInitialized(cluster *clusterv1.Cluster) bool {
 	for _, condition := range cluster.GetConditions() {
-		// TODO: Should we check for the ControlPlaneInitialized condition or the ControlPlaneReadyCondition?
-		// From the description of the hook it looks like it should be the ControlPlaneReadyCondition - but need to double check.
 		if condition.Type == clusterv1.ControlPlaneInitializedCondition {
 			if condition.Status == corev1.ConditionTrue {
 				return true
@@ -245,23 +243,26 @@ func isControlPlaneInitialized(cluster *clusterv1.Cluster) bool {
 }
 
 func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope) error {
-	/*
-		TODO: Working comment - DELETE LATER:
-		- if the AfterClusterUpgrade hook is pending then check that the cluster is fully upgraded. If it is fully upgraded then call the hook.
-			- A cluster is full upgraded if
-				- Control plane is not upgrading
-				- Control plane is not scaling
-				- Control plane is not pending an upgrade
-				- MachineDeployments are not currently rolling out
-				- MAchineDeployments are not about to roll out
-				- MachineDeployments are not pending an upgrade
-	*/
+	// Call the hook only if we are tracking the intent to do so. If it is not tracked it means we don't need to call the
+	// hook because we didn't go through an upgrade or we already called the hook after the upgrade.
 	if hooks.IsPending(runtimehooksv1.AfterClusterUpgrade, s.Current.Cluster) {
+		// Call the registered extensions for the hook after the cluster is fully upgraded.
+		// A clusters is considered fully upgraded if:
+		// - Control plane is not upgrading
+		// - Control plane is not scaling
+		// - Control plane is not pending an upgrade
+		// - MachineDeployments are not currently rolling out
+		// - MAchineDeployments are not about to roll out
+		// - MachineDeployments are not pending an upgrade
+
+		// Check if the control plane is upgrading.
 		cpUpgrading, err := contract.ControlPlane().IsUpgrading(s.Current.ControlPlane.Object)
 		if err != nil {
 			return errors.Wrap(err, "failed to check if control plane is upgrading")
 		}
 
+		// Check if the control plane is scaling. If the control plane does not support replicas
+		// it will be considered as not scaling.
 		var cpScaling bool
 		if s.Blueprint.Topology.ControlPlane.Replicas != nil {
 			cpScaling, err = contract.ControlPlane().IsScaling(s.Current.ControlPlane.Object)
@@ -272,7 +273,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
 
 		if !cpUpgrading && !cpScaling && !s.UpgradeTracker.ControlPlane.PendingUpgrade && // Control Plane checks
 			len(s.UpgradeTracker.MachineDeployments.RolloutNames()) == 0 && // Machine deployments are not rollout out or not about to roll out
-			!s.UpgradeTracker.MachineDeployments.PendingUpgrade() { // Machine Deployments is are not pending an upgrade
+			!s.UpgradeTracker.MachineDeployments.PendingUpgrade() { // Machine Deployments are not pending an upgrade
 			// Everything is stable and the cluster can be considered fully upgraded.
 			hookRequest := &runtimehooksv1.AfterClusterUpgradeRequest{
 				Cluster:           *s.Current.Cluster,
@@ -283,11 +284,9 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
 				return errors.Wrapf(err, "failed to call %s hook", runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade))
 			}
 			s.HookResponseTracker.Add(runtimehooksv1.AfterClusterUpgrade, hookResponse)
-			// The hook is successfully called. We can unmark the hook.
-			// TODO: follow up check - what if the cluster object in current is not updated with the latest tracking annotation.
-			// 							Is that possible?
+			// The hook is successfully called; we can remove this hook from the list of pending-hooks.
 			if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil {
-				return errors.Wrapf(err, "failed to unmark the %s hook", runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade))
+				return errors.Wrapf(err, "failed to remove the %s hook from pending hooks tracker", runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade))
 			}
 		}
 	}

internal/controllers/topology/cluster/scope/upgradetracker.go

@@ -78,7 +78,8 @@ func (m *MachineDeploymentUpgradeTracker) RolloutNames() []string {
 	return m.rollingOutNames.List()
 }
 
-// HoldUpgrades is used to set if any subsequent upgrade operations should be paused.
+// HoldUpgrades is used to set if any subsequent upgrade operations should be paused,
+// e.g. because a AfterControlPlaneUpgrade hook response asked to do so.
 // If HoldUpgrades is called with `true` then AllowUpgrade would return false.
 func (m *MachineDeploymentUpgradeTracker) HoldUpgrades(val bool) {
 	m.holdUpgrades = val

internal/hooks/tracking.go

@@ -30,8 +30,9 @@ import (
 	"sigs.k8s.io/cluster-api/util/patch"
 )
 
-// MarkAsPending sets the information on the object to signify that the hook is marked.
-func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) (retErr error) {
+// MarkAsPending adds to the object's PendingHooksAnnotation the intent to execute a hook after an operation completes.
+// Usually this function is called when an operation is starting in order to track the intent to call an After<operation> hook later in the process.
+func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
 		return errors.Wrap(err, "failed to create patch helper")
@@ -56,7 +57,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
 	return nil
 }
 
-// IsPending returns true if the hook is marked on the object.
+// IsPending returns true if there is an intent to call a hook being tracked in the object's PendingHooksAnnotation.
 func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
 	hookName := runtimecatalog.HookName(hook)
 	annotations := obj.GetAnnotations()
@@ -66,8 +67,10 @@ func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
 	return isInCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookName)
 }
 
-// MarkAsDone removes the information on the object that represents that the hook is pending.
-func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) (retErr error) {
+// MarkAsDone removes the intent to call a Hook from the object's PendingHooksAnnotation.
+// Usually this func is called after all the registered extensions for the Hook returned an answer without requests
+// to hold on to the object's lifecycle (retryAfterSeconds).
+func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) error {
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
 		return errors.Wrap(err, "failed to create patch helper")