Repository: tektoncd/operator (go)

Issue:
"options" adding resource entries on multiple installersets
it is possible to create `HPA` and `ConfigMap` via `options` [1] 
I was not aware of multiple installersets creation for a component at the time of `options` implementation.
I see, Two type of installersets [2] created for pipelines component,
1. deployment only
2. static - all others (except deployment)

The `options` transformer called two times via transformer[3].
Assume user provided a details to update a existing HPA resource.
first the transformer called with only deployments manifests,
* checks HPA resource
* HPA resource not available
* Creates HPA resource and adding in that manifest (deployments manifests)

second time the transformer called with static manifests(except deployments)
* checks HPA resource
* HPA resource found
* updates HPA resource on the manifest

Now we have the same HPA resource on two installersets. This leads race condition and malfunction.

[1] - https://tekton.dev/docs/operator/tektonconfig/#additional-fields-as-options
[2] - https://github.com/tektoncd/operator/blob/2754ea9e1bcf7fd74cb55d4faf6f993c5f1fbe16/pkg/reconciler/kubernetes/tektoninstallerset/client/create.go#L61-L62
[3] - https://github.com/tektoncd/operator/blob/2754ea9e1bcf7fd74cb55d4faf6f993c5f1fbe16/pkg/reconciler/kubernetes/tektonpipeline/transform.go#L55 https://github.com/tektoncd/operator/blob/2754ea9e1bcf7fd74cb55d4faf6f993c5f1fbe16/pkg/reconciler/kubernetes/tektonpipeline/transform.go#L88

Pull request description:
[main] fix HPA maxReplicas update and disable HPA creation via options 
# Changes

