Skip to content

Commit 04d3b9e

Browse files
author
Yuvaraj Kakaraparthi
committed
fix review comments
1 parent 34235b0 commit 04d3b9e

File tree

5 files changed

+158
-8
lines changed

5 files changed

+158
-8
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result
243243
}
244244

245245
// requeueAfter will not be 0 if any of the runtime hooks returns a blocking response.
246-
requeueAfter := s.HookResponseTracker.EffectiveRetryAfter()
246+
requeueAfter := s.HookResponseTracker.AggregateRetryAfter()
247247
if requeueAfter != 0 {
248248
return ctrl.Result{RequeueAfter: requeueAfter}, nil
249249
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,14 @@ func (r *Reconciler) reconcileTopologyReconciledCondition(s *scope.Scope, cluste
5959

6060
// If any of the lifecycle hooks are blocking any part of the reconciliation then topology
6161
// is not considered as fully reconciled.
62-
if s.HookResponseTracker.EffectiveRetryAfter() != 0 {
62+
if s.HookResponseTracker.AggregateRetryAfter() != 0 {
6363
conditions.Set(
6464
cluster,
6565
conditions.FalseCondition(
6666
clusterv1.TopologyReconciledCondition,
6767
clusterv1.TopologyReconciledHookBlockingReason,
6868
clusterv1.ConditionSeverityInfo,
69-
s.HookResponseTracker.EffectiveMessage(),
69+
s.HookResponseTracker.AggregateMessage(),
7070
),
7171
)
7272
return nil

‎internal/controllers/topology/cluster/scope/hookresponsetracker.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ func (h *HookResponseTracker) Add(hook runtimecatalog.Hook, response runtimehook
4343
h.responses[hookName] = response
4444
}
4545

46-
// EffectiveRetryAfter calculates the lowest non-zero retryAfterSeconds time from all the tracked responses.
47-
func (h *HookResponseTracker) EffectiveRetryAfter() time.Duration {
46+
// AggregateRetryAfter calculates the lowest non-zero retryAfterSeconds time from all the tracked responses.
47+
func (h *HookResponseTracker) AggregateRetryAfter() time.Duration {
4848
res := int32(0)
4949
for _, resp := range h.responses {
5050
if retryResponse, ok := resp.(runtimehooksv1.RetryResponseObject); ok {
@@ -54,8 +54,8 @@ func (h *HookResponseTracker) EffectiveRetryAfter() time.Duration {
5454
return time.Duration(res) * time.Second
5555
}
5656

57-
// EffectiveMessage returns a human friendly message about the blocking status of hooks.
58-
func (h *HookResponseTracker) EffectiveMessage() string {
57+
// AggregateMessage returns a human friendly message about the blocking status of hooks.
58+
func (h *HookResponseTracker) AggregateMessage() string {
5959
blockingHooks := []string{}
6060
for hook, resp := range h.responses {
6161
if retryResponse, ok := resp.(runtimehooksv1.RetryResponseObject); ok {
@@ -64,6 +64,9 @@ func (h *HookResponseTracker) EffectiveMessage() string {
6464
}
6565
}
6666
}
67+
if len(blockingHooks) == 0 {
68+
return ""
69+
}
6770
return fmt.Sprintf("hooks %q are blocking", strings.Join(blockingHooks, ","))
6871
}
6972

‎internal/controllers/topology/cluster/scope/hookresponsetracker_test.go

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/*
2+
Copyright 2021 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package scope
18+
19+
import (
20+
"testing"
21+
"time"
22+
23+
. "github.com/onsi/gomega"
24+
25+
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
26+
runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
27+
)
28+
29+
func TestHookResponseTracker_AggregateRetryAfter(t *testing.T) {
30+
nonBlockingBeforeClusterCreateResponse := &runtimehooksv1.BeforeClusterCreateResponse{
31+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
32+
RetryAfterSeconds: int32(0),
33+
CommonResponse: runtimehooksv1.CommonResponse{
34+
Status: runtimehooksv1.ResponseStatusSuccess,
35+
},
36+
},
37+
}
38+
blockingBeforeClusterCreateResponse := &runtimehooksv1.BeforeClusterCreateResponse{
39+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
40+
RetryAfterSeconds: int32(10),
41+
CommonResponse: runtimehooksv1.CommonResponse{
42+
Status: runtimehooksv1.ResponseStatusSuccess,
43+
},
44+
},
45+
}
46+
47+
nonBlockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
48+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
49+
RetryAfterSeconds: int32(0),
50+
CommonResponse: runtimehooksv1.CommonResponse{
51+
Status: runtimehooksv1.ResponseStatusSuccess,
52+
},
53+
},
54+
}
55+
blockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
56+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
57+
RetryAfterSeconds: int32(5),
58+
CommonResponse: runtimehooksv1.CommonResponse{
59+
Status: runtimehooksv1.ResponseStatusSuccess,
60+
},
61+
},
62+
}
63+
64+
t.Run("AggregateRetryAfter should return non zero value if there are any blocking hook responses", func(t *testing.T) {
65+
g := NewWithT(t)
66+
67+
hrt := NewHookResponseTracker()
68+
hrt.Add(runtimehooksv1.BeforeClusterCreate, blockingBeforeClusterCreateResponse)
69+
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, nonBlockingBeforeClusterUpgradeResponse)
70+
71+
g.Expect(hrt.AggregateRetryAfter()).To(Equal(time.Duration(10) * time.Second))
72+
})
73+
t.Run("AggregateRetryAfter should return zero value if there are no blocking hook responses", func(t *testing.T) {
74+
g := NewWithT(t)
75+
76+
hrt := NewHookResponseTracker()
77+
hrt.Add(runtimehooksv1.BeforeClusterCreate, nonBlockingBeforeClusterCreateResponse)
78+
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, nonBlockingBeforeClusterUpgradeResponse)
79+
80+
g.Expect(hrt.AggregateRetryAfter()).To(Equal(time.Duration(0)))
81+
})
82+
t.Run("AggregateRetryAfter should return the lowest non-zero value if there are multiple blocking hook responses", func(t *testing.T) {
83+
g := NewWithT(t)
84+
85+
hrt := NewHookResponseTracker()
86+
hrt.Add(runtimehooksv1.BeforeClusterCreate, blockingBeforeClusterCreateResponse)
87+
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, blockingBeforeClusterUpgradeResponse)
88+
89+
g.Expect(hrt.AggregateRetryAfter()).To(Equal(time.Duration(5) * time.Second))
90+
})
91+
}
92+
93+
func TestHookResponseTracker_AggregateMessage(t *testing.T) {
94+
nonBlockingBeforeClusterCreateResponse := &runtimehooksv1.BeforeClusterCreateResponse{
95+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
96+
RetryAfterSeconds: int32(0),
97+
CommonResponse: runtimehooksv1.CommonResponse{
98+
Status: runtimehooksv1.ResponseStatusSuccess,
99+
},
100+
},
101+
}
102+
blockingBeforeClusterCreateResponse := &runtimehooksv1.BeforeClusterCreateResponse{
103+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
104+
RetryAfterSeconds: int32(10),
105+
CommonResponse: runtimehooksv1.CommonResponse{
106+
Status: runtimehooksv1.ResponseStatusSuccess,
107+
},
108+
},
109+
}
110+
111+
nonBlockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
112+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
113+
RetryAfterSeconds: int32(0),
114+
CommonResponse: runtimehooksv1.CommonResponse{
115+
Status: runtimehooksv1.ResponseStatusSuccess,
116+
},
117+
},
118+
}
119+
blockingBeforeClusterUpgradeResponse := &runtimehooksv1.BeforeClusterUpgradeResponse{
120+
CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
121+
RetryAfterSeconds: int32(5),
122+
CommonResponse: runtimehooksv1.CommonResponse{
123+
Status: runtimehooksv1.ResponseStatusSuccess,
124+
},
125+
},
126+
}
127+
128+
t.Run("AggregateMessage should return a message with the names of all the blocking hooks", func(t *testing.T) {
129+
g := NewWithT(t)
130+
131+
hrt := NewHookResponseTracker()
132+
hrt.Add(runtimehooksv1.BeforeClusterCreate, blockingBeforeClusterCreateResponse)
133+
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, blockingBeforeClusterUpgradeResponse)
134+
135+
g.Expect(hrt.AggregateMessage()).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterCreate)))
136+
g.Expect(hrt.AggregateMessage()).To(ContainSubstring(runtimecatalog.HookName(runtimehooksv1.BeforeClusterUpgrade)))
137+
})
138+
t.Run("AggregateMessage should return empty string if there are no blocking hook responses", func(t *testing.T) {
139+
g := NewWithT(t)
140+
141+
hrt := NewHookResponseTracker()
142+
hrt.Add(runtimehooksv1.BeforeClusterCreate, nonBlockingBeforeClusterCreateResponse)
143+
hrt.Add(runtimehooksv1.BeforeClusterUpgrade, nonBlockingBeforeClusterUpgradeResponse)
144+
145+
g.Expect(hrt.AggregateMessage()).To(Equal(""))
146+
})
147+
}

‎main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
315315

316316
var runtimeClient runtimeclient.Client
317317
if feature.Gates.Enabled(feature.RuntimeSDK) {
318-
// This is the creation of the single RuntimeExtension registry for the controller.
318+
// This is the creation of the runtimeClient for the controllers, embedding a shared catalog and registry instance.
319319
runtimeClient = runtimeclient.New(runtimeclient.Options{
320320
Catalog: catalog,
321321
Registry: runtimeregistry.New(),

0 commit comments

Comments
 (0)