Skip to content

Commit 9b741f1

Browse files
mdboothmdbooth
committed
Use field indexer instead of labels for image dependencies
1 parent e3c4521 commit 9b741f1

File tree

5 files changed

+42
-160
lines changed

5 files changed

+42
-160
lines changed

‎controllers/openstackserver_controller.go

Lines changed: 18 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ import (
5555
"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
5656
capoerrors "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors"
5757
"sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names"
58-
"sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/objects"
5958
)
6059

6160
const (
@@ -168,24 +167,33 @@ func patchServer(ctx context.Context, patchHelper *patch.Helper, openStackServer
168167
}
169168

170169
func (r *OpenStackServerReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, _ controller.Options) error {
170+
const imageRefPath = "spec.image.imageRef.name"
171+
171172
log := ctrl.LoggerFrom(ctx)
172173

174+
// Index servers by referenced image
175+
if err := mgr.GetFieldIndexer().IndexField(ctx, &infrav1alpha1.OpenStackServer{}, imageRefPath, func(obj client.Object) []string {
176+
server, ok := obj.(*infrav1alpha1.OpenStackServer)
177+
if !ok {
178+
return nil
179+
}
180+
if server.Spec.Image.ImageRef == nil {
181+
return nil
182+
}
183+
return []string{server.Spec.Image.ImageRef.Name}
184+
}); err != nil {
185+
return fmt.Errorf("adding servers by image index: %w", err)
186+
}
187+
173188
return ctrl.NewControllerManagedBy(mgr).
174189
For(&infrav1alpha1.OpenStackServer{}).
175190
Watches(&orcv1alpha1.Image{},
176191
handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request {
177192
log = log.WithValues("watch", "Image")
178193

179194
k8sClient := mgr.GetClient()
180-
label, err := objToDependencyLabel(obj, k8sClient.Scheme())
181-
if err != nil {
182-
log.Error(err, "objToDependencyLabel")
183-
}
184-
185-
labels := map[string]string{label: ""}
186195
serverList := &infrav1alpha1.OpenStackServerList{}
187-
err = k8sClient.List(ctx, serverList, client.MatchingLabels(labels), client.InNamespace(obj.GetNamespace()))
188-
if err != nil {
196+
if err := k8sClient.List(ctx, serverList, client.InNamespace(obj.GetNamespace()), client.MatchingFields{imageRefPath: obj.GetName()}); err != nil {
189197
log.Error(err, "listing OpenStackServers")
190198
return nil
191199
}
@@ -264,27 +272,6 @@ func (r *OpenStackServerReconciler) reconcileDelete(scope *scope.WithLogger, ope
264272
return nil
265273
}
266274

267-
func objToDependencyLabel(obj client.Object, scheme *runtime.Scheme) (string, error) {
268-
gvk, err := objects.GetGVK(obj, scheme)
269-
if err != nil {
270-
return "", err
271-
}
272-
273-
if gvk.Group != orcv1alpha1.SchemeGroupVersion.Group {
274-
return "", fmt.Errorf("dependency object has unexpected group %s", gvk.Group)
275-
}
276-
277-
var prefixType string
278-
switch gvk.Kind {
279-
case "Image":
280-
prefixType = "image"
281-
default:
282-
return "", fmt.Errorf("dependency object has unexpected kind %s", gvk.Kind)
283-
}
284-
285-
return "orc-dependency-" + prefixType + "/" + obj.GetName(), nil
286-
}
287-
288275
func (r *OpenStackServerReconciler) reconcileNormal(ctx context.Context, scope *scope.WithLogger, openStackServer *infrav1alpha1.OpenStackServer) (_ ctrl.Result, reterr error) {
289276
// If the OpenStackServer is in an error state, return early.
290277
if openStackServer.Status.InstanceState != nil && *openStackServer.Status.InstanceState == infrav1.InstanceStateError {
@@ -300,31 +287,7 @@ func (r *OpenStackServerReconciler) reconcileNormal(ctx context.Context, scope *
300287
openStackServer.SetLabels(labels)
301288
}
302289

303-
changed, dependencies, resolveDone, err := compute.ResolveServerSpec(ctx, scope, r.Client, openStackServer)
304-
305-
errs := make([]error, 0, len(dependencies)+1)
306-
errs = append(errs, err)
307-
308-
// Add a dependency label for every object we depend on
309-
// Set changed if we update any labels
310-
// Collate all errors and return in a single transaction
311-
for i := range dependencies {
312-
errs = append(errs, func() error {
313-
dependency := dependencies[i]
314-
label, err := objToDependencyLabel(dependency, r.Client.Scheme())
315-
if err != nil {
316-
return err
317-
}
318-
319-
if _, ok := labels[label]; !ok {
320-
labels[label] = ""
321-
changed = true
322-
}
323-
return nil
324-
}())
325-
}
326-
err = errors.Join(errs...)
327-
290+
changed, resolveDone, err := compute.ResolveServerSpec(ctx, scope, r.Client, openStackServer)
328291
if err != nil || !resolveDone {
329292
return ctrl.Result{}, err
330293
}

‎pkg/cloud/services/compute/instance.go

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -346,25 +346,25 @@ func (s *Service) getBlockDevices(eventObject runtime.Object, instanceSpec *Inst
346346
}
347347

348348
// Helper function for getting image ID from name, ID, or tags.
349-
func (s *Service) GetImageID(ctx context.Context, k8sClient client.Client, namespace string, image infrav1.ImageParam) (*string, client.Object, error) {
349+
func (s *Service) GetImageID(ctx context.Context, k8sClient client.Client, namespace string, image infrav1.ImageParam) (*string, error) {
350350
switch {
351351
case image.ID != nil:
352-
return image.ID, nil, nil
352+
return image.ID, nil
353353
case image.Filter != nil:
354354
return s.getImageIDByFilter(image.Filter)
355355
case image.ImageRef != nil:
356356
return s.getImageIDByReference(ctx, k8sClient, namespace, image.ImageRef)
357357
default:
358358
// Should have been caught by validation
359-
return nil, nil, errors.New("image id, filter, and reference are all nil")
359+
return nil, errors.New("image id, filter, and reference are all nil")
360360
}
361361
}
362362

363-
func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, client.Object, error) {
363+
func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, error) {
364364
listOpts := filterconvert.ImageFilterToListOpts(filter)
365365
allImages, err := s.getImageClient().ListImages(listOpts)
366366
if err != nil {
367-
return nil, nil, err
367+
return nil, err
368368
}
369369

370370
switch len(allImages) {
@@ -373,51 +373,48 @@ func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, clie
373373
if filter.Name != nil {
374374
name = *filter.Name
375375
}
376-
return nil, nil, fmt.Errorf("no images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
376+
return nil, fmt.Errorf("no images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
377377
case 1:
378-
return &allImages[0].ID, nil, nil
378+
return &allImages[0].ID, nil
379379
default:
380380
// this should never happen
381381
var name string
382382
if filter.Name != nil {
383383
name = *filter.Name
384384
}
385-
return nil, nil, fmt.Errorf("too many images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
385+
return nil, fmt.Errorf("too many images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
386386
}
387387
}
388388

389-
func (s *Service) getImageIDByReference(ctx context.Context, k8sClient client.Client, namespace string, ref *infrav1.ResourceReference) (*string, client.Object, error) {
389+
func (s *Service) getImageIDByReference(ctx context.Context, k8sClient client.Client, namespace string, ref *infrav1.ResourceReference) (*string, error) {
390390
orcImage := &orcv1alpha1.Image{}
391391
err := k8sClient.Get(ctx, client.ObjectKey{
392392
Namespace: namespace,
393393
Name: ref.Name,
394394
}, orcImage)
395395
if err != nil {
396-
// If the object doesn't exist yet, we still need to return a hydrated reference to the object we're waiting on
396+
// Not an error if it doesn't exist yet
397397
if apierrors.IsNotFound(err) {
398-
orcImage.SetName(ref.Name)
399-
orcImage.SetNamespace(namespace)
400-
401-
return nil, orcImage, nil
398+
return nil, nil
402399
}
403400

404-
return nil, nil, err
401+
return nil, err
405402
}
406403

407404
if orcv1alpha1.IsAvailable(orcImage) {
408-
return orcImage.Status.ID, orcImage, nil
405+
return orcImage.Status.ID, nil
409406
}
410407

411408
if !orcv1alpha1.IsReconciliationComplete(orcImage) {
412-
return nil, orcImage, nil
409+
return nil, nil
413410
}
414411

415412
err = orcv1alpha1.GetTerminalError(orcImage)
416413
if err != nil {
417-
return nil, orcImage, capoerrors.Terminal(infrav1.DependencyFailedReason, orcImage.Kind+" "+orcImage.GetNamespace()+"/"+orcImage.GetName()+" failed: "+err.Error())
414+
return nil, capoerrors.Terminal(infrav1.DependencyFailedReason, orcImage.Kind+" "+orcImage.GetNamespace()+"/"+orcImage.GetName()+" failed: "+err.Error())
418415
}
419416

420-
return nil, orcImage, nil
417+
return nil, nil
421418
}
422419

423420
// GetManagementPort returns the port which is used for management and external

‎pkg/cloud/services/compute/instance_test.go

Lines changed: 1 addition & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ import (
3333
"github.com/gophercloud/gophercloud/v2/openstack/image/v2/images"
3434
. "github.com/onsi/gomega" //nolint:revive
3535
"go.uber.org/mock/gomock"
36-
apiequality "k8s.io/apimachinery/pkg/api/equality"
3736
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3837
"k8s.io/apimachinery/pkg/runtime"
3938
"k8s.io/utils/ptr"
@@ -52,8 +51,6 @@ func TestService_getImageID(t *testing.T) {
5251
imageID = "ce96e584-7ebc-46d6-9e55-987d72e3806c"
5352
imageName = "test-image"
5453
namespace = "test-namespace"
55-
56-
fakeClientResourceVersion = "999"
5754
)
5855
imageTags := []string{"test-tag"}
5956

@@ -68,7 +65,6 @@ func TestService_getImageID(t *testing.T) {
6865
fakeObjects []runtime.Object
6966
expect func(m *mock.MockImageClientMockRecorder)
7067
want *string
71-
wantDep runtime.Object
7268
wantErr bool
7369
wantTerminalError bool
7470
}{
@@ -164,12 +160,6 @@ func TestService_getImageID(t *testing.T) {
164160
},
165161
},
166162
want: nil,
167-
wantDep: &orcv1alpha1.Image{
168-
ObjectMeta: metav1.ObjectMeta{
169-
Name: imageName,
170-
Namespace: namespace,
171-
},
172-
},
173163
},
174164
{
175165
testName: "Image by reference exists, is available",
@@ -196,22 +186,6 @@ func TestService_getImageID(t *testing.T) {
196186
},
197187
},
198188
want: ptr.To(imageID),
199-
wantDep: &orcv1alpha1.Image{
200-
ObjectMeta: metav1.ObjectMeta{
201-
Name: imageName,
202-
Namespace: namespace,
203-
ResourceVersion: fakeClientResourceVersion,
204-
},
205-
Status: orcv1alpha1.ImageStatus{
206-
Conditions: []metav1.Condition{
207-
{
208-
Type: orcv1alpha1.OpenStackConditionAvailable,
209-
Status: metav1.ConditionTrue,
210-
},
211-
},
212-
ID: ptr.To(imageID),
213-
},
214-
},
215189
},
216190
{
217191
testName: "Image by reference exists, still reconciling",
@@ -243,27 +217,6 @@ func TestService_getImageID(t *testing.T) {
243217
},
244218
},
245219
want: nil,
246-
wantDep: &orcv1alpha1.Image{
247-
ObjectMeta: metav1.ObjectMeta{
248-
Name: imageName,
249-
Namespace: namespace,
250-
ResourceVersion: fakeClientResourceVersion,
251-
},
252-
Status: orcv1alpha1.ImageStatus{
253-
Conditions: []metav1.Condition{
254-
{
255-
Type: orcv1alpha1.OpenStackConditionAvailable,
256-
Status: metav1.ConditionFalse,
257-
},
258-
{
259-
Type: orcv1alpha1.OpenStackConditionProgressing,
260-
Status: metav1.ConditionTrue,
261-
Reason: orcv1alpha1.OpenStackConditionReasonProgressing,
262-
},
263-
},
264-
ID: ptr.To(imageID),
265-
},
266-
},
267220
},
268221
{
269222
testName: "Image by reference exists, terminal failure",
@@ -297,28 +250,6 @@ func TestService_getImageID(t *testing.T) {
297250
},
298251
want: nil,
299252
wantTerminalError: true,
300-
wantDep: &orcv1alpha1.Image{
301-
ObjectMeta: metav1.ObjectMeta{
302-
Name: imageName,
303-
Namespace: namespace,
304-
ResourceVersion: fakeClientResourceVersion,
305-
},
306-
Status: orcv1alpha1.ImageStatus{
307-
Conditions: []metav1.Condition{
308-
{
309-
Type: orcv1alpha1.OpenStackConditionAvailable,
310-
Status: metav1.ConditionFalse,
311-
},
312-
{
313-
Type: orcv1alpha1.OpenStackConditionProgressing,
314-
Status: metav1.ConditionFalse,
315-
Reason: orcv1alpha1.OpenStackConditionReasonUnrecoverableError,
316-
Message: "test error",
317-
},
318-
},
319-
ID: ptr.To(imageID),
320-
},
321-
},
322253
},
323254
}
324255
for _, tt := range tests {
@@ -337,7 +268,7 @@ func TestService_getImageID(t *testing.T) {
337268

338269
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.fakeObjects...).Build()
339270

340-
got, dependency, err := s.GetImageID(context.TODO(), fakeClient, namespace, tt.image)
271+
got, err := s.GetImageID(context.TODO(), fakeClient, namespace, tt.image)
341272

342273
if tt.wantTerminalError {
343274
tt.wantErr = true
@@ -353,10 +284,6 @@ func TestService_getImageID(t *testing.T) {
353284
t.Errorf("Terminal error: wanted = %v, got = %v", tt.wantTerminalError, !tt.wantTerminalError)
354285
}
355286

356-
if !apiequality.Semantic.DeepEqual(tt.wantDep, dependency) {
357-
t.Errorf("Dependency does not match: %s", cmp.Diff(tt.wantDep, dependency))
358-
}
359-
360287
// NOTE(mdbooth): there must be a simpler way to write this!
361288
if (tt.want == nil && got != nil) || (tt.want != nil && (got == nil || *tt.want != *got)) {
362289
t.Errorf("Service.getImageID() = '%v', want '%v'", ptr.Deref(got, ""), ptr.Deref(tt.want, ""))

0 commit comments

Comments
 (0)