Update only affected replicated policies

[yiraeChristineKim]

Refactoring propagated policy controller to update only targeted replicated policies to reduce meaningless reconcile and improve performance.

Whenever there is a PlacementRule, Placement/PlacementDecision, or PlacementBinding change, it causes the root policy to be regenerated for every managed cluster, even those not affected.

This should be separated out to a separate controller in the Policy Propagator so that a placement update only affects the impacted clusters. For example, if a cluster is added to a placement, the propagated policy should generated just for that one cluster.

This is related to ACM-7332.

Ref: https://issues.redhat.com/browse/ACM-7403

[mprahl]

/hold until commits are squashed

[JustinKuli]

It mostly looks good! I'm just worried about the PolicySet situation

[mprahl]

@yiraeChristineKim, the logs are quite confusing now because the code moved in common.go for some reason has a logger with the name policy-automation. Perhaps this would be best:

diff --git a/controllers/common/ansible.go b/controllers/automation/ansible.go
similarity index 97%
rename from controllers/common/ansible.go
rename to controllers/automation/ansible.go
index 4464679..a3bf21a 100644
--- a/controllers/common/ansible.go
+++ b/controllers/automation/ansible.go
@@ -1,6 +1,6 @@
 // Copyright Contributors to the Open Cluster Management project
 
-package common
+package automation
 
 import (
        "context"
@@ -15,21 +15,17 @@ import (
        "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "k8s.io/client-go/dynamic"
-       ctrl "sigs.k8s.io/controller-runtime"
 
        policyv1beta1 "open-cluster-management.io/governance-policy-propagator/api/v1beta1"
 )
 
 const (
-       ControllerName             string = "policy-automation"
        PolicyAutomationLabel      string = "policy.open-cluster-management.io/policyautomation-name"
        PolicyAutomationGeneration string = "policy.open-cluster-management.io/policyautomation-generation"
        // policyautomation-ResouceVersion
        PolicyAutomationResouceV string = "policy.open-cluster-management.io/policyautomation-resource-version"
 )
 
-var log = ctrl.Log.WithName(ControllerName)
-
 var ansibleJobRes = schema.GroupVersionResource{
        Group: "tower.ansible.com", Version: "v1alpha1",
        Resource: "ansiblejobs",
diff --git a/controllers/automation/policyautomation_controller.go b/controllers/automation/policyautomation_controller.go
index 481d670..e7aa37c 100644
--- a/controllers/automation/policyautomation_controller.go
+++ b/controllers/automation/policyautomation_controller.go
@@ -302,7 +302,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
        }
 
        if policyAutomation.Annotations["policy.open-cluster-management.io/rerun"] == "true" {
-               AjExist, err := common.MatchPAResouceV(policyAutomation,
+               AjExist, err := MatchPAResouceV(policyAutomation,
                        r.DynamicClient, policyAutomation.GetResourceVersion())
                if err != nil {
                        log.Error(err, "Failed to compare Ansible job's resourceVersion")
@@ -323,7 +323,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
 
                violationContext, _ := r.getViolationContext(ctx, policy, targetList, policyAutomation)
 
-               err = common.CreateAnsibleJob(
+               err = CreateAnsibleJob(
                        policyAutomation,
                        r.DynamicClient,
                        "manual",
@@ -373,7 +373,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
                        if len(targetList) > 0 {
                                log.Info("Creating An Ansible job", "targetList", targetList)
                                violationContext, _ := r.getViolationContext(ctx, policy, targetList, policyAutomation)
-                               err = common.CreateAnsibleJob(policyAutomation, r.DynamicClient, "scan",
+                               err = CreateAnsibleJob(policyAutomation, r.DynamicClient, "scan",
                                        violationContext)
                                if err != nil {
                                        return reconcile.Result{RequeueAfter: requeueAfter}, err
@@ -395,7 +395,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
                        if len(targetList) > 0 {
                                log.Info("Creating an Ansible job", "targetList", targetList)
 
-                               AjExist, err := common.MatchPAGeneration(policyAutomation,
+                               AjExist, err := MatchPAGeneration(policyAutomation,
                                        r.DynamicClient, policyAutomation.GetGeneration())
                                if err != nil {
                                        log.Error(err, "Failed to get Ansible job's generation")
@@ -406,7 +406,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
                                        return reconcile.Result{}, nil
                                }
                                violationContext, _ := r.getViolationContext(ctx, policy, targetList, policyAutomation)
-                               err = common.CreateAnsibleJob(
+                               err = CreateAnsibleJob(
                                        policyAutomation,
                                        r.DynamicClient,
                                        string(policyv1beta1.Once),
@@ -517,7 +517,7 @@ func (r *PolicyAutomationReconciler) Reconcile(
                                }
                                log.Info("Creating An Ansible job", "trimmedTargetList", trimmedTargetList)
                                violationContext, _ := r.getViolationContext(ctx, policy, trimmedTargetList, policyAutomation)
-                               err = common.CreateAnsibleJob(
+                               err = CreateAnsibleJob(
                                        policyAutomation,
                                        r.DynamicClient,
                                        string(policyv1beta1.EveryEvent),
diff --git a/controllers/common/common.go b/controllers/common/common.go
index cc40cb8..60d1b68 100644
--- a/controllers/common/common.go
+++ b/controllers/common/common.go
@@ -15,6 +15,7 @@ import (
        clusterv1 "open-cluster-management.io/api/cluster/v1"
        clusterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
        appsv1 "open-cluster-management.io/multicloud-operators-subscription/pkg/apis/apps/placementrule/v1"
+       ctrl "sigs.k8s.io/controller-runtime"
        "sigs.k8s.io/controller-runtime/pkg/client"
        "sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -29,7 +30,10 @@ const (
        RootPolicyLabel       string = APIGroup + "/root-policy"
 )
 
-var ErrInvalidLabelValue = errors.New("unexpected format of label value")
+var (
+       log                  = ctrl.Log.WithName("common")
+       ErrInvalidLabelValue = errors.New("unexpected format of label value")
+)
 
 // IsInClusterNamespace check if policy is in cluster namespace
 func IsInClusterNamespace(c client.Client, ns string) (bool, error) {

[mprahl]

This looks good to me! Could you please squash your commits since the last commit now includes other refactoring work/improvements?

@JustinKuli , could you please take a look on Monday morning?

[JustinKuli]

/unhold

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl, yiraeChristineKim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JustinKuli,mprahl,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment