refactor namespace reconciliation to ensure openshift rbac requisites

[anithapriyanatarajan]

Changes

During upgrade of pipelines in OpenShift cluster there are additional rbac related reconciliation to be done across all namespaces in the cluster. Right now the namespace reconciliation happens one at a time in a for loop. This results in longer upgrade time. This PR improvises this by

• Utilising Patch in place of Update
• Refactor to improve code modularity
• Bulk update of ClusterInterceptor ClusterRoleBinding(CRB) with namespaces that are processed succesfully instead of updating the CRB everytime for each namespace
  [Note: The original scope included concurrency which was later dropped since there was no significant impact in time taken to process the namespaces]

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
• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

NONE

[tekton-robot]

Hi @anithapriyanatarajan. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pratap0007]

/ok-to-test

[vdemeester]

Naive question (on this and in some other places in general), shouldn't we use PATCH more often ? It would reduce the possibility to get "this object ways already update" type of failures.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	85.7%	83.3%	-2.4

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	85.7%	83.3%	-2.4

[jkhelil]

@anithapriyanatarajan have you tested upgrade scenario with this PR ?

• Clone Operatror from main branch
• install Operator make apply TARGET=openshift
• git checkout your current branch
• change VERSION to someother value in config/openshift/base/operator.yaml file
• make apply TARGET=openshift again
  you would eventually,
• create a high number of namespaces (before the first make apply)
• save and compare clusterinterceptore role binding object before and after the upgrade
• other checks ...?

[anithapriyanatarajan]

@anithapriyanatarajan have you tested upgrade scenario with this PR ?

• Clone Operatror from main branch
• install Operator make apply TARGET=openshift
• git checkout your current branch
• change VERSION to someother value in config/openshift/base/operator.yaml file
• make apply TARGET=openshift again
  you would eventually,
• create a high number of namespaces (before the first make apply)
• save and compare clusterinterceptore role binding object before and after the upgrade
• other checks ...?

@jkhelil - Thanks for the detailed comments. The following steps were performed to test the upgrade.

1. Create new openshift cluster
2. Create a large number of namespaces.
   For testing, 500 namespaces were created with below command
   $ seq 1 500 | xargs -P 20 -I {} kubectl create namespace namespace-{}
3. Clone Operator main branch
4. Install make apply TARGET=openshift
5. Observe & capture the logs, operator pod CPU usage, tektonconfig, clusterinterceptor rolebinding yaml
6. Update the Version to random value & make apply again
7. Observe & capture the logs, operator pod CPU usage, tektonconfig, clusterinterceptor rolebinding yaml
   Key observation:
   config, rolebinding were all updated as expected. Duration for upgrade of ~500 namespaces: 2m54.381192967s
8. Check out the PR
   $ gh pr checkout 2468
9. Update the Version to new value & make apply again
10. Observe & capture the logs, operator pod CPU usage, tektonconfig, clusterinterceptor rolebinding yaml
    Key observation:
    config, rolebinding were all updated as expected. Duration for upgrade: 2m25.410893374s with concurrency of handling 75 namespaces.

Though the refactored code is working fine for upgrade, the improvement in duration is very minimal, hope that is expected. For all the cases observed CPU and memory metrics from Openshift Metrics dashboard

[jkhelil]

@anithapriyanatarajan
Have you done a test with fresh install on openshift ?
I have an issue running the PR on openshift

operator git:(openshift-rbac-concurrent-ns-reconcilation) oc get tektonconfig
NAME     VERSION   READY   REASON
config   devel     False   PreReconciliation failed with message: the body of the request was in an unknown format - accepted media types include: application/json-patch+json, application/merge-patch+json, application/apply-patch+yaml
➜

[jkhelil]

@anithapriyanatarajan

• first thanks for all the work you have done so far
• regarding your great conclusion that concurrency doesnt brings any significant enhancement, my opinion is that we should avoid employ it there, it is useless, but we should use some of your work to
• rewrite the createResource method, as you suggested (process namespace, in each own function, and so one)
• bulk update the clusterrolebnding
• use patch instead of update (be careful here, i think the issue iam facing in fresh install is may be related to this)

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	85.7%	83.3%	-2.4

[anithapriyanatarajan]

/test pull-tekton-operator-build-tests

[anithapriyanatarajan]

@anithapriyanatarajan

• first thanks for all the work you have done so far
• regarding your great conclusion that concurrency doesnt brings any significant enhancement, my opinion is that we should avoid employ it there, it is useless, but we should use some of your work to
• rewrite the createResource method, as you suggested (process namespace, in each own function, and so one)
• bulk update the clusterrolebnding
• use patch instead of update (be careful here, i think the issue iam facing in fresh install is may be related to this)

@jkhelil

Removed the concurrency and mutex aspects from the PR. The resulting changes are just meant for refactoring for modularity and incorporate enhancements to bulk update clusterrolebinidng and utilising Patch instead of Update. Please review and share your views

[anithapriyanatarajan]

@anithapriyanatarajan Have you done a test with fresh install on openshift ? I have an issue running the PR on openshift

operator git:(openshift-rbac-concurrent-ns-reconcilation) oc get tektonconfig
NAME     VERSION   READY   REASON
config   devel     False   PreReconciliation failed with message: the body of the request was in an unknown format - accepted media types include: application/json-patch+json, application/merge-patch+json, application/apply-patch+yaml
➜

Thank you very much for the catch. This issue happens when there is a fresh install of the new version in which case Patch of exising tektonconfig is triggered to update labels. I had used a deprecated type 'StrategicMergePatchType' instead of 'MergePatchTpe' which has created the issue. Fixed this now.

[anithapriyanatarajan]

/test pull-tekton-operator-build-tests

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	85.7%	83.3%	-2.4

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/utils.go	85.7%	83.3%	-2.4

[jkhelil]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkhelil

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 [jkhelil]

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

[jkhelil]

/lgtm