Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make rbac max concurrent calls configurable #2331

Merged
merged 2 commits into from
Sep 23, 2024
Merged

make rbac max concurrent calls configurable #2331

merged 2 commits into from
Sep 23, 2024

Conversation

jkhelil
Copy link
Member

@jkhelil jkhelil commented Sep 17, 2024 •
edited by jkandasa
Loading

Changes

Improvements for Namespace RBAC Reconciliation on OpenShift

  • Pre-Reconciliation Check: Implement a check to verify if there are any namespaces to reconcile. If none are found, cancel the reconciliation process early.
  • Adjust Worker Count: Modify the number of workers handling namespace reconciliation to optimize performance.
  • Retry Mechanism for ClusterRoleBinding Updates: Introduce a retry mechanism for updating shared resources like ClusterRoleBinding to prevent job failures caused by concurrent update conflicts.
  • Environment Variable for Concurrency Control: Add an environment variable OCP_RBAC_MAX_CONCURRENT_CALLS to configure the maximum number of concurrent calls for namespace RBAC reconciliation. This is particularly useful for clusters with a large number of namespaces.
  • default concurrent calls: 20
  • supported range: 1 to 50

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Release Notes

Add an environment variable OCP_RBAC_MAX_CONCURRENT_CALLS to configure the maximum number of concurrent calls for namespace RBAC reconciliation.
This is particularly useful for clusters with a large number of namespaces.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 17, 2024
@tekton-robot tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 17, 2024
@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 269de8f to 4f52692 Compare September 17, 2024 13:57
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2024
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/rbac.go 45.2% 45.4% 0.1

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 17, 2024
@jkhelil
Copy link
Member Author

jkhelil commented Sep 17, 2024

/release-note-none

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 4f52692 to 1cc8aa8 Compare September 17, 2024 16:52
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/rbac.go 45.2% 45.3% 0.1

khrm
khrm reviewed Sep 17, 2024
View reviewed changes
pkg/reconciler/openshift/tektonconfig/rbac.go Outdated
@@ -376,6 +378,17 @@ func (r *rbac) handleSCCInNamespace(ctx context.Context, ns *corev1.Namespace) e
return nil
}

func getRBACMaxCalls() int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing an environment variable will lead to the recreation of the pod. So this can be part of init where we change defaultRbacMaxConcurrentCalls if environment variable is provided.

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 1cc8aa8 to 37a195b Compare September 18, 2024 13:35
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2024
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%

@jkhelil
Copy link
Member Author

jkhelil commented Sep 19, 2024

@khrm @jkandasa Can you have a look please

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 37a195b to 851b661 Compare September 19, 2024 11:58
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 851b661 to adb5b4d Compare September 19, 2024 13:37
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from adb5b4d to d3f97a8 Compare September 20, 2024 06:49
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from d3f97a8 to 09274f0 Compare September 20, 2024 15:17
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%
pkg/reconciler/openshift/tektonconfig/rbac.go 45.2% 45.3% 0.0

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 09274f0 to 2d0709a Compare September 20, 2024 18:55
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%
pkg/reconciler/openshift/tektonconfig/rbac.go 45.2% 45.3% 0.0

@jkhelil jkhelil force-pushed the incraese_rbac_concurrent_goroutines branch from 2d0709a to 96f6962 Compare September 22, 2024 08:31
@tekton-robot
Copy link
Contributor

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/openshift/tektonconfig/init.go Do not exist 100.0%
pkg/reconciler/openshift/tektonconfig/rbac.go 45.2% 45.6% 0.3

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2024
@jkandasa
Copy link
Member

jkandasa commented Sep 22, 2024 •
edited
Loading

@jkhelil in the release notes, please describe about the new environment variable
also include the description about the PR

@jkandasa jkandasa added the platform/openshift This issue targets the OpenShift Platform label Sep 22, 2024
@jkandasa
Copy link
Member

/cherry-pick release-v0.73.x

@tekton-robot
Copy link
Contributor

@jkandasa: once the present PR merges, I will cherry-pick it on top of release-v0.73.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.73.x

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.

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Sep 23, 2024
@jkandasa
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
@tekton-robot tekton-robot merged commit 4769713 into tektoncd:main Sep 23, 2024
7 of 8 checks passed
@tekton-robot
Copy link
Contributor

@jkandasa: new pull request created: #2337

In response to this:

/cherry-pick release-v0.73.x

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.

@tekton-robot tekton-robot mentioned this pull request Sep 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khrm khrm khrm left review comments

@jkandasa jkandasa jkandasa approved these changes

@piyush-garg piyush-garg Awaiting requested review from piyush-garg

@PuneetPunamiya PuneetPunamiya Awaiting requested review from PuneetPunamiya

Assignees

@jkandasa jkandasa

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/openshift This issue targets the OpenShift Platform release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkhelil @tekton-robot @jkandasa @khrm