Filter workspaces by current run status (#419)

.changes/unreleased/ENHANCEMENTS-419-20240612-132330.yaml

@@ -0,0 +1,5 @@
+kind: ENHANCEMENTS
+body: '`AgentPool`: The agent auroscaling logic has been updated to decrease the frequency of API calls. The controller now utilizes the List Workspaces API call with filtering based on the current run status, thereby reducing the total number of API calls needed.'
+time: 2024-06-12T13:23:30.219348+02:00
+custom:
+    PR: "419"

controllers/agentpool_controller_autoscaling.go

@@ -12,115 +12,107 @@ import (
 	tfc "github.com/hashicorp/go-tfe"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 
 	appv1alpha2 "github.com/hashicorp/terraform-cloud-operator/api/v1alpha2"
 )
 
-func computeRequiredAgentsForWorkspace(ctx context.Context, ap *agentPoolInstance, workspaceID string) (int, error) {
-	statuses := []string{
+// matchWildcardName checks if a given string matches a specified wildcard pattern.
+// The wildcard pattern can contain '*' at the beginning and/or end to match any sequence of characters.
+// If the pattern contains '*' at both ends, the function checks if the substring exists within the string.
+// If the pattern contains '*' only at the beginning, the function checks if the string ends with the substring.
+// If the pattern contains '*' only at the end, the function checks if the string starts with the substring.
+// If there are no '*' characters, the function checks for an exact match.
+// For example:
+// (1) '*-terraform-workspace' -- the wildcard indicator '*' is at the beginning of the wildcard name (prefix),
+// therefore, we should search for a workspace name that ends with the suffix '-terraform-workspace'.
+// (2) 'hcp-terraform-workspace-*' -- the wildcard indicator '*' is at the end of the wildcard name (suffix),
+// therefore, we should search for a workspace name that starts with the prefix 'hcp-terraform-workspace-'.
+// (3) '*-terraform-workspace-*' -- the wildcard indicator '*' is at the beginning and the end of the wildcard name (prefix and suffix),
+// therefore, we should search for a workspace name containing the substring '-terraform-workspace-'.
+func matchWildcardName(wildcard string, str string) bool {
+	// Both 'prefix' and 'suffix' indicate whether a part of the name is in the prefix, suffix, or both.
+	// If the wildcard indicator '*' is in the PREFIX part, then search for a substring that is in the SUFFIX.
+	// If the wildcard indicator '*' is in the SUFFIX part, then search for a substring that is in the PREFIX.
+	// If the wildcard indicator '*' is in both the prefix and the suffix, then search for a substring that is in between '*'.
+	prefix := strings.HasSuffix(wildcard, "*")
+	suffix := strings.HasPrefix(wildcard, "*")
+	wn := strings.Trim(wildcard, "*")
+	switch {
+	case prefix && suffix:
+		return strings.Contains(str, wn)
+	case prefix:
+		return strings.HasPrefix(str, wn)
+	case suffix:
+		return strings.HasSuffix(str, wn)
+	default:
+		return wn == str
+	}
+}
+
+func computeRequiredAgents(ctx context.Context, ap *agentPoolInstance) (int32, error) {
+	required := 0
+	runStatuses := strings.Join([]string{
 		string(tfc.RunPlanQueued),
 		string(tfc.RunApplyQueued),
 		string(tfc.RunApplying),
 		string(tfc.RunPlanning),
-	}
-	runs, err := ap.tfClient.Client.Runs.List(ctx, workspaceID, &tfc.RunListOptions{
-		Status: strings.Join(statuses, ","),
-	})
-	if err != nil {
-		return 0, err
-	}
-	return len(runs.Items), nil
-}
-
-func getAllAgentPoolWorkspaceIDs(ctx context.Context, ap *agentPoolInstance) ([]string, error) {
-	agentPool, err := ap.tfClient.Client.AgentPools.Read(ctx, ap.instance.Status.AgentPoolID)
-	if err != nil {
-		return []string{}, nil
-	}
-	ids := []string{}
-	for _, w := range agentPool.Workspaces {
-		ids = append(ids, w.ID)
-	}
-	return ids, nil
-}
-
-func getTargetWorkspaceIDs(ctx context.Context, ap *agentPoolInstance) ([]string, error) {
-	workspaces := ap.instance.Spec.AgentDeploymentAutoscaling.TargetWorkspaces
-	if workspaces == nil {
-		return getAllAgentPoolWorkspaceIDs(ctx, ap)
-	}
-	workspaceIDs := map[string]struct{}{} // NOTE: this is a map so we avoid duplicates when using wildcards
-	for _, w := range *workspaces {
-		if w.WildcardName != "" {
-			ids, err := getTargetWorkspaceIDsByWildcardName(ctx, ap, w)
-			if err != nil {
-				return []string{}, err
-			}
-			for _, id := range ids {
-				workspaceIDs[id] = struct{}{}
-			}
-			continue
-		}
-		id, err := getTargetWorkspaceID(ctx, ap, w)
+	}, ",")
+	// NOTE:
+	// - Two maps are used here to simplify target workspace searching by ID, name, and wildcard.
+	workspaceNames := map[string]struct{}{}
+	workspaceIDs := map[string]struct{}{}
+
+	pageNumber := 1
+	for {
+		workspaceList, err := ap.tfClient.Client.Workspaces.List(ctx, ap.instance.Spec.Organization, &tfc.WorkspaceListOptions{
+			CurrentRunStatus: runStatuses,
+			ListOptions: tfc.ListOptions{
+				PageSize:   maxPageSize,
+				PageNumber: pageNumber,
+			},
+		})
 		if err != nil {
-			return []string{}, err
+			return 0, err
 		}
-		workspaceIDs[id] = struct{}{}
-	}
-	ids := []string{}
-	for v := range workspaceIDs {
-		ids = append(ids, v)
-	}
-	return ids, nil
-}
-
-func getTargetWorkspaceID(ctx context.Context, ap *agentPoolInstance, targetWorkspace appv1alpha2.TargetWorkspace) (string, error) {
-	if targetWorkspace.ID != "" {
-		return targetWorkspace.ID, nil
-	}
-	list, err := ap.tfClient.Client.Workspaces.List(ctx, ap.instance.Spec.Organization, &tfc.WorkspaceListOptions{
-		Search: targetWorkspace.Name,
-	})
-	if err != nil {
-		return "", err
-	}
-	for _, w := range list.Items {
-		if w.Name == targetWorkspace.Name {
-			return w.ID, nil
+		for _, ws := range workspaceList.Items {
+			if ws.AgentPool.ID == ap.instance.Status.AgentPoolID {
+				workspaceNames[ws.Name] = struct{}{}
+				workspaceIDs[ws.ID] = struct{}{}
+			}
+		}
+		if workspaceList.NextPage == 0 {
+			break
 		}
+		pageNumber = workspaceList.NextPage
 	}
-	return "", fmt.Errorf("no such workspace found %q", targetWorkspace.Name)
-}
 
-func getTargetWorkspaceIDsByWildcardName(ctx context.Context, ap *agentPoolInstance, targetWorkspace appv1alpha2.TargetWorkspace) ([]string, error) {
-	list, err := ap.tfClient.Client.Workspaces.List(ctx, ap.instance.Spec.Organization, &tfc.WorkspaceListOptions{
-		WildcardName: targetWorkspace.WildcardName,
-	})
-	if err != nil {
-		return []string{}, err
-	}
-	workspaceIDs := []string{}
-	for _, w := range list.Items {
-		workspaceIDs = append(workspaceIDs, w.ID)
+	if ap.instance.Spec.AgentDeploymentAutoscaling.TargetWorkspaces == nil {
+		return int32(len(workspaceNames)), nil
 	}
-	return workspaceIDs, nil
-}
 
-func computeRequiredAgents(ctx context.Context, ap *agentPoolInstance) (int32, error) {
-	required := 0
-	workspaceIDs, err := getTargetWorkspaceIDs(ctx, ap)
-	if err != nil {
-		return 0, err
-	}
-	for _, workspaceID := range workspaceIDs {
-		r, err := computeRequiredAgentsForWorkspace(ctx, ap, workspaceID)
-		if err != nil {
-			return 0, err
+	for _, t := range *ap.instance.Spec.AgentDeploymentAutoscaling.TargetWorkspaces {
+		switch {
+		case t.Name != "":
+			if _, ok := workspaceNames[t.Name]; ok {
+				required++
+				delete(workspaceNames, t.Name)
+			}
+		case t.ID != "":
+			if _, ok := workspaceIDs[t.ID]; ok {
+				required++
+			}
+		case t.WildcardName != "":
+			for w := range workspaceNames {
+				if ok := matchWildcardName(t.WildcardName, w); ok {
+					required++
+					delete(workspaceNames, w)
+				}
+			}
 		}
-		required += r
 	}
+
 	return int32(required), nil
 }
 
@@ -189,20 +181,22 @@ func (r *AgentPoolReconciler) reconcileAgentAutoscaling(ctx context.Context, ap
 		r.Recorder.Eventf(&ap.instance, corev1.EventTypeWarning, "AutoscaleAgentPoolDeployment", "Autoscaling failed: %v", err.Error())
 		return err
 	}
+	ap.log.Info("Reconcile Agent Autoscaling", "msg", fmt.Sprintf("%d workspaces have pending runs", requiredAgents))
 
 	currentReplicas, err := r.getAgentDeploymentReplicas(ctx, ap)
 	if err != nil {
 		ap.log.Error(err, "Reconcile Agent Autoscaling", "msg", "Failed to get current replicas")
 		r.Recorder.Eventf(&ap.instance, corev1.EventTypeWarning, "AutoscaleAgentPoolDeployment", "Autoscaling failed: %v", err.Error())
 		return err
 	}
+	ap.log.Info("Reconcile Agent Autoscaling", "msg", fmt.Sprintf("%d agent replicas are running", currentReplicas))
 
 	minReplicas := *ap.instance.Spec.AgentDeploymentAutoscaling.MinReplicas
 	maxReplicas := *ap.instance.Spec.AgentDeploymentAutoscaling.MaxReplicas
 	desiredReplicas := computeDesiredReplicas(requiredAgents, minReplicas, maxReplicas)
 	if desiredReplicas != currentReplicas {
 		scalingEvent := fmt.Sprintf("Scaling agent deployment from %v to %v replicas", currentReplicas, desiredReplicas)
-		ap.log.Info("Reconcile Agent Autoscaling", "msg", scalingEvent)
+		ap.log.Info("Reconcile Agent Autoscaling", "msg", strings.ToLower(scalingEvent))
 		r.Recorder.Event(&ap.instance, corev1.EventTypeNormal, "AutoscaleAgentPoolDeployment", scalingEvent)
 		err := r.scaleAgentDeployment(ctx, ap, &desiredReplicas)
 		if err != nil {
@@ -212,7 +206,7 @@ func (r *AgentPoolReconciler) reconcileAgentAutoscaling(ctx context.Context, ap
 		}
 		ap.instance.Status.AgentDeploymentAutoscalingStatus = &appv1alpha2.AgentDeploymentAutoscalingStatus{
 			DesiredReplicas: &desiredReplicas,
-			LastScalingEvent: &v1.Time{
+			LastScalingEvent: &metav1.Time{
 				Time: time.Now(),
 			},
 		}

controllers/agentpool_controller_autoscaling_test.go

@@ -0,0 +1,48 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package controllers
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Helpers", Label("Unit"), func() {
+	Context("Match wildcard name", func() {
+		// True
+		It("match prefix", func() {
+			result := matchWildcardName("*-terraform-workspace", "hcp-terraform-workspace")
+			Expect(result).To(BeTrue())
+		})
+		It("match suffix", func() {
+			result := matchWildcardName("hcp-terraform-*", "hcp-terraform-workspace")
+			Expect(result).To(BeTrue())
+		})
+		It("match prefix and suffix", func() {
+			result := matchWildcardName("*-terraform-*", "hcp-terraform-workspace")
+			Expect(result).To(BeTrue())
+		})
+		It("match no prefix and no suffix", func() {
+			result := matchWildcardName("hcp-terraform-workspace", "hcp-terraform-workspace")
+			Expect(result).To(BeTrue())
+		})
+		// False
+		It("does not match prefix", func() {
+			result := matchWildcardName("*-terraform-workspace", "hcp-tf-workspace")
+			Expect(result).To(BeFalse())
+		})
+		It("does not match suffix", func() {
+			result := matchWildcardName("hcp-terraform-*", "hashicorp-tf-workspace")
+			Expect(result).To(BeFalse())
+		})
+		It("does not match prefix and suffix", func() {
+			result := matchWildcardName("*-terraform-*", "hcp-tf-workspace")
+			Expect(result).To(BeFalse())
+		})
+		It("does not match no prefix and no suffix", func() {
+			result := matchWildcardName("hcp-terraform-workspace", "hcp-tf-workspace")
+			Expect(result).To(BeFalse())
+		})
+	})
+})

controllers/consts.go

@@ -11,6 +11,7 @@ import (
 const (
 	annotationTrue  = "true"
 	annotationFalse = "false"
+	maxPageSize     = 100
 	requeueInterval = 15 * time.Second
 	runMessage      = "Triggered by HCP Terraform Operator"
 )

controllers/workspace_controller_outputs.go

@@ -109,7 +109,7 @@ func (r *WorkspaceReconciler) setOutputs(ctx context.Context, w *workspaceInstan
 		Namespace: w.instance.Namespace,
 	}
 	labels := map[string]string{
-		"workspaceID":   w.instance.Status.WorkspaceID,
+		"workspaceID": w.instance.Status.WorkspaceID,
 	}
 
 	// update ConfigMap output