Use field indexer instead of labels for image dependencies

Author: mdbooth

controllers/openstackserver_controller.go

@@ -58,7 +58,6 @@ import (
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 	capoerrors "sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/errors"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/names"
-	"sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/objects"
 )
 
 const (
@@ -171,24 +170,33 @@ func patchServer(ctx context.Context, patchHelper *patch.Helper, openStackServer
 }
 
 func (r *OpenStackServerReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, _ controller.Options) error {
+	const imageRefPath = "spec.image.imageRef.name"
+
 	log := ctrl.LoggerFrom(ctx)
 
+	// Index servers by referenced image
+	if err := mgr.GetFieldIndexer().IndexField(ctx, &infrav1alpha1.OpenStackServer{}, imageRefPath, func(obj client.Object) []string {
+		server, ok := obj.(*infrav1alpha1.OpenStackServer)
+		if !ok {
+			return nil
+		}
+		if server.Spec.Image.ImageRef == nil {
+			return nil
+		}
+		return []string{server.Spec.Image.ImageRef.Name}
+	}); err != nil {
+		return fmt.Errorf("adding servers by image index: %w", err)
+	}
+
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&infrav1alpha1.OpenStackServer{}).
 		Watches(&orcv1alpha1.Image{},
 			handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request {
 				log = log.WithValues("watch", "Image")
 
 				k8sClient := mgr.GetClient()
-				label, err := objToDependencyLabel(obj, k8sClient.Scheme())
-				if err != nil {
-					log.Error(err, "objToDependencyLabel")
-				}
-
-				labels := map[string]string{label: ""}
 				serverList := &infrav1alpha1.OpenStackServerList{}
-				err = k8sClient.List(ctx, serverList, client.MatchingLabels(labels), client.InNamespace(obj.GetNamespace()))
-				if err != nil {
+				if err := k8sClient.List(ctx, serverList, client.InNamespace(obj.GetNamespace()), client.MatchingFields{imageRefPath: obj.GetName()}); err != nil {
 					log.Error(err, "listing OpenStackServers")
 					return nil
 				}
@@ -267,27 +275,6 @@ func (r *OpenStackServerReconciler) reconcileDelete(scope *scope.WithLogger, ope
 	return nil
 }
 
-func objToDependencyLabel(obj client.Object, scheme *runtime.Scheme) (string, error) {
-	gvk, err := objects.GetGVK(obj, scheme)
-	if err != nil {
-		return "", err
-	}
-
-	if gvk.Group != orcv1alpha1.SchemeGroupVersion.Group {
-		return "", fmt.Errorf("dependency object has unexpected group %s", gvk.Group)
-	}
-
-	var prefixType string
-	switch gvk.Kind {
-	case "Image":
-		prefixType = "image"
-	default:
-		return "", fmt.Errorf("dependency object has unexpected kind %s", gvk.Kind)
-	}
-
-	return "orc-dependency-" + prefixType + "/" + obj.GetName(), nil
-}
-
 func IsServerTerminalError(server *infrav1alpha1.OpenStackServer) bool {
 	if server.Status.InstanceState != nil && *server.Status.InstanceState == infrav1.InstanceStateError {
 		return true
@@ -310,31 +297,7 @@ func (r *OpenStackServerReconciler) reconcileNormal(ctx context.Context, scope *
 		openStackServer.SetLabels(labels)
 	}
 
-	changed, dependencies, resolveDone, err := compute.ResolveServerSpec(ctx, scope, r.Client, openStackServer)
-
-	errs := make([]error, 0, len(dependencies)+1)
-	errs = append(errs, err)
-
-	// Add a dependency label for every object we depend on
-	// Set changed if we update any labels
-	// Collate all errors and return in a single transaction
-	for i := range dependencies {
-		errs = append(errs, func() error {
-			dependency := dependencies[i]
-			label, err := objToDependencyLabel(dependency, r.Client.Scheme())
-			if err != nil {
-				return err
-			}
-
-			if _, ok := labels[label]; !ok {
-				labels[label] = ""
-				changed = true
-			}
-			return nil
-		}())
-	}
-	err = errors.Join(errs...)
-
+	changed, resolveDone, err := compute.ResolveServerSpec(ctx, scope, r.Client, openStackServer)
 	if err != nil || !resolveDone {
 		return ctrl.Result{}, err
 	}

pkg/cloud/services/compute/instance.go

@@ -346,25 +346,25 @@ func (s *Service) getBlockDevices(eventObject runtime.Object, instanceSpec *Inst
 }
 
 // Helper function for getting image ID from name, ID, or tags.
-func (s *Service) GetImageID(ctx context.Context, k8sClient client.Client, namespace string, image infrav1.ImageParam) (*string, client.Object, error) {
+func (s *Service) GetImageID(ctx context.Context, k8sClient client.Client, namespace string, image infrav1.ImageParam) (*string, error) {
 	switch {
 	case image.ID != nil:
-		return image.ID, nil, nil
+		return image.ID, nil
 	case image.Filter != nil:
 		return s.getImageIDByFilter(image.Filter)
 	case image.ImageRef != nil:
 		return s.getImageIDByReference(ctx, k8sClient, namespace, image.ImageRef)
 	default:
 		// Should have been caught by validation
-		return nil, nil, errors.New("image id, filter, and reference are all nil")
+		return nil, errors.New("image id, filter, and reference are all nil")
 	}
 }
 
-func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, client.Object, error) {
+func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, error) {
 	listOpts := filterconvert.ImageFilterToListOpts(filter)
 	allImages, err := s.getImageClient().ListImages(listOpts)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	switch len(allImages) {
@@ -373,51 +373,48 @@ func (s *Service) getImageIDByFilter(filter *infrav1.ImageFilter) (*string, clie
 		if filter.Name != nil {
 			name = *filter.Name
 		}
-		return nil, nil, fmt.Errorf("no images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
+		return nil, fmt.Errorf("no images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
 	case 1:
-		return &allImages[0].ID, nil, nil
+		return &allImages[0].ID, nil
 	default:
 		// this should never happen
 		var name string
 		if filter.Name != nil {
 			name = *filter.Name
 		}
-		return nil, nil, fmt.Errorf("too many images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
+		return nil, fmt.Errorf("too many images were found with the given image filter: name=%v, tags=%v", name, filter.Tags)
 	}
 }
 
-func (s *Service) getImageIDByReference(ctx context.Context, k8sClient client.Client, namespace string, ref *infrav1.ResourceReference) (*string, client.Object, error) {
+func (s *Service) getImageIDByReference(ctx context.Context, k8sClient client.Client, namespace string, ref *infrav1.ResourceReference) (*string, error) {
 	orcImage := &orcv1alpha1.Image{}
 	err := k8sClient.Get(ctx, client.ObjectKey{
 		Namespace: namespace,
 		Name:      ref.Name,
 	}, orcImage)
 	if err != nil {
-		// If the object doesn't exist yet, we still need to return a hydrated reference to the object we're waiting on
+		// Not an error if it doesn't exist yet
 		if apierrors.IsNotFound(err) {
-			orcImage.SetName(ref.Name)
-			orcImage.SetNamespace(namespace)
-
-			return nil, orcImage, nil
+			return nil, nil
 		}
 
-		return nil, nil, err
+		return nil, err
 	}
 
 	if orcv1alpha1.IsAvailable(orcImage) {
-		return orcImage.Status.ID, orcImage, nil
+		return orcImage.Status.ID, nil
 	}
 
 	if !orcv1alpha1.IsReconciliationComplete(orcImage) {
-		return nil, orcImage, nil
+		return nil, nil
 	}
 
 	err = orcv1alpha1.GetTerminalError(orcImage)
 	if err != nil {
-		return nil, orcImage, capoerrors.Terminal(infrav1.DependencyFailedReason, orcImage.Kind+" "+orcImage.GetNamespace()+"/"+orcImage.GetName()+" failed: "+err.Error())
+		return nil, capoerrors.Terminal(infrav1.DependencyFailedReason, orcImage.Kind+" "+orcImage.GetNamespace()+"/"+orcImage.GetName()+" failed: "+err.Error())
 	}
 
-	return nil, orcImage, nil
+	return nil, nil
 }
 
 // GetManagementPort returns the port which is used for management and external

pkg/cloud/services/compute/instance_test.go

@@ -33,7 +33,6 @@ import (
 	"github.com/gophercloud/gophercloud/v2/openstack/image/v2/images"
 	. "github.com/onsi/gomega" //nolint:revive
 	"go.uber.org/mock/gomock"
-	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
@@ -52,8 +51,6 @@ func TestService_getImageID(t *testing.T) {
 		imageID   = "ce96e584-7ebc-46d6-9e55-987d72e3806c"
 		imageName = "test-image"
 		namespace = "test-namespace"
-
-		fakeClientResourceVersion = "999"
 	)
 	imageTags := []string{"test-tag"}
 
@@ -68,7 +65,6 @@ func TestService_getImageID(t *testing.T) {
 		fakeObjects       []runtime.Object
 		expect            func(m *mock.MockImageClientMockRecorder)
 		want              *string
-		wantDep           runtime.Object
 		wantErr           bool
 		wantTerminalError bool
 	}{
@@ -164,12 +160,6 @@ func TestService_getImageID(t *testing.T) {
 				},
 			},
 			want: nil,
-			wantDep: &orcv1alpha1.Image{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      imageName,
-					Namespace: namespace,
-				},
-			},
 		},
 		{
 			testName: "Image by reference exists, is available",
@@ -196,22 +186,6 @@ func TestService_getImageID(t *testing.T) {
 				},
 			},
 			want: ptr.To(imageID),
-			wantDep: &orcv1alpha1.Image{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:            imageName,
-					Namespace:       namespace,
-					ResourceVersion: fakeClientResourceVersion,
-				},
-				Status: orcv1alpha1.ImageStatus{
-					Conditions: []metav1.Condition{
-						{
-							Type:   orcv1alpha1.OpenStackConditionAvailable,
-							Status: metav1.ConditionTrue,
-						},
-					},
-					ID: ptr.To(imageID),
-				},
-			},
 		},
 		{
 			testName: "Image by reference exists, still reconciling",
@@ -243,27 +217,6 @@ func TestService_getImageID(t *testing.T) {
 				},
 			},
 			want: nil,
-			wantDep: &orcv1alpha1.Image{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:            imageName,
-					Namespace:       namespace,
-					ResourceVersion: fakeClientResourceVersion,
-				},
-				Status: orcv1alpha1.ImageStatus{
-					Conditions: []metav1.Condition{
-						{
-							Type:   orcv1alpha1.OpenStackConditionAvailable,
-							Status: metav1.ConditionFalse,
-						},
-						{
-							Type:   orcv1alpha1.OpenStackConditionProgressing,
-							Status: metav1.ConditionTrue,
-							Reason: orcv1alpha1.OpenStackConditionReasonProgressing,
-						},
-					},
-					ID: ptr.To(imageID),
-				},
-			},
 		},
 		{
 			testName: "Image by reference exists, terminal failure",
@@ -297,28 +250,6 @@ func TestService_getImageID(t *testing.T) {
 			},
 			want:              nil,
 			wantTerminalError: true,
-			wantDep: &orcv1alpha1.Image{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:            imageName,
-					Namespace:       namespace,
-					ResourceVersion: fakeClientResourceVersion,
-				},
-				Status: orcv1alpha1.ImageStatus{
-					Conditions: []metav1.Condition{
-						{
-							Type:   orcv1alpha1.OpenStackConditionAvailable,
-							Status: metav1.ConditionFalse,
-						},
-						{
-							Type:    orcv1alpha1.OpenStackConditionProgressing,
-							Status:  metav1.ConditionFalse,
-							Reason:  orcv1alpha1.OpenStackConditionReasonUnrecoverableError,
-							Message: "test error",
-						},
-					},
-					ID: ptr.To(imageID),
-				},
-			},
 		},
 	}
 	for _, tt := range tests {
@@ -337,7 +268,7 @@ func TestService_getImageID(t *testing.T) {
 
 			fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.fakeObjects...).Build()
 
-			got, dependency, err := s.GetImageID(context.TODO(), fakeClient, namespace, tt.image)
+			got, err := s.GetImageID(context.TODO(), fakeClient, namespace, tt.image)
 
 			if tt.wantTerminalError {
 				tt.wantErr = true
@@ -353,10 +284,6 @@ func TestService_getImageID(t *testing.T) {
 				t.Errorf("Terminal error: wanted = %v, got = %v", tt.wantTerminalError, !tt.wantTerminalError)
 			}
 
-			if !apiequality.Semantic.DeepEqual(tt.wantDep, dependency) {
-				t.Errorf("Dependency does not match: %s", cmp.Diff(tt.wantDep, dependency))
-			}
-
 			// NOTE(mdbooth): there must be a simpler way to write this!
 			if (tt.want == nil && got != nil) || (tt.want != nil && (got == nil || *tt.want != *got)) {
 				t.Errorf("Service.getImageID() = '%v', want '%v'", ptr.Deref(got, ""), ptr.Deref(tt.want, ""))