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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

make rbac max concurrent calls configurable #2331

wants to merge 1 commit into from

Conversation

jkhelil
Copy link
Member

@jkhelil jkhelil commented Sep 17, 2024 •
edited
Loading

Changes

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

@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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester after the PR has been reviewed.
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found 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 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.

jkandasa
jkandasa reviewed Sep 17, 2024
View reviewed changes
pkg/reconciler/openshift/tektonconfig/rbac.go Outdated
return defaultRbacMaxConcurrentCalls
}
if parsedValue, err := strconv.Atoi(envValue); err == nil {
return parsedValue
Copy link
Member

@jkandasa jkandasa Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jkhelil please do some range verification, for example: allow only 1 to 100.
do not allow, 0, -1, 10000, etc.,

jkandasa
jkandasa reviewed Sep 17, 2024
View reviewed changes
pkg/reconciler/openshift/tektonconfig/rbac.go Outdated
return parsedValue
}
return defaultRbacMaxConcurrentCalls
}
Copy link
Member

@jkandasa jkandasa Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

please write a unit test for this function: getRBACMaxCalls

jkandasa
jkandasa 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 {
envValue := os.Getenv("OCP_RBAC_MAX_CONCURRENT_CALLS")
Copy link
Member

@jkandasa jkandasa Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jkhelil keep the environment variable key as a constant in https://github.com/tektoncd/operator/blob/main/pkg/reconciler/openshift/const.go

jkandasa
jkandasa reviewed Sep 17, 2024
View reviewed changes
pkg/reconciler/openshift/tektonconfig/rbac.go Outdated
if envValue == "" {
return defaultRbacMaxConcurrentCalls
}
if parsedValue, err := strconv.Atoi(envValue); err == nil {
Copy link
Member

@jkandasa jkandasa Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jkhelil if the error is not nil, print the error in the log and say, taking the default value: 20
also print a info message, about what value used finally

@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%

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 left review comments

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

@PuneetPunamiya PuneetPunamiya Awaiting requested review from PuneetPunamiya

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesnt merit a release note. 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 @khrm @jkandasa