Skip to content

Commit 52affec

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1642 from srm09/automated-cherry-pick-of-#1641-release-1.4
🐛 Automated cherry pick of #1641: Adds default resource pool logic
2 parents b9533f5 + c2b4416 commit 52affec

File tree

4 files changed

+167
-25
lines changed

4 files changed

+167
-25
lines changed

‎controllers/clustermodule_reconciler.go‎

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@ limitations under the License.
1717
package controllers
1818

1919
import (
20+
"fmt"
21+
"strings"
22+
2023
"github.com/pkg/errors"
2124
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
22-
kerrors "k8s.io/apimachinery/pkg/util/errors"
2325
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2426
controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
2527
"sigs.k8s.io/cluster-api/util"
@@ -70,9 +72,7 @@ func (r Reconciler) Reconcile(ctx *context.ClusterContext) (reconcile.Result, er
7072
return reconcile.Result{}, err
7173
}
7274

73-
var errList []error
7475
clusterModuleSpecs := []infrav1.ClusterModule{}
75-
7676
for _, mod := range ctx.VSphereCluster.Spec.ClusterModules {
7777
curr := mod.TargetObjectName
7878
if mod.ControlPlane {
@@ -84,14 +84,14 @@ func (r Reconciler) Reconcile(ctx *context.ClusterContext) (reconcile.Result, er
8484
if err := r.ClusterModuleService.Remove(ctx, mod.ModuleUUID); err != nil {
8585
ctx.Logger.Error(err, "failed to delete cluster module for object",
8686
"name", mod.TargetObjectName, "moduleUUID", mod.ModuleUUID)
87-
errList = append(errList, err)
8887
}
8988
delete(objectMap, curr)
9089
} else {
9190
// verify the cluster module
9291
exists, err := r.ClusterModuleService.DoesExist(ctx, obj, mod.ModuleUUID)
9392
if err != nil {
94-
errList = append(errList, err)
93+
ctx.Logger.Error(err, "failed to verify cluster module for object",
94+
"name", mod.TargetObjectName, "moduleUUID", mod.ModuleUUID)
9595
}
9696
// append the module and object info to the VSphereCluster object
9797
// and remove it from the object map since no new cluster module
@@ -110,17 +110,14 @@ func (r Reconciler) Reconcile(ctx *context.ClusterContext) (reconcile.Result, er
110110
}
111111
}
112112
}
113-
if len(errList) > 0 {
114-
ctx.Logger.Error(kerrors.NewAggregate(errList), "errors reconciling cluster modules for cluster",
115-
"namespace", ctx.VSphereCluster.Namespace, "name", ctx.VSphereCluster.Name)
116-
}
117113

118-
errList = []error{}
114+
modErrs := []clusterModError{}
119115
for _, obj := range objectMap {
120116
moduleUUID, err := r.ClusterModuleService.Create(ctx, obj)
121117
if err != nil {
122118
ctx.Logger.Error(err, "failed to create cluster module for target object", "name", obj.GetName())
123-
errList = append(errList, err)
119+
modErrs = append(modErrs, clusterModError{obj.GetName(), err})
120+
continue
124121
}
125122
clusterModuleSpecs = append(clusterModuleSpecs, infrav1.ClusterModule{
126123
ControlPlane: obj.IsControlPlane(),
@@ -130,16 +127,19 @@ func (r Reconciler) Reconcile(ctx *context.ClusterContext) (reconcile.Result, er
130127
}
131128
ctx.VSphereCluster.Spec.ClusterModules = clusterModuleSpecs
132129

133-
if len(errList) > 0 {
134-
err = kerrors.NewAggregate(errList)
135-
ctx.Logger.Error(err, "errors reconciling cluster modules for cluster",
136-
"namespace", ctx.VSphereCluster.Namespace, "name", ctx.VSphereCluster.Name)
137-
}
138-
139130
switch {
140-
case err != nil:
141-
conditions.MarkFalse(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition, infrav1.ClusterModuleSetupFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
142-
case err == nil && len(clusterModuleSpecs) > 0:
131+
case len(modErrs) > 0:
132+
incompatibleOwnerErrs := incompatibleOwnerErrors(modErrs)
133+
// if cluster module creation is not possible due to incompatibility,
134+
// cluster creation should succeed with a warning condition
135+
if len(incompatibleOwnerErrs) > 0 && len(incompatibleOwnerErrs) == len(modErrs) {
136+
err = nil
137+
} else {
138+
err = errors.New(generateClusterModuleErrorMessage(modErrs))
139+
}
140+
conditions.MarkFalse(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition, infrav1.ClusterModuleSetupFailedReason,
141+
clusterv1.ConditionSeverityWarning, generateClusterModuleErrorMessage(modErrs))
142+
case len(modErrs) == 0 && len(clusterModuleSpecs) > 0:
143143
conditions.MarkTrue(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)
144144
default:
145145
conditions.Delete(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)
@@ -253,3 +253,29 @@ func (r Reconciler) fetchMachineOwnerObjects(ctx *context.ClusterContext) (map[s
253253
func appendKCPKey(name string) string {
254254
return "kcp" + name
255255
}
256+
257+
func incompatibleOwnerErrors(errList []clusterModError) []clusterModError {
258+
toReport := []clusterModError{}
259+
for _, e := range errList {
260+
if clustermodule.IsIncompatibleOwnerError(e.err) {
261+
toReport = append(toReport, e)
262+
}
263+
}
264+
return toReport
265+
}
266+
267+
type clusterModError struct {
268+
name string
269+
err error
270+
}
271+
272+
func generateClusterModuleErrorMessage(errList []clusterModError) string {
273+
sb := strings.Builder{}
274+
sb.WriteString("failed to create cluster modules for: ")
275+
276+
for _, e := range errList {
277+
sb.WriteString(fmt.Sprintf("%s %s, ", e.name, e.err.Error()))
278+
}
279+
msg := sb.String()
280+
return msg[:len(msg)-2]
281+
}

‎controllers/clustermodule_reconciler_test.go‎

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,12 @@ import (
2222

2323
"github.com/google/uuid"
2424
"github.com/onsi/gomega"
25+
"github.com/pkg/errors"
2526
"github.com/stretchr/testify/mock"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2728
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2829
controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
30+
"sigs.k8s.io/cluster-api/util/conditions"
2931
"sigs.k8s.io/controller-runtime/pkg/client"
3032

3133
infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
@@ -42,6 +44,7 @@ func TestReconciler_Reconcile(t *testing.T) {
4244

4345
tests := []struct {
4446
name string
47+
haveError bool
4548
clusterModules []infrav1.ClusterModule
4649
beforeFn func(object client.Object)
4750
setupMocks func(*cmodfake.CMService)
@@ -89,6 +92,60 @@ func TestReconciler_Reconcile(t *testing.T) {
8992
g.Expect(moduleUUIDs).To(gomega.ConsistOf(kcpUUID, mdUUID))
9093
},
9194
},
95+
{
96+
name: "when cluster module creation is called for a resource pool owned by non compute cluster resource",
97+
clusterModules: []infrav1.ClusterModule{},
98+
setupMocks: func(svc *cmodfake.CMService) {
99+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(kcp)).Return("", clustermodule.NewIncompatibleOwnerError("foo-123"))
100+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(md)).Return(mdUUID, nil)
101+
},
102+
customAssert: func(g *gomega.WithT, ctx *context.ClusterContext) {
103+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules).To(gomega.HaveLen(1))
104+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].TargetObjectName).To(gomega.Equal("md"))
105+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].ModuleUUID).To(gomega.Equal(mdUUID))
106+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].ControlPlane).To(gomega.BeFalse())
107+
108+
g.Expect(conditions.Has(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
109+
g.Expect(conditions.IsFalse(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
110+
g.Expect(conditions.Get(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition).Message).To(gomega.ContainSubstring("kcp"))
111+
},
112+
},
113+
{
114+
name: "when cluster module creation fails",
115+
clusterModules: []infrav1.ClusterModule{},
116+
setupMocks: func(svc *cmodfake.CMService) {
117+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(kcp)).Return(kcpUUID, nil)
118+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(md)).Return("", errors.New("failed to reach API"))
119+
},
120+
// if cluster module creation fails for any reason apart from incompatibility, error should be returned
121+
haveError: true,
122+
customAssert: func(g *gomega.WithT, ctx *context.ClusterContext) {
123+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules).To(gomega.HaveLen(1))
124+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].TargetObjectName).To(gomega.Equal("kcp"))
125+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].ModuleUUID).To(gomega.Equal(kcpUUID))
126+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules[0].ControlPlane).To(gomega.BeTrue())
127+
128+
g.Expect(conditions.Has(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
129+
g.Expect(conditions.IsFalse(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
130+
g.Expect(conditions.Get(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition).Message).To(gomega.ContainSubstring("md"))
131+
},
132+
},
133+
{
134+
name: "when all cluster module creations fail for a resource pool owned by non compute cluster resource",
135+
clusterModules: []infrav1.ClusterModule{},
136+
setupMocks: func(svc *cmodfake.CMService) {
137+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(kcp)).Return("", clustermodule.NewIncompatibleOwnerError("foo-123"))
138+
svc.On("Create", mock.Anything, clustermodule.NewWrapper(md)).Return("", clustermodule.NewIncompatibleOwnerError("bar-123"))
139+
},
140+
// if cluster module creation fails due to resource pool owner incompatibility, vSphereCluster object is set to Ready
141+
haveError: false,
142+
customAssert: func(g *gomega.WithT, ctx *context.ClusterContext) {
143+
g.Expect(ctx.VSphereCluster.Spec.ClusterModules).To(gomega.HaveLen(0))
144+
g.Expect(conditions.Has(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
145+
g.Expect(conditions.IsFalse(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition)).To(gomega.BeTrue())
146+
g.Expect(conditions.Get(ctx.VSphereCluster, infrav1.ClusterModulesAvailableCondition).Message).To(gomega.ContainSubstring("kcp"))
147+
},
148+
},
92149
{
93150
name: "when machine deployment is being deleted",
94151
beforeFn: func(object client.Object) {
@@ -200,7 +257,11 @@ func TestReconciler_Reconcile(t *testing.T) {
200257
ClusterModuleService: svc,
201258
}
202259
_, err := r.Reconcile(ctx)
203-
g.Expect(err).ToNot(gomega.HaveOccurred())
260+
if tt.haveError {
261+
g.Expect(err).To(gomega.HaveOccurred())
262+
} else {
263+
g.Expect(err).ToNot(gomega.HaveOccurred())
264+
}
204265
tt.customAssert(g, ctx)
205266

206267
svc.AssertExpectations(t)

‎pkg/clustermodule/error.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/*
2+
Copyright 2022 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 clustermodule
18+
19+
import (
20+
"fmt"
21+
"strings"
22+
)
23+
24+
const errString = "not a compute cluster"
25+
26+
// IncompatibleOwnerError represents the error condition wherein the resource pool in use
27+
// during cluster module creation is not owned by compute cluster resource but owned by
28+
// a standalone host.
29+
type IncompatibleOwnerError struct {
30+
resource string
31+
}
32+
33+
func (e IncompatibleOwnerError) Error() string {
34+
return fmt.Sprintf("%s %s", e.resource, errString)
35+
}
36+
37+
// NewIncompatibleOwnerError creates an instance of the IncompatibleOwnerError struct.
38+
// This is introduced for testing purposes only.
39+
func NewIncompatibleOwnerError(name string) IncompatibleOwnerError {
40+
return IncompatibleOwnerError{resource: name}
41+
}
42+
43+
func IsIncompatibleOwnerError(err error) bool {
44+
return strings.HasSuffix(err.Error(), errString)
45+
}

‎pkg/clustermodule/service.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
"sigs.k8s.io/cluster-api-provider-vsphere/pkg/context"
2626
"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services/govmomi/clustermodules"
27+
"sigs.k8s.io/cluster-api-provider-vsphere/pkg/session"
2728
)
2829

2930
type service struct{}
@@ -48,7 +49,7 @@ func (s service) Create(ctx *context.ClusterContext, wrapper Wrapper) (string, e
4849

4950
// Fetch the compute cluster resource by tracing the owner of the resource pool in use.
5051
// TODO (srm09): How do we support Multi AZ scenarios here
51-
computeClusterRef, err := getComputeClusterResource(ctx, vCenterSession.Finder, template.Spec.Template.Spec.ResourcePool)
52+
computeClusterRef, err := getComputeClusterResource(ctx, vCenterSession, template.Spec.Template.Spec.ResourcePool)
5253
if err != nil {
5354
logger.V(4).Error(err, "error fetching compute cluster resource")
5455
return "", err
@@ -80,7 +81,7 @@ func (s service) DoesExist(ctx *context.ClusterContext, wrapper Wrapper, moduleU
8081

8182
// Fetch the compute cluster resource by tracing the owner of the resource pool in use.
8283
// TODO (srm09): How do we support Multi AZ scenarios here
83-
computeClusterRef, err := getComputeClusterResource(ctx, vCenterSession.Finder, template.Spec.Template.Spec.ResourcePool)
84+
computeClusterRef, err := getComputeClusterResource(ctx, vCenterSession, template.Spec.Template.Spec.ResourcePool)
8485
if err != nil {
8586
logger.V(4).Error(err, "error fetching compute cluster resource")
8687
return false, err
@@ -104,8 +105,8 @@ func (s service) Remove(ctx *context.ClusterContext, moduleUUID string) error {
104105
return nil
105106
}
106107

107-
func getComputeClusterResource(ctx goctx.Context, finder *find.Finder, resourcePool string) (types.ManagedObjectReference, error) {
108-
rp, err := finder.ResourcePool(ctx, resourcePool)
108+
func getComputeClusterResource(ctx goctx.Context, s *session.Session, resourcePool string) (types.ManagedObjectReference, error) {
109+
rp, err := s.Finder.ResourcePoolOrDefault(ctx, resourcePool)
109110
if err != nil {
110111
return types.ManagedObjectReference{}, err
111112
}
@@ -114,5 +115,14 @@ func getComputeClusterResource(ctx goctx.Context, finder *find.Finder, resourceP
114115
if err != nil {
115116
return types.ManagedObjectReference{}, err
116117
}
118+
119+
ownerPath, err := find.InventoryPath(ctx, s.Client.Client, cc.Reference())
120+
if err != nil {
121+
return types.ManagedObjectReference{}, err
122+
}
123+
if _, err = s.Finder.ClusterComputeResource(ctx, ownerPath); err != nil {
124+
return types.ManagedObjectReference{}, IncompatibleOwnerError{cc.Reference().Value}
125+
}
126+
117127
return cc.Reference(), nil
118128
}

0 commit comments

Comments
 (0)