Skip to content

Commit 8a124da

Browse files
fabriziopandinifabriziopandini
committed
Address comments
1 parent 67cc286 commit 8a124da

File tree

13 files changed

+138
-139
lines changed

13 files changed

+138
-139
lines changed

‎api/core/v1beta2/cluster_types.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ const (
8080
// failing due to an error.
8181
ClusterTopologyReconciledFailedReason = "ReconcileFailed"
8282

83+
// ClusterTopologyReconciledClusterCreatingReason documents reconciliation of a Cluster topology
84+
// not yet created because the BeforeClusterCreate hook is blocking.
85+
ClusterTopologyReconciledClusterCreatingReason = "ClusterCreating"
86+
8387
// ClusterTopologyReconciledControlPlaneUpgradePendingReason documents reconciliation of a Cluster topology
8488
// not yet completed because Control Plane is not yet updated to match the desired topology spec.
8589
// Deprecated: please use ClusterUpgrading instead.
@@ -113,7 +117,6 @@ const (
113117

114118
// ClusterTopologyReconciledMachinePoolsUpgradeDeferredReason documents reconciliation of a Cluster topology
115119
// not yet completed because the upgrade for at least one of the MachinePools has been deferred.
116-
// Deprecated: please use ClusterUpgrading instead.
117120
ClusterTopologyReconciledMachinePoolsUpgradeDeferredReason = "MachinePoolsUpgradeDeferred"
118121

119122
// ClusterTopologyReconciledHookBlockingReason documents reconciliation of a Cluster topology
@@ -124,7 +127,6 @@ const (
124127
// ClusterTopologyReconciledClusterUpgradingReason documents reconciliation of a Cluster topology
125128
// not yet completed because a cluster upgrade is still in progress.
126129
ClusterTopologyReconciledClusterUpgradingReason = "ClusterUpgrading"
127-
128130
// ClusterTopologyReconciledClusterClassNotReconciledReason documents reconciliation of a Cluster topology not
129131
// yet completed because the ClusterClass has not reconciled yet. If this condition persists there may be an issue
130132
// with the ClusterClass surfaced in the ClusterClass status or controller logs.

‎api/core/v1beta2/v1beta1_condition_consts.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,10 @@ const (
300300
// failing due to an error.
301301
TopologyReconcileFailedV1Beta1Reason = "TopologyReconcileFailed"
302302

303+
// TopologyReconciledClusterCreatingV1Beta1Reason documents reconciliation of a Cluster topology
304+
// not yet created because the BeforeClusterCreate hook is blocking.
305+
TopologyReconciledClusterCreatingV1Beta1Reason = "ClusterCreating"
306+
303307
// TopologyReconciledControlPlaneUpgradePendingV1Beta1Reason (Severity=Info) documents reconciliation of a Cluster topology
304308
// not yet completed because Control Plane is not yet updated to match the desired topology spec.
305309
// Deprecated: please use ClusterUpgrading instead.
@@ -340,9 +344,9 @@ const (
340344
// Deprecated: please use ClusterUpgrading instead.
341345
TopologyReconciledHookBlockingV1Beta1Reason = "LifecycleHookBlocking"
342346

343-
// ClusterTopologyReconciledClusterUpgradingV1Beta1Reason documents reconciliation of a Cluster topology
347+
// TopologyReconciledClusterUpgradingV1Beta1Reason documents reconciliation of a Cluster topology
344348
// not yet completed because a cluster upgrade is still in progress.
345-
ClusterTopologyReconciledClusterUpgradingV1Beta1Reason = "ClusterUpgrading"
349+
TopologyReconciledClusterUpgradingV1Beta1Reason = "ClusterUpgrading"
346350

347351
// TopologyReconciledClusterClassNotReconciledV1Beta1Reason (Severity=Info) documents reconciliation of a Cluster topology not
348352
// yet completed because the ClusterClass has not reconciled yet. If this condition persists there may be an issue

‎exp/topology/desiredstate/desired_state_test.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,10 +1064,8 @@ func TestComputeControlPlaneVersion(t *testing.T) {
10641064
_ = runtimehooksv1.AddToCatalog(catalog)
10651065
beforeClusterUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.BeforeClusterUpgrade)
10661066
beforeControlPlaneUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.BeforeControlPlaneUpgrade)
1067-
afterControlPlaneUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterControlPlaneUpgrade)
10681067
beforeWorkersUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.BeforeWorkersUpgrade)
10691068
afterWorkersUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterWorkersUpgrade)
1070-
afterClusterUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterClusterUpgrade)
10711069

10721070
nonBlockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
10731071
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
@@ -1698,10 +1696,8 @@ func TestComputeControlPlaneVersion(t *testing.T) {
16981696
WithGetAllExtensionResponses(map[runtimecatalog.GroupVersionHook][]string{
16991697
beforeClusterUpgradeGVH: {"foo"},
17001698
beforeControlPlaneUpgradeGVH: {"foo"},
1701-
afterControlPlaneUpgradeGVH: {"foo"},
17021699
beforeWorkersUpgradeGVH: {"foo"},
17031700
afterWorkersUpgradeGVH: {"foo"},
1704-
afterClusterUpgradeGVH: {"foo"},
17051701
}).
17061702
WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
17071703
beforeClusterUpgradeGVH: tt.beforeClusterUpgradeResponse,

‎exp/topology/desiredstate/lifecycle_hooks.go‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,6 @@ func (g *generator) callBeforeClusterUpgradeHook(ctx context.Context, s *scope.S
4040
log := ctrl.LoggerFrom(ctx)
4141

4242
if !hooks.IsPending(runtimehooksv1.AfterClusterUpgrade, s.Current.Cluster) {
43-
// Return quickly if the hook is not defined.
44-
extensionHandlers, err := g.RuntimeClient.GetAllExtensions(ctx, runtimehooksv1.BeforeClusterUpgrade, s.Current.Cluster)
45-
if err != nil {
46-
return false, err
47-
}
48-
if len(extensionHandlers) == 0 {
49-
return true, nil
50-
}
51-
5243
var hookAnnotations []string
5344
for key := range s.Current.Cluster.Annotations {
5445
if strings.HasPrefix(key, clusterv1.BeforeClusterUpgradeHookAnnotationPrefix) {
@@ -57,9 +48,9 @@ func (g *generator) callBeforeClusterUpgradeHook(ctx context.Context, s *scope.S
5748
}
5849
if len(hookAnnotations) > 0 {
5950
slices.Sort(hookAnnotations)
60-
message := fmt.Sprintf("annotations [%s] are set", strings.Join(hookAnnotations, ", "))
51+
message := fmt.Sprintf("annotations %s are set", strings.Join(hookAnnotations, ", "))
6152
if len(hookAnnotations) == 1 {
62-
message = fmt.Sprintf("annotation [%s] is set", strings.Join(hookAnnotations, ", "))
53+
message = fmt.Sprintf("annotation %s is set", strings.Join(hookAnnotations, ", "))
6354
}
6455
// Add the hook with a response to the tracker so we can later update the condition.
6556
s.HookResponseTracker.Add(runtimehooksv1.BeforeClusterUpgrade, &runtimehooksv1.BeforeClusterUpgradeResponse{
@@ -81,6 +72,15 @@ func (g *generator) callBeforeClusterUpgradeHook(ctx context.Context, s *scope.S
8172
return false, nil
8273
}
8374

75+
// Return quickly if the hook is not defined.
76+
extensionHandlers, err := g.RuntimeClient.GetAllExtensions(ctx, runtimehooksv1.BeforeClusterUpgrade, s.Current.Cluster)
77+
if err != nil {
78+
return false, err
79+
}
80+
if len(extensionHandlers) == 0 {
81+
return true, nil
82+
}
83+
8484
v1beta1Cluster := &clusterv1beta1.Cluster{}
8585
// DeepCopy cluster because ConvertFrom has side effects like adding the conversion annotation.
8686
if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster.DeepCopy()); err != nil {
@@ -110,7 +110,7 @@ func (g *generator) callBeforeClusterUpgradeHook(ctx context.Context, s *scope.S
110110
return false, nil
111111
}
112112

113-
log.Info(fmt.Sprintf("Control plane upgrade from version %s to version %s unblocked by %s hook", hookRequest.FromKubernetesVersion, hookRequest.ToKubernetesVersion, runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade)),
113+
log.Info(fmt.Sprintf("Cluster upgrade from version %s to version %s unblocked by %s hook", hookRequest.FromKubernetesVersion, hookRequest.ToKubernetesVersion, runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade)),
114114
"ControlPlaneUpgrades", hookRequest.ControlPlaneUpgrades,
115115
"WorkersUpgrades", hookRequest.WorkersUpgrades,
116116
)
@@ -214,7 +214,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco
214214
s.HookResponseTracker.Add(runtimehooksv1.AfterControlPlaneUpgrade, hookResponse)
215215

216216
if hookResponse.RetryAfterSeconds != 0 {
217-
log.Info(fmt.Sprintf("Cluster Upgrade to version %s is blocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade)),
217+
log.Info(fmt.Sprintf("Control plane upgrade to version %s completed but next steps are blocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade)),
218218
"ControlPlaneUpgrades", hookRequest.ControlPlaneUpgrades,
219219
"WorkersUpgrades", hookRequest.WorkersUpgrades,
220220
)
@@ -224,7 +224,7 @@ func (g *generator) callAfterControlPlaneUpgradeHook(ctx context.Context, s *sco
224224
return false, err
225225
}
226226

227-
log.Info(fmt.Sprintf("Cluster upgrade to version %s unblocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade)),
227+
log.Info(fmt.Sprintf("Control plane upgrade to version %s and %s hook completed, next steps unblocked", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterControlPlaneUpgrade)),
228228
"ControlPlaneUpgrades", hookRequest.ControlPlaneUpgrades,
229229
"WorkersUpgrades", hookRequest.WorkersUpgrades,
230230
)
@@ -338,7 +338,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc
338338
s.HookResponseTracker.Add(runtimehooksv1.AfterWorkersUpgrade, hookResponse)
339339

340340
if hookResponse.RetryAfterSeconds != 0 {
341-
log.Info(fmt.Sprintf("Cluster upgrade to version %s is blocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterWorkersUpgrade)),
341+
log.Info(fmt.Sprintf("Workers upgrade to version %s completed but next steps are blocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterWorkersUpgrade)),
342342
"ControlPlaneUpgrades", hookRequest.ControlPlaneUpgrades,
343343
"WorkersUpgrades", hookRequest.WorkersUpgrades,
344344
)
@@ -348,7 +348,7 @@ func (g *generator) callAfterWorkersUpgradeHook(ctx context.Context, s *scope.Sc
348348
return false, err
349349
}
350350

351-
log.Info(fmt.Sprintf("Cluster upgrade to version %s unblocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterWorkersUpgrade)),
351+
log.Info(fmt.Sprintf("Workers upgrade to version %s and %s hook completed, next steps unblocked", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterWorkersUpgrade)),
352352
"ControlPlaneUpgrades", hookRequest.ControlPlaneUpgrades,
353353
"WorkersUpgrades", hookRequest.WorkersUpgrades,
354354
)

‎exp/topology/desiredstate/lifecycle_hooks_test.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ func TestComputeControlPlaneVersion_LifecycleHooksSequences(t *testing.T) {
7777
afterControlPlaneUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterControlPlaneUpgrade)
7878
beforeWorkersUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.BeforeWorkersUpgrade)
7979
afterWorkersUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterWorkersUpgrade)
80-
afterClusterUpgradeGVH, _ := catalog.GroupVersionHook(runtimehooksv1.AfterClusterUpgrade)
8180

8281
blockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
8382
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
@@ -1231,7 +1230,6 @@ func TestComputeControlPlaneVersion_LifecycleHooksSequences(t *testing.T) {
12311230
afterControlPlaneUpgradeGVH: {"foo"},
12321231
beforeWorkersUpgradeGVH: {"foo"},
12331232
afterWorkersUpgradeGVH: {"foo"},
1234-
afterClusterUpgradeGVH: {"foo"},
12351233
}).
12361234
WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
12371235
beforeClusterUpgradeGVH: tt.beforeClusterUpgradeResponse,

‎exp/topology/scope/hookresponsetracker.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func (h *HookResponseTracker) AggregateRetryAfter() time.Duration {
8181
}
8282

8383
// AggregateMessage returns a human friendly message about the blocking status of hooks.
84-
func (h *HookResponseTracker) AggregateMessage() string {
84+
func (h *HookResponseTracker) AggregateMessage(action string) string {
8585
blockingHooks := map[string]string{}
8686
for hook, resp := range h.responses {
8787
if retryResponse, ok := resp.(runtimehooksv1.RetryResponseObject); ok {
@@ -102,5 +102,5 @@ func (h *HookResponseTracker) AggregateMessage() string {
102102
hookAndMessages = append(hookAndMessages, fmt.Sprintf("%s: %s", hook, message))
103103
}
104104
}
105-
return fmt.Sprintf("Following hooks are blocking upgrade progress: %s", strings.Join(hookAndMessages, "; "))
105+
return fmt.Sprintf("Following hooks are blocking %s: %s", action, strings.Join(hookAndMessages, "; "))
106106
}

‎exp/topology/scope/hookresponsetracker_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,8 @@ func TestHookResponseTracker_AggregateMessage(t *testing.T) {
132132
hrt.Add(runtimehooksv1.BeforeClusterCreate, blockingBeforeClusterCreateResponse)
133133
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, blockingBeforeClusterUpgradeResponse)
134134

135-
g.Expect(hrt.AggregateMessage()).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterCreate)))
136-
g.Expect(hrt.AggregateMessage()).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade)))
135+
g.Expect(hrt.AggregateMessage("upgrade")).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterCreate)))
136+
g.Expect(hrt.AggregateMessage("upgrade")).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade)))
137137
})
138138
t.Run("AggregateMessage should return empty string if there are no blocking hook responses", func(t *testing.T) {
139139
g := NewWithT(t)
@@ -142,7 +142,7 @@ func TestHookResponseTracker_AggregateMessage(t *testing.T) {
142142
hrt.Add(runtimehooksv1.BeforeClusterCreate, nonBlockingBeforeClusterCreateResponse)
143143
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, nonBlockingBeforeClusterUpgradeResponse)
144144

145-
g.Expect(hrt.AggregateMessage()).To(Equal(""))
145+
g.Expect(hrt.AggregateMessage("upgrade")).To(Equal(""))
146146
})
147147
}
148148

‎internal/controllers/cluster/cluster_controller_status_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2846,7 +2846,7 @@ func TestSetAvailableCondition(t *testing.T) {
28462846
{
28472847
Type: clusterv1.ClusterTopologyReconciledCondition,
28482848
Status: metav1.ConditionFalse,
2849-
Reason: clusterv1.ClusterTopologyReconciledClusterUpgradingV1Beta1Reason,
2849+
Reason: clusterv1.ClusterTopologyReconciledClusterUpgradingReason,
28502850
Message: "Cluster is upgrading to v1.22.0\n" +
28512851
" * MachineDeployment md1 upgrading to version v1.22.0",
28522852
},
@@ -2907,7 +2907,7 @@ func TestSetAvailableCondition(t *testing.T) {
29072907
{
29082908
Type: clusterv1.ClusterTopologyReconciledCondition,
29092909
Status: metav1.ConditionFalse,
2910-
Reason: clusterv1.ClusterTopologyReconciledClusterUpgradingV1Beta1Reason,
2910+
Reason: clusterv1.ClusterTopologyReconciledClusterUpgradingReason,
29112911
Message: "Cluster is upgrading to v1.22.0\n" +
29122912
" * MachineDeployment md1 upgrading to version v1.22.0",
29132913
},

‎internal/controllers/topology/cluster/cluster_controller.go‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -440,10 +440,7 @@ func (r *Reconciler) callBeforeClusterCreateHook(ctx context.Context, s *scope.S
440440
if err != nil {
441441
return ctrl.Result{}, err
442442
}
443-
if err != nil || len(extensionHandlers) == 0 {
444-
if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.BeforeClusterCreate); err != nil {
445-
return ctrl.Result{}, err
446-
}
443+
if len(extensionHandlers) == 0 {
447444
return ctrl.Result{}, nil
448445
}
449446

@@ -546,6 +543,18 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
546543
log := ctrl.LoggerFrom(ctx)
547544
if feature.Gates.Enabled(feature.RuntimeSDK) {
548545
if !hooks.IsOkToDelete(cluster) {
546+
// Return quickly if the hook is not defined.
547+
extensionHandlers, err := r.RuntimeClient.GetAllExtensions(ctx, runtimehooksv1.BeforeClusterDelete, s.Current.Cluster)
548+
if err != nil {
549+
return ctrl.Result{}, err
550+
}
551+
if len(extensionHandlers) == 0 {
552+
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
553+
return ctrl.Result{}, err
554+
}
555+
return ctrl.Result{}, nil
556+
}
557+
549558
v1beta1Cluster := &clusterv1beta1.Cluster{}
550559
// DeepCopy cluster because ConvertFrom has side effects like adding the conversion annotation.
551560
if err := v1beta1Cluster.ConvertFrom(cluster.DeepCopy()); err != nil {
@@ -560,7 +569,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
560569
return ctrl.Result{}, err
561570
}
562571
// Add the response to the tracker so we can later update condition or requeue when required.
563-
s.HookResponseTracker.Add(runtimehooksv1.BeforeClusterUpgrade, hookResponse)
572+
s.HookResponseTracker.Add(runtimehooksv1.BeforeClusterDelete, hookResponse)
564573

565574
if hookResponse.RetryAfterSeconds != 0 {
566575
log.Info(fmt.Sprintf("Cluster deletion is blocked by %q hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete)))
@@ -571,6 +580,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
571580
if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
572581
return ctrl.Result{}, err
573582
}
583+
log.Info(fmt.Sprintf("Cluster deletion js unblocked by %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete)))
574584
}
575585
}
576586
return ctrl.Result{}, nil

‎internal/controllers/topology/cluster/cluster_controller_test.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -581,7 +581,10 @@ func TestClusterReconciler_reconcileDelete(t *testing.T) {
581581
tt.cluster.Annotations[conversion.DataAnnotation] = "should be cleaned up"
582582

583583
fakeClient := fake.NewClientBuilder().WithObjects(tt.cluster).Build()
584-
fakeRuntimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
584+
fakeRuntimeClient := (fakeruntimeclient.NewRuntimeClientBuilder().
585+
WithGetAllExtensionResponses(map[runtimecatalog.GroupVersionHook][]string{
586+
beforeClusterDeleteGVH: {"foo"},
587+
})).
585588
WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{
586589
beforeClusterDeleteGVH: tt.hookResponse,
587590
}).
@@ -609,7 +612,7 @@ func TestClusterReconciler_reconcileDelete(t *testing.T) {
609612
g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.BeforeClusterDelete)).To(Equal(1), "Expected hook to be called once")
610613
if !tt.wantOkToDelete {
611614
g.Expect(s.HookResponseTracker.AggregateRetryAfter()).ToNot(BeZero())
612-
g.Expect(s.HookResponseTracker.AggregateMessage()).To(Equal("Following hooks are blocking upgrade progress: BeforeClusterUpgrade: hook is blocking"))
615+
g.Expect(s.HookResponseTracker.AggregateMessage("delete")).To(Equal("Following hooks are blocking delete: BeforeClusterDelete: hook is blocking"))
613616
}
614617
} else {
615618
g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.BeforeClusterDelete)).To(Equal(0), "Did not expect hook to be called")

0 commit comments

Comments
 (0)