-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
Run rbac createresources for namespaces concurrently to avoid slowness #2218
Conversation
/retest |
The following is the coverage report on the affected files.
|
@jkandasa @piyush-garg can you lgtm please ? |
The following is the coverage report on the affected files.
|
@jkandasa @piyush-garg Please have a look to my PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkhelil Thanks for the PR
LGTM
@piyush-garg @vdemeester can you please review? |
@@ -393,54 +396,73 @@ func (r *rbac) createResources(ctx context.Context) error { | |||
return err | |||
} | |||
|
|||
var wg sync.WaitGroup | |||
sem := make(chan struct{}, rbacMaxConcurrentCalls) // Semaphore with buffer size of rbacMaxConcurrentCalls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this if we use sync.WaitGroup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdemeester I am using semaphore to set a maximum number of concurrent goroutine to avoid overloading the api server as processResourcesForSingleNamespace is doing a couple of calls to apiserver
I think there is noneed to the waitgroup, removing the waitgroup as it is useless
My bad we need the wait group to ensure that createResource doesnt return before the goroutines finishes their job
and we use the semaphore to set maximum number of concurrent goroutines
/test pull-tekton-operator-build-tests |
|
||
// if No error add `openshift-pipelines.tekton.dev/namespace-reconcile-version` label to namespace | ||
// so that rbac won't loop on it again | ||
if err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err will be nil right if above function failed but last one succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thank you for catching this @piyush-garg, proposing a fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that might be a data race using the shared logger stored in the context with
logger := logging.FromContext(ctx), it returns a zaplogger that is shared between goroutines and fortunately the issue happened on one of the tests
https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_operator/2218/pull-tekton-operator-unit-tests/1809201707154935808/build-log.txt
I am now creating a logger for each goroutine
094536f
to
1aeb77c
Compare
The following is the coverage report on the affected files.
|
/test pull-tekton-operator-build-tests |
5 similar comments
/test pull-tekton-operator-build-tests |
/test pull-tekton-operator-build-tests |
/test pull-tekton-operator-build-tests |
/test pull-tekton-operator-build-tests |
/test pull-tekton-operator-build-tests |
/test pull-tekton-operator-build-tests |
The following is the coverage report on the affected files.
|
/retest |
The following is the coverage report on the affected files.
|
} | ||
} else { | ||
logger.Errorf("failed to reconcile namespace %s, %w", ns.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small thought like, dont we need to requeue the request or something to process it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, i ve added an error chan that capture the error and return it to main createResource. createResource returns the error to the caller, and request is then requeued
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah ohk fine
@@ -0,0 +1,179 @@ | |||
package tektonconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add license header
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
I am in this situation, thanks for this pull request 👍 |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/approve cc @jkandasa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkandasa, piyush-garg, vdemeester 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 |
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lint
before submitting a PRSee the contribution guide for more details.
Release Notes