Address review feedback

bundle/manifests/flows.netobserv.io_flowcollectors.yaml

@@ -8033,8 +8033,8 @@ spec:
                   type: object
                 type: array
               namespace:
-                description: Namespace where console plugin and flowlogs-pipeline
-                  have been deployed.
+                description: 'Namespace where console plugin and flowlogs-pipeline
+                  have been deployed. Deprecated: annotations are used instead'
                 type: string
             required:
             - conditions

config/crd/bases/flows.netobserv.io_flowcollectors.yaml

@@ -8019,8 +8019,8 @@ spec:
                   type: object
                 type: array
               namespace:
-                description: Namespace where console plugin and flowlogs-pipeline
-                  have been deployed.
+                description: 'Namespace where console plugin and flowlogs-pipeline
+                  have been deployed. Deprecated: annotations are used instead'
                 type: string
             required:
             - conditions

controllers/flp/flp_controller.go

@@ -64,6 +64,8 @@ type subReconciler interface {
 	getStatus() *status.Instance
 }
 
+// Reconcile is the controller entry point for reconciling current state with desired state.
+// It manages the controller status at a high level. Business logic is delegated into `reconcile`.
 func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
 	l := log.Log.WithName("flp") // clear context (too noisy)
 	ctx = log.IntoContext(ctx, l)
@@ -114,6 +116,8 @@ func (r *Reconciler) reconcile(ctx context.Context) error {
 	}
 
 	// Create sub-reconcilers
+	// TODO: refactor to move these subReconciler allocations in `Start`. It will involve some decoupling work, as currently
+	// `reconcilers.Common` is dependent on the FlowCollector object, which isn't known at start time.
 	reconcilers := []subReconciler{
 		newMonolithReconciler(cmn.NewInstance(r.mgr.Config.FlowlogsPipelineImage, r.mgr.Status.ForComponent(status.FLPMonolith))),
 		newTransformerReconciler(cmn.NewInstance(r.mgr.Config.FlowlogsPipelineImage, r.mgr.Status.ForComponent(status.FLPTransformOnly))),

controllers/flp/flp_test.go

@@ -370,7 +370,7 @@ func TestDeploymentNoChange(t *testing.T) {
 	second := b.deployment(annotate(digest))
 
 	report := helper.NewChangeReport("")
-	assert.False(helper.DeploymentChanged(first, second, constants.FLPName, helper.HPADisabled(&cfg.Processor.KafkaConsumerAutoscaler), *cfg.Processor.KafkaConsumerReplicas, &report))
+	assert.False(helper.DeploymentChanged(first, second, constants.FLPName, !helper.HPAEnabled(&cfg.Processor.KafkaConsumerAutoscaler), *cfg.Processor.KafkaConsumerReplicas, &report))
 	assert.Contains(report.String(), "no change")
 }
 
@@ -394,7 +394,7 @@ func TestDeploymentChanged(t *testing.T) {
 
 	report := helper.NewChangeReport("")
 	checkChanged := func(old, new *appsv1.Deployment, spec flowslatest.FlowCollectorSpec) bool {
-		return helper.DeploymentChanged(old, new, constants.FLPName, helper.HPADisabled(&spec.Processor.KafkaConsumerAutoscaler), *spec.Processor.KafkaConsumerReplicas, &report)
+		return helper.DeploymentChanged(old, new, constants.FLPName, !helper.HPAEnabled(&spec.Processor.KafkaConsumerAutoscaler), *spec.Processor.KafkaConsumerReplicas, &report)
 	}
 
 	assert.True(checkChanged(first, second, cfg))
@@ -475,7 +475,7 @@ func TestDeploymentChangedReplicasNoHPA(t *testing.T) {
 	second := b.deployment(annotate(digest))
 
 	report := helper.NewChangeReport("")
-	assert.True(helper.DeploymentChanged(first, second, constants.FLPName, helper.HPADisabled(&cfg2.Processor.KafkaConsumerAutoscaler), *cfg2.Processor.KafkaConsumerReplicas, &report))
+	assert.True(helper.DeploymentChanged(first, second, constants.FLPName, !helper.HPAEnabled(&cfg2.Processor.KafkaConsumerAutoscaler), *cfg2.Processor.KafkaConsumerReplicas, &report))
 	assert.Contains(report.String(), "Replicas changed")
 }
 

controllers/monitoring/monitoring_controller.go

@@ -39,6 +39,8 @@ func Start(ctx context.Context, mgr *manager.Manager) error {
 		Complete(&r)
 }
 
+// Reconcile is the controller entry point for reconciling current state with desired state.
+// It manages the controller status at a high level. Business logic is delegated into `reconcile`.
 func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
 	l := log.Log.WithName("monitoring") // clear context (too noisy)
 	ctx = log.IntoContext(ctx, l)

controllers/reconcilers/reconcilers.go

@@ -168,23 +168,22 @@ func ReconcileDeployment(ctx context.Context, ci *Instance, old, new *appsv1.Dep
 		return ci.CreateOwned(ctx, new)
 	}
 	ci.Status.CheckDeploymentProgress(old)
-	if helper.DeploymentChanged(old, new, containerName, helper.HPADisabled(hpa), replicas, report) {
+	if helper.DeploymentChanged(old, new, containerName, !helper.HPAEnabled(hpa), replicas, report) {
 		return ci.UpdateIfOwned(ctx, old, new)
 	}
 	return nil
 }
 
 func ReconcileHPA(ctx context.Context, ci *Instance, old, new *ascv2.HorizontalPodAutoscaler, desired *flowslatest.FlowCollectorHPA, report *helper.ChangeReport) error {
 	// Delete or Create / Update Autoscaler according to HPA option
-	if helper.HPADisabled(desired) {
-		ci.Managed.TryDelete(ctx, old)
-	} else {
+	if helper.HPAEnabled(desired) {
 		if !ci.Managed.Exists(old) {
 			return ci.CreateOwned(ctx, new)
 		} else if helper.AutoScalerChanged(old, *desired, report) {
 			return ci.UpdateIfOwned(ctx, old, new)
 		}
 	}
+	ci.Managed.TryDelete(ctx, old)
 	return nil
 }
 

docs/FlowCollector.md

@@ -14049,7 +14049,7 @@ ResourceClaim references one entry in PodSpec.ResourceClaims.
         <td><b>namespace</b></td>
         <td>string</td>
         <td>
-          Namespace where console plugin and flowlogs-pipeline have been deployed.<br/>
+          Namespace where console plugin and flowlogs-pipeline have been deployed. Deprecated: annotations are used instead<br/>
         </td>
         <td>false</td>
       </tr></tbody>

hack/cloned.flows.netobserv.io_flowcollectors.yaml

@@ -5567,7 +5567,7 @@ spec:
                     type: object
                   type: array
                 namespace:
-                  description: Namespace where console plugin and flowlogs-pipeline have been deployed.
+                  description: 'Namespace where console plugin and flowlogs-pipeline have been deployed. Deprecated: annotations are used instead'
                   type: string
               required:
                 - conditions

pkg/helper/flowcollector.go

@@ -37,10 +37,6 @@ func HasKafkaExporter(spec *flowslatest.FlowCollectorSpec) bool {
 	return false
 }
 
-func HPADisabled(spec *flowslatest.FlowCollectorHPA) bool {
-	return spec == nil || spec.Status == flowslatest.HPAStatusDisabled
-}
-
 func HPAEnabled(spec *flowslatest.FlowCollectorHPA) bool {
 	return spec != nil && spec.Status == flowslatest.HPAStatusEnabled
 }