fix review comments

Author: Yuvaraj Kakaraparthi

internal/controllers/cluster/cluster_controller.go

@@ -40,7 +40,6 @@ import (
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
-	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	"sigs.k8s.io/cluster-api/feature"
 	runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client"
 	"sigs.k8s.io/cluster-api/util"
@@ -219,22 +218,6 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster)
 func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	// Call the BeforeClusterDelete hook to before proceeding further.
-	if feature.Gates.Enabled(feature.RuntimeSDK) {
-		hookRequest := &runtimehooksv1.BeforeClusterDeleteRequest{
-			Cluster: *cluster,
-		}
-		hookResponse := &runtimehooksv1.BeforeClusterDeleteResponse{}
-		if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.BeforeClusterDelete, hookRequest, hookResponse); err != nil {
-			return ctrl.Result{}, errors.Wrap(err, "error calling BeforeClusterDelete hook")
-		}
-		if hookResponse.RetryAfterSeconds != 0 {
-			// Cannot proceed with deleting the cluster yet. Lets requeue to retry at a later time.
-			return ctrl.Result{RequeueAfter: time.Duration(hookResponse.RetryAfterSeconds) * time.Second}, nil
-		}
-		// We can proceed with the delete operation.
-	}
-
 	descendants, err := r.listDescendants(ctx, cluster)
 	if err != nil {
 		log.Error(err, "Failed to list descendants")

internal/controllers/cluster/cluster_controller_test.go

@@ -18,24 +18,18 @@ package cluster
 
 import (
 	"testing"
-	"time"
 
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	utilfeature "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
-	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	"sigs.k8s.io/cluster-api/feature"
-	runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
-	runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client"
-	runtimeclienttest "sigs.k8s.io/cluster-api/internal/runtime/client/test"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
@@ -355,91 +349,6 @@ func TestClusterReconciler(t *testing.T) {
 		}, timeout).Should(BeTrue())
 	})
 }
-
-func TestReconcileDelete(t *testing.T) {
-	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
-	gvh, err := runtimeclienttest.DefaultTestCatalog.GroupVersionHook(runtimehooksv1.BeforeClusterDelete)
-	if err != nil {
-		panic(err)
-	}
-
-	blockingResponse := &runtimehooksv1.BeforeClusterDeleteResponse{
-		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
-			RetryAfterSeconds: int32(10),
-		},
-	}
-	runtimeClientWithBlockingResponse := runtimeclienttest.NewFakeRuntimeClientBuilder().
-		WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
-			gvh: blockingResponse,
-		}).
-		Build()
-
-	nonBlockingResponse := &runtimehooksv1.BeforeClusterDeleteResponse{
-		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
-			RetryAfterSeconds: int32(0),
-		},
-	}
-	runtimeClientWithNonBlockingResponse := runtimeclienttest.NewFakeRuntimeClientBuilder().
-		WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
-			gvh: nonBlockingResponse,
-		}).
-		Build()
-
-	tests := []struct {
-		name          string
-		cluster       *clusterv1.Cluster
-		runtimeClient runtimeclient.Client
-		wantResult    ctrl.Result
-		wantErr       bool
-	}{
-		{
-			name: "should requeue if BeforeClusterDelete hook is blocking",
-			cluster: &clusterv1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-name",
-					Namespace: "test-ns",
-				},
-				Spec: clusterv1.ClusterSpec{},
-			},
-			runtimeClient: runtimeClientWithBlockingResponse,
-			wantResult:    ctrl.Result{RequeueAfter: time.Duration(10) * time.Second},
-			wantErr:       false,
-		},
-		{
-			name: "should perform delete if BeforeClusterDelete hook is non-blocking",
-			cluster: &clusterv1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-name",
-					Namespace: "test-ns",
-				},
-				Spec: clusterv1.ClusterSpec{},
-			},
-			runtimeClient: runtimeClientWithNonBlockingResponse,
-			wantResult:    ctrl.Result{},
-			wantErr:       false,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			r := &Reconciler{
-				RuntimeClient: tt.runtimeClient,
-				Client:        env,
-				APIReader:     env,
-			}
-			result, err := r.reconcileDelete(ctx, tt.cluster)
-			if tt.wantErr {
-				g.Expect(err).NotTo(BeNil())
-			} else {
-				g.Expect(err).To(BeNil())
-				g.Expect(result).To(Equal(tt.wantResult))
-			}
-		})
-	}
-}
-
 func TestClusterReconcilerNodeRef(t *testing.T) {
 	t.Run("machine to cluster", func(t *testing.T) {
 		cluster := &clusterv1.Cluster{

internal/controllers/topology/cluster/cluster_controller.go

@@ -41,6 +41,7 @@ import (
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope"
+	runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
 	runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -234,7 +235,7 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result
 		return ctrl.Result{}, errors.Wrap(err, "error reconciling the Cluster topology")
 	}
 
-	requeueAfter := s.HookResponseTracker.EffectiveRequeueAfter()
+	requeueAfter := s.HookResponseTracker.EffectiveRetryAfter()
 	if requeueAfter != 0 {
 		return ctrl.Result{RequeueAfter: requeueAfter}, nil
 	}
@@ -272,9 +273,9 @@ func (r *Reconciler) callBeforeClusterCreateHook(ctx context.Context, s *scope.S
 		}
 		hookResponse := &runtimehooksv1.BeforeClusterCreateResponse{}
 		if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.BeforeClusterCreate, hookRequest, hookResponse); err != nil {
-			return ctrl.Result{}, errors.Wrap(err, "error calling the BeforeClusterCreate hook")
+			return ctrl.Result{}, errors.Wrapf(err, "error calling the %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterCreate))
 		}
-		s.HookResponseTracker.Add("BeforeClusterCreate", hookResponse)
+		s.HookResponseTracker.Add(runtimehooksv1.BeforeClusterCreate, hookResponse)
 		if hookResponse.RetryAfterSeconds != 0 {
 			return ctrl.Result{RequeueAfter: time.Duration(hookResponse.RetryAfterSeconds) * time.Second}, nil
 		}

internal/controllers/topology/cluster/cluster_controller_test.go

@@ -36,7 +36,7 @@ import (
 	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope"
 	runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
-	runtimeclienttest "sigs.k8s.io/cluster-api/internal/runtime/client/test"
+	fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
@@ -443,18 +443,26 @@ func TestClusterReconciler_deleteClusterClass(t *testing.T) {
 }
 
 func TestReconciler_callBeforeClusterCreateHook(t *testing.T) {
-	gvh, err := runtimeclienttest.DefaultTestCatalog.GroupVersionHook(runtimehooksv1.BeforeClusterCreate)
+	catalog := runtimecatalog.New()
+	_ = runtimehooksv1.AddToCatalog(catalog)
+	gvh, err := catalog.GroupVersionHook(runtimehooksv1.BeforeClusterCreate)
 	if err != nil {
 		panic(err)
 	}
 
 	blockingResponse := &runtimehooksv1.BeforeClusterCreateResponse{
 		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+			CommonResponse: runtimehooksv1.CommonResponse{
+				Status: runtimehooksv1.ResponseStatusSuccess,
+			},
 			RetryAfterSeconds: int32(10),
 		},
 	}
 	nonBlockingResponse := &runtimehooksv1.BeforeClusterCreateResponse{
 		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+			CommonResponse: runtimehooksv1.CommonResponse{
+				Status: runtimehooksv1.ResponseStatusSuccess,
+			},
 			RetryAfterSeconds: int32(0),
 		},
 	}
@@ -496,7 +504,8 @@ func TestReconciler_callBeforeClusterCreateHook(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			runtimeClient := runtimeclienttest.NewFakeRuntimeClientBuilder().
+			runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
+				WithCatalog(catalog).
 				WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
 					gvh: tt.hookResponse,
 				}).

internal/controllers/topology/cluster/conditions.go

@@ -59,14 +59,14 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
 
 	// If any of the lifecycle hooks are blocking any part of the reconciliation then topology
 	// is not considered as fully reconciled.
-	if s.HookResponseTracker.EffectiveRequeueAfter() != 0 {
+	if s.HookResponseTracker.EffectiveRetryAfter() != 0 {
 		conditions.Set(
 			cluster,
 			conditions.FalseCondition(
 				clusterv1.TopologyReconciledCondition,
 				clusterv1.TopologyReconciledHookBlockingReason,
 				clusterv1.ConditionSeverityInfo,
-				s.HookResponseTracker.MessageSummary(),
+				s.HookResponseTracker.EffectiveMessage(),
 			),
 		)
 		return nil

internal/controllers/topology/cluster/desired_state.go

@@ -36,6 +36,7 @@ import (
 	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope"
 	tlog "sigs.k8s.io/cluster-api/internal/log"
+	runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
 )
 
 // computeDesiredState computes the desired state of the cluster topology.
@@ -158,7 +159,7 @@ func computeControlPlaneInfrastructureMachineTemplate(_ context.Context, s *scop
 
 // computeControlPlane computes the desired state for the ControlPlane object starting from the
 // corresponding template defined in the blueprint.
-func (r *Reconciler) computeControlPlane(_ context.Context, s *scope.Scope, infrastructureMachineTemplate *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+func (r *Reconciler) computeControlPlane(ctx context.Context, s *scope.Scope, infrastructureMachineTemplate *unstructured.Unstructured) (*unstructured.Unstructured, error) {
 	template := s.Blueprint.ControlPlane.Template
 	templateClonedFromRef := s.Blueprint.ClusterClass.Spec.ControlPlane.Ref
 	cluster := s.Current.Cluster
@@ -222,7 +223,7 @@ func (r *Reconciler) computeControlPlane(_ context.Context, s *scope.Scope, infr
 	}
 
 	// Sets the desired Kubernetes version for the control plane.
-	version, err := r.computeControlPlaneVersion(s)
+	version, err := r.computeControlPlaneVersion(ctx, s)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to compute version of control plane")
 	}
@@ -236,7 +237,7 @@ func (r *Reconciler) computeControlPlane(_ context.Context, s *scope.Scope, infr
 // computeControlPlaneVersion calculates the version of the desired control plane.
 // The version is calculated using the state of the current machine deployments, the current control plane
 // and the version defined in the topology.
-func (r *Reconciler) computeControlPlaneVersion(s *scope.Scope) (string, error) {
+func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Scope) (string, error) {
 	desiredVersion := s.Blueprint.Topology.Version
 	// If we are creating the control plane object (current control plane is nil), use version from topology.
 	if s.Current.ControlPlane == nil || s.Current.ControlPlane.Object == nil {
@@ -325,12 +326,10 @@ func (r *Reconciler) computeControlPlaneVersion(s *scope.Scope) (string, error)
 			ToKubernetesVersion:   desiredVersion,
 		}
 		hookResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{}
-		// TODO: consider adding ctx to the functions above the chain?
-		// Will also be useful to pickup a logger from the context to add some log messages.
-		if err := r.RuntimeClient.CallAllExtensions(context.Background(), runtimehooksv1.BeforeClusterUpgrade, hookRequest, hookResponse); err != nil {
-			return *currentVersion, errors.Wrap(err, "failed to call BeforeClusterUpgrade hook")
+		if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.BeforeClusterUpgrade, hookRequest, hookResponse); err != nil {
+			return "", errors.Wrapf(err, "failed to call %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade))
 		}
-		s.HookResponseTracker.Add("BeforeClusterUpgrade", hookResponse)
+		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