* fixes [SRVKP-4160](https://issues.redhat.com/browse/SRVKP-4160) - on "options" leaving maxReplicas on HPA, updates minReplicas+1 on the cluster
* fixes [SRVKP-4161](https://issues.redhat.com/browse/SRVKP-4161) - disable HPA creation via options
   * HPA will be enabled once the issue resolved #2002

# Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

- [ ] Run `make test lint` before submitting a PR
- [x] Includes [tests](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if functionality changed/added)
- [x] Includes [docs](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if user facing)
- [x] Commit messages follow [commit message best practices](https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)

_See [the contribution guide](https://github.com/tektoncd/operator/blob/master/CONTRIBUTING.md) for more details._

# Release Notes

```release-note
- HPA creation disabled, see https://github.com/tektoncd/operator/issues/2002 for more details
- HPA maxReplicas will not be updated, if it is not set under options
```


Patch:
diff --git a/docs/TektonConfig.md b/docs/TektonConfig.md
index 1645ab23a..ef51f77ef 100644
--- a/docs/TektonConfig.md
+++ b/docs/TektonConfig.md
@@ -626,7 +626,8 @@ The following fields are supported in `StatefulSet`
         * `args` - appends given args with existing arguments. **NOTE: THIS OPERATION DO NOT REPLACE EXISTING ARGS** 
 
 #### HorizontalPodAutoscalers
-Supports to update the existing HorizontalPodAutoscaler(HPA) also supports to create new HPA.
+Supports to update the existing HorizontalPodAutoscaler(HPA) ~~also supports to create new HPA.~~
+There is open [issue](https://github.com/tektoncd/operator/issues/2002) on `options` transformer. Until resolve the issue create HPA feature will not be available.
 
 The following fields are supported in `HorizontalPodAutoscaler` (aka HPA)
 * `metadata`
diff --git a/pkg/reconciler/common/transformer_additional_options.go b/pkg/reconciler/common/transformer_additional_options.go
index e35d50385..4c48fbca0 100644
--- a/pkg/reconciler/common/transformer_additional_options.go
+++ b/pkg/reconciler/common/transformer_additional_options.go
@@ -71,15 +71,17 @@ func ExecuteAdditionalOptionsTransformer(ctx context.Context, manifest *mf.Manif
 		return err
 	}
 
+	// disabled HPA creation until we resolve the bug
+	// BUG: https://github.com/tektoncd/operator/issues/2002
 	// create HorizontalPodAutoscaler, if not found in the existing manifest
-	extraHPAs, err := ot.createHorizontalPodAutoscalers(manifest, targetNamespace, additionalOptions)
-	if err != nil {
-		return err
-	}
-	// update into the manifests
-	if err = ot.addInToManifest(manifest, extraHPAs); err != nil {
-		return err
-	}
+	// extraHPAs, err := ot.createHorizontalPodAutoscalers(manifest, targetNamespace, additionalOptions)
+	// if err != nil {
+	// 	return err
+	// }
+	// // update into the manifests
+	// if err = ot.addInToManifest(manifest, extraHPAs); err != nil {
+	// 	return err
+	// }
 
 	return nil
 }
@@ -585,23 +587,15 @@ func (ot *OptionsTransformer) updateHorizontalPodAutoscalers(u *unstructured.Uns
 		targetHpa.Spec.ScaleTargetRef = hpaOptions.Spec.ScaleTargetRef
 	}
 
-	// update minimum replicas
-	// default minimum replicas count is 1
-	minReplicas := int32(1)
+	// updates min replicas
 	if hpaOptions.Spec.MinReplicas != nil {
-		minReplicas = *hpaOptions.Spec.MinReplicas
+		targetHpa.Spec.MinReplicas = ptr.Int32(*hpaOptions.Spec.MinReplicas)
 	}
-	// update min replicas
-	targetHpa.Spec.MinReplicas = ptr.Int32(minReplicas)
 
-	// update maximum replicas
-	// maxReplicas should be greater than minReplicas
-	maxReplicas := hpaOptions.Spec.MaxReplicas
-	if maxReplicas <= minReplicas {
-		maxReplicas = minReplicas + 1
+	// updates max replicas
+	if hpaOptions.Spec.MaxReplicas > 0 {
+		targetHpa.Spec.MaxReplicas = hpaOptions.Spec.MaxReplicas
 	}
-	// update max replicas
-	targetHpa.Spec.MaxReplicas = maxReplicas
 
 	// update metrics
 	if len(hpaOptions.Spec.Metrics) > 0 {
@@ -636,46 +630,48 @@ func (ot *OptionsTransformer) updateHorizontalPodAutoscalers(u *unstructured.Uns
 	return nil
 }
 
-func (ot *OptionsTransformer) createHorizontalPodAutoscalers(manifest *mf.Manifest, targetNamespace string, additionalOptions v1alpha1.AdditionalOptions) ([]unstructured.Unstructured, error) {
-	newHPAs := []unstructured.Unstructured{}
-	existingHPAs := manifest.Filter(mf.Any(mf.ByKind(KindHorizontalPodAutoscaler)))
-	for hpaName, newHPA := range additionalOptions.HorizontalPodAutoscalers {
-		found := false
-		for _, resource := range existingHPAs.Resources() {
-			if resource.GetName() == hpaName {
-				found = true
-				break
-			}
-		}
-		if found {
-			continue
-		}
-
-		// update name
-		newHPA.SetName(hpaName)
-
-		// update the namespace to targetNamespace
-		newHPA.SetNamespace(targetNamespace)
-
-		// update kind
-		if newHPA.TypeMeta.Kind == "" {
-			newHPA.TypeMeta.Kind = KindHorizontalPodAutoscaler
-		}
-
-		// update api version
-		if newHPA.TypeMeta.APIVersion == "" {
-			newHPA.TypeMeta.APIVersion = autoscalingv2.SchemeGroupVersion.String()
-		}
-
-		// convert hpa to unstructured object
-		obj, err := apimachineryRuntime.DefaultUnstructuredConverter.ToUnstructured(&newHPA)
-		if err != nil {
-			return nil, err
-		}
-		u := unstructured.Unstructured{}
-		u.SetUnstructuredContent(obj)
-		newHPAs = append(newHPAs, u)
-	}
-
-	return newHPAs, nil
-}
+// disabled HPA creation until we resolve the bug
+// BUG: https://github.com/tektoncd/operator/issues/2002
+// func (ot *OptionsTransformer) createHorizontalPodAutoscalers(manifest *mf.Manifest, targetNamespace string, additionalOptions v1alpha1.AdditionalOptions) ([]unstructured.Unstructured, error) {
+// 	newHPAs := []unstructured.Unstructured{}
+// 	existingHPAs := manifest.Filter(mf.Any(mf.ByKind(KindHorizontalPodAutoscaler)))
+// 	for hpaName, newHPA := range additionalOptions.HorizontalPodAutoscalers {
+// 		found := false
+// 		for _, resource := range existingHPAs.Resources() {
+// 			if resource.GetName() == hpaName {
+// 				found = true
+// 				break
+// 			}
+// 		}
+// 		if found {
+// 			continue
+// 		}
+//
+// 		// update name
+// 		newHPA.SetName(hpaName)
+//
+// 		// update the namespace to targetNamespace
+// 		newHPA.SetNamespace(targetNamespace)
+//
+// 		// update kind
+// 		if newHPA.TypeMeta.Kind == "" {
+// 			newHPA.TypeMeta.Kind = KindHorizontalPodAutoscaler
+// 		}
+//
+// 		// update api version
+// 		if newHPA.TypeMeta.APIVersion == "" {
+// 			newHPA.TypeMeta.APIVersion = autoscalingv2.SchemeGroupVersion.String()
+// 		}
+//
+// 		// convert hpa to unstructured object
+// 		obj, err := apimachineryRuntime.DefaultUnstructuredConverter.ToUnstructured(&newHPA)
+// 		if err != nil {
+// 			return nil, err
+// 		}
+// 		u := unstructured.Unstructured{}
+// 		u.SetUnstructuredContent(obj)
+// 		newHPAs = append(newHPAs, u)
+// 	}
+//
+// 	return newHPAs, nil
+// }


Test patch:
diff --git a/pkg/reconciler/common/testdata/test-additional-options-base-hpa.yaml b/pkg/reconciler/common/testdata/test-additional-options-base-hpa.yaml
index 0b8471343..aea1dbb49 100644
--- a/pkg/reconciler/common/testdata/test-additional-options-base-hpa.yaml
+++ b/pkg/reconciler/common/testdata/test-additional-options-base-hpa.yaml
@@ -30,7 +30,7 @@ apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
   creationTimestamp: null
-  name: test-max-replicas
+  name: test-metrics
   namespace: tekton-pipelines
 spec:
   scaleTargetRef:
@@ -49,3 +49,53 @@ spec:
 status:
   currentMetrics: null
   desiredReplicas: 0
+
+---
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  creationTimestamp: null
+  name: test-max-replicas
+  namespace: tekton-pipelines
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: bar
+  minReplicas: 3
+  maxReplicas: 5
+  metrics:
+    - resource:
+        name: cpu
+        target:
+          averageUtilization: 80
+          type: Utilization
+      type: Resource
+status:
+  currentMetrics: null
+  desiredReplicas: 0
+
+---
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  creationTimestamp: null
+  name: test-min-replicas
+  namespace: tekton-pipelines
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: bar
+  minReplicas: 1
+  maxReplicas: 5
+  metrics:
+    - resource:
+        name: cpu
+        target:
+          averageUtilization: 80
+          type: Utilization
+      type: Resource
+status:
+  currentMetrics: null
+  desiredReplicas: 0
diff --git a/pkg/reconciler/common/testdata/test-additional-options-test-hpa.yaml b/pkg/reconciler/common/testdata/test-additional-options-test-hpa.yaml
index 40c8f2fb1..648bf3299 100644
--- a/pkg/reconciler/common/testdata/test-additional-options-test-hpa.yaml
+++ b/pkg/reconciler/common/testdata/test-additional-options-test-hpa.yaml
@@ -39,6 +39,30 @@ status:
 ---
 apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
+metadata:
+  creationTimestamp: null
+  name: test-metrics
+  namespace: tekton-pipelines
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: bar
+  minReplicas: 0
+  maxReplicas: 1
+  metrics:
+    - resource:
+        name: cpu
+        target:
+          averageUtilization: 80
+          type: Utilization
+      type: Resource
+status:
+  currentMetrics: null
+  desiredReplicas: 0
+---
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
 metadata:
   creationTimestamp: null
   name: test-max-replicas
@@ -48,8 +72,8 @@ spec:
     apiVersion: apps/v1
     kind: Deployment
     name: bar
-  minReplicas: 1
-  maxReplicas: 2
+  minReplicas: 3
+  maxReplicas: 9
   metrics:
     - resource:
         name: cpu
@@ -66,26 +90,53 @@ apiVersion: autoscaling/v2
 kind: HorizontalPodAutoscaler
 metadata:
   creationTimestamp: null
-  annotations:
-    foo: bar
-  labels:
-    foo: bar
-  name: new-hpa
+  name: test-min-replicas
   namespace: tekton-pipelines
 spec:
   scaleTargetRef:
     apiVersion: apps/v1
     kind: Deployment
-    name: foo
+    name: bar
   minReplicas: 2
   maxReplicas: 5
   metrics:
     - resource:
         name: cpu
         target:
-          averageUtilization: 100
+          averageUtilization: 80
           type: Utilization
       type: Resource
 status:
   currentMetrics: null
   desiredReplicas: 0
+
+# disabled HPA creation until we resolve the bug
+# BUG: https://github.com/tektoncd/operator/issues/2002
+# ---
+# apiVersion: autoscaling/v2
+# kind: HorizontalPodAutoscaler
+# metadata:
+#   creationTimestamp: null
+#   annotations:
+#     foo: bar
+#   labels:
+#     foo: bar
+#   name: new-hpa
+#   namespace: tekton-pipelines
+# spec:
+#   scaleTargetRef:
+#     apiVersion: apps/v1
+#     kind: Deployment
+#     name: foo
+#   minReplicas: 2
+#   maxReplicas: 5
+#   metrics:
+#     - resource:
+#         name: cpu
+#         target:
+#           averageUtilization: 100
+#           type: Utilization
+#       type: Resource
+# status:
+#   currentMetrics: null
+#   desiredReplicas: 0
diff --git a/pkg/reconciler/common/transformer_additional_options_test.go b/pkg/reconciler/common/transformer_additional_options_test.go
index c7a8798ce..24371a97a 100644
--- a/pkg/reconciler/common/transformer_additional_options_test.go
+++ b/pkg/reconciler/common/transformer_additional_options_test.go
@@ -503,7 +503,7 @@ func TestExecuteAdditionalOptionsTransformer(t *testing.T) {
 							},
 						},
 					},
-					"test-max-replicas": {
+					"test-metrics": {
 						Spec: autoscalingv2.HorizontalPodAutoscalerSpec{
 							ScaleTargetRef: autoscalingv2.CrossVersionObjectReference{
 								APIVersion: "apps/v1",
@@ -526,6 +526,18 @@ func TestExecuteAdditionalOptionsTransformer(t *testing.T) {
 							},
 						},
 					},
+					"test-max-replicas": {
+						Spec: autoscalingv2.HorizontalPodAutoscalerSpec{
+							MinReplicas: nil,
+							MaxReplicas: 9,
+						},
+					},
+					"test-min-replicas": {
+						Spec: autoscalingv2.HorizontalPodAutoscalerSpec{
+							MinReplicas: ptr.Int32(2),
+							MaxReplicas: 0,
+						},
+					},
 				},
 			},
 			inputFilename:          "./testdata/test-additional-options-base-hpa.yaml",