Skip to content

Commit 96f99a8

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent 24ba35a commit 96f99a8

File tree

6 files changed

+92
-29
lines changed

6 files changed

+92
-29
lines changed

‎exp/topology/desiredstate/desired_state.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3232
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
3333
"k8s.io/apimachinery/pkg/runtime/schema"
34-
"k8s.io/apimachinery/pkg/util/intstr"
3534
"k8s.io/klog/v2"
3635
"k8s.io/utils/ptr"
3736
ctrl "sigs.k8s.io/controller-runtime"
@@ -821,10 +820,7 @@ func (g *generator) computeMachineDeployment(ctx context.Context, s *scope.Scope
821820
}
822821
}
823822

824-
var remediationMaxInFlight *intstr.IntOrString
825-
if machineDeploymentClass.HealthCheck.Remediation.MaxInFlight != nil {
826-
remediationMaxInFlight = machineDeploymentClass.HealthCheck.Remediation.MaxInFlight
827-
}
823+
remediationMaxInFlight := machineDeploymentClass.HealthCheck.Remediation.MaxInFlight
828824
if machineDeploymentTopology.HealthCheck.Remediation.MaxInFlight != nil {
829825
remediationMaxInFlight = machineDeploymentTopology.HealthCheck.Remediation.MaxInFlight
830826
}

‎exp/topology/scope/blueprint_test.go

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
5757
want: false,
5858
},
5959
{
60-
name: "should return true if MachineHealthCheck if defined in ClusterClass, not defined in cluster topology and enable is not set",
60+
name: "should return true if MachineHealthCheck is defined in ClusterClass, not defined in cluster topology and enable is not set",
6161
blueprint: &ClusterBlueprint{
6262
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
6363
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
@@ -80,11 +80,21 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
8080
want: true,
8181
},
8282
{
83-
name: "should return false if MachineHealthCheck if defined in ClusterClass, not defined in cluster topology and enable is false",
83+
name: "should return false if MachineHealthCheck is defined in ClusterClass, not defined in cluster topology and enable is false",
8484
blueprint: &ClusterBlueprint{
8585
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
8686
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
87-
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{}).
87+
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{
88+
Checks: clusterv1.ControlPlaneClassHealthCheckChecks{
89+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
90+
{
91+
Type: corev1.NodeReady,
92+
Status: corev1.ConditionUnknown,
93+
TimeoutSeconds: 5 * 60,
94+
},
95+
},
96+
},
97+
}).
8898
Build(),
8999
Topology: builder.ClusterTopology().
90100
WithClass("cluster-class").
@@ -96,7 +106,7 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
96106
want: false,
97107
},
98108
{
99-
name: "should return true if MachineHealthCheck if defined in ClusterClass, not defined in cluster topology and enable is true",
109+
name: "should return true if MachineHealthCheck is defined in ClusterClass, not defined in cluster topology and enable is true",
100110
blueprint: &ClusterBlueprint{
101111
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
102112
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
@@ -122,7 +132,7 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
122132
want: true,
123133
},
124134
{
125-
name: "should return true if MachineHealthCheck if defined in cluster topology, not defined in ClusterClass and enable is not set",
135+
name: "should return true if MachineHealthCheck is defined in cluster topology, not defined in ClusterClass and enable is not set",
126136
blueprint: &ClusterBlueprint{
127137
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
128138
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
@@ -145,7 +155,7 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
145155
want: true,
146156
},
147157
{
148-
name: "should return false if MachineHealthCheck if defined in cluster topology, not defined in ClusterClass and enable is false",
158+
name: "should return false if MachineHealthCheck is defined in cluster topology, not defined in ClusterClass and enable is false",
149159
blueprint: &ClusterBlueprint{
150160
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
151161
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
@@ -169,7 +179,7 @@ func TestIsControlPlaneMachineHealthCheckEnabled(t *testing.T) {
169179
want: false,
170180
},
171181
{
172-
name: "should return true if MachineHealthCheck if defined in cluster topology, not defined in ClusterClass and enable is true",
182+
name: "should return true if MachineHealthCheck is defined in cluster topology, not defined in ClusterClass and enable is true",
173183
blueprint: &ClusterBlueprint{
174184
ClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "cluster-class").
175185
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
@@ -350,7 +360,17 @@ func TestIsMachineDeploymentMachineHealthCheckEnabled(t *testing.T) {
350360
blueprint: &ClusterBlueprint{
351361
MachineDeployments: map[string]*MachineDeploymentBlueprint{
352362
"worker-class": {
353-
HealthCheck: clusterv1.MachineDeploymentClassHealthCheck{},
363+
HealthCheck: clusterv1.MachineDeploymentClassHealthCheck{
364+
Checks: clusterv1.MachineDeploymentClassHealthCheckChecks{
365+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
366+
{
367+
Type: corev1.NodeReady,
368+
Status: corev1.ConditionUnknown,
369+
TimeoutSeconds: 5 * 60,
370+
},
371+
},
372+
},
373+
},
354374
},
355375
},
356376
},

‎internal/webhooks/cluster.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,7 @@ func validateMachineHealthChecks(cluster *clusterv1.Cluster, clusterClass *clust
655655
if clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
656656
allErrs = append(allErrs, field.Forbidden(
657657
fldPath,
658-
"can be set only if spec.controlPlane.machineInfrastructure is set in ClusterClass",
658+
"can be only set if spec.controlPlane.machineInfrastructure is set in ClusterClass",
659659
))
660660
}
661661
allErrs = append(allErrs, validateMachineHealthCheckNodeStartupTimeoutSeconds(fldPath, cluster.Spec.Topology.ControlPlane.HealthCheck.Checks.NodeStartupTimeoutSeconds)...)

‎internal/webhooks/cluster_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2267,7 +2267,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
22672267
WithControlPlaneReplicas(3).
22682268
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneTopologyHealthCheck{
22692269
Checks: clusterv1.ControlPlaneTopologyHealthCheckChecks{
2270-
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{},
2270+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{},
2271+
NodeStartupTimeoutSeconds: ptr.To(int32(30)),
22712272
},
22722273
}).
22732274
Build()).
@@ -2398,7 +2399,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
23982399
WithClass("worker-class").
23992400
WithMachineHealthCheck(clusterv1.MachineDeploymentTopologyHealthCheck{
24002401
Checks: clusterv1.MachineDeploymentTopologyHealthCheckChecks{
2401-
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{},
2402+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{},
2403+
NodeStartupTimeoutSeconds: ptr.To(int32(30)),
24022404
},
24032405
}).
24042406
Build(),

‎internal/webhooks/clusterclass.go

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -371,21 +371,26 @@ func validateMachineHealthCheckClasses(clusterClass *clusterv1.ClusterClass) fie
371371
var allErrs field.ErrorList
372372

373373
// Validate ControlPlane MachineHealthCheck if defined.
374-
fldPath := field.NewPath("spec", "controlPlane", "healthCheck")
374+
if clusterClass.Spec.ControlPlane.HealthCheck.IsDefined() {
375+
fldPath := field.NewPath("spec", "controlPlane", "healthCheck")
375376

376-
allErrs = append(allErrs, validateMachineHealthCheckNodeStartupTimeoutSeconds(fldPath, clusterClass.Spec.ControlPlane.HealthCheck.Checks.NodeStartupTimeoutSeconds)...)
377-
allErrs = append(allErrs, validateMachineHealthCheckUnhealthyLessThanOrEqualTo(fldPath, clusterClass.Spec.ControlPlane.HealthCheck.Remediation.TriggerIf.UnhealthyLessThanOrEqualTo)...)
377+
allErrs = append(allErrs, validateMachineHealthCheckNodeStartupTimeoutSeconds(fldPath, clusterClass.Spec.ControlPlane.HealthCheck.Checks.NodeStartupTimeoutSeconds)...)
378+
allErrs = append(allErrs, validateMachineHealthCheckUnhealthyLessThanOrEqualTo(fldPath, clusterClass.Spec.ControlPlane.HealthCheck.Remediation.TriggerIf.UnhealthyLessThanOrEqualTo)...)
378379

379-
// Ensure ControlPlane does not define a MachineHealthCheck if it does not define MachineInfrastructure.
380-
if clusterClass.Spec.ControlPlane.HealthCheck.IsDefined() && clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
381-
allErrs = append(allErrs, field.Forbidden(
382-
fldPath,
383-
"can be set only if spec.controlPlane.machineInfrastructure is set",
384-
))
380+
// Ensure ControlPlane does not define a MachineHealthCheck if it does not define MachineInfrastructure.
381+
if clusterClass.Spec.ControlPlane.MachineInfrastructure == nil {
382+
allErrs = append(allErrs, field.Forbidden(
383+
fldPath,
384+
"can be only set if spec.controlPlane.machineInfrastructure is set",
385+
))
386+
}
385387
}
386388

387389
// Validate MachineDeployment MachineHealthChecks.
388390
for _, md := range clusterClass.Spec.Workers.MachineDeployments {
391+
if !md.HealthCheck.IsDefined() {
392+
continue
393+
}
389394
fldPath := field.NewPath("spec", "workers", "machineDeployments").Key(md.Class).Child("healthCheck")
390395

391396
allErrs = append(allErrs, validateMachineHealthCheckNodeStartupTimeoutSeconds(fldPath, md.HealthCheck.Checks.NodeStartupTimeoutSeconds)...)

‎internal/webhooks/clusterclass_test.go

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2132,7 +2132,17 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
21322132
WithControlPlaneTemplate(
21332133
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
21342134
Build()).
2135-
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{}).
2135+
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{
2136+
Checks: clusterv1.ControlPlaneClassHealthCheckChecks{
2137+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
2138+
{
2139+
Type: corev1.NodeReady,
2140+
Status: corev1.ConditionUnknown,
2141+
TimeoutSeconds: 5 * 60,
2142+
},
2143+
},
2144+
},
2145+
}).
21362146
Build(),
21372147
newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass1").
21382148
WithInfrastructureClusterTemplate(
@@ -2170,7 +2180,17 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
21702180
WithControlPlaneTemplate(
21712181
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
21722182
Build()).
2173-
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{}).
2183+
WithControlPlaneMachineHealthCheck(clusterv1.ControlPlaneClassHealthCheck{
2184+
Checks: clusterv1.ControlPlaneClassHealthCheckChecks{
2185+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
2186+
{
2187+
Type: corev1.NodeReady,
2188+
Status: corev1.ConditionUnknown,
2189+
TimeoutSeconds: 5 * 60,
2190+
},
2191+
},
2192+
},
2193+
}).
21742194
Build(),
21752195
newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass1").
21762196
WithInfrastructureClusterTemplate(
@@ -2263,7 +2283,17 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
22632283
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
22642284
WithBootstrapTemplate(
22652285
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
2266-
WithMachineHealthCheckClass(clusterv1.MachineDeploymentClassHealthCheck{}).
2286+
WithMachineHealthCheckClass(clusterv1.MachineDeploymentClassHealthCheck{
2287+
Checks: clusterv1.MachineDeploymentClassHealthCheckChecks{
2288+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
2289+
{
2290+
Type: corev1.NodeReady,
2291+
Status: corev1.ConditionUnknown,
2292+
TimeoutSeconds: 5 * 60,
2293+
},
2294+
},
2295+
},
2296+
}).
22672297
Build(),
22682298
).
22692299
Build(),
@@ -2320,7 +2350,17 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
23202350
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
23212351
WithBootstrapTemplate(
23222352
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
2323-
WithMachineHealthCheckClass(clusterv1.MachineDeploymentClassHealthCheck{}).
2353+
WithMachineHealthCheckClass(clusterv1.MachineDeploymentClassHealthCheck{
2354+
Checks: clusterv1.MachineDeploymentClassHealthCheckChecks{
2355+
UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
2356+
{
2357+
Type: corev1.NodeReady,
2358+
Status: corev1.ConditionUnknown,
2359+
TimeoutSeconds: 5 * 60,
2360+
},
2361+
},
2362+
},
2363+
}).
23242364
Build(),
23252365
).
23262366
Build(),

0 commit comments

Comments
 (0)