Run rbac createresources for namespaces concurrently to avoid slowness #2218

jkhelil · 2024-06-19T08:26:18Z

Changes

Process rbac resource creation for openshift namespace reconciliation concurrently to avoid slowness on clusters with high number of namespace using a workerpool
Avoid blocking the reconciliation loop on error, when errors happen on namespace rbac resource reconciliation for openshift
Add unit tests to createRessource in rbac.go

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

jkandasa · 2024-06-21T04:25:20Z

/retest

tekton-robot · 2024-06-21T08:46:22Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	43.4%	43.4

jkhelil · 2024-06-21T09:14:56Z

@jkandasa @piyush-garg can you lgtm please ?

tekton-robot · 2024-06-24T08:53:37Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	43.4%	43.4

jkhelil · 2024-06-26T11:44:57Z

@jkandasa @piyush-garg Please have a look to my PR

jkandasa

@jkhelil Thanks for the PR
LGTM

jkandasa · 2024-07-03T07:20:02Z

@piyush-garg @vdemeester can you please review?

vdemeester · 2024-07-03T08:46:25Z

pkg/reconciler/openshift/tektonconfig/rbac.go

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


Why do we need this if we use sync.WaitGroup ?

@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

jkhelil · 2024-07-03T15:47:28Z

/test pull-tekton-operator-build-tests

piyush-garg · 2024-07-05T10:31:30Z

pkg/reconciler/openshift/tektonconfig/rbac.go

+
+	// 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 {


err will be nil right if above function failed but last one succeed?

Yeah, thank you for catching this @piyush-garg, proposing a fix

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

tekton-robot · 2024-07-05T13:49:57Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	42.4%	42.4

jkhelil · 2024-07-05T14:04:12Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-05T14:14:21Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-05T14:23:58Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-05T16:13:48Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-06T15:06:06Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-07T07:14:50Z

/test pull-tekton-operator-build-tests

jkhelil · 2024-07-08T07:39:23Z

/test pull-tekton-operator-build-tests

tekton-robot · 2024-07-10T06:48:39Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	42.4%	42.4

piyush-garg · 2024-07-13T21:23:54Z

/retest

tekton-robot · 2024-07-22T07:34:23Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	42.4%	42.4

piyush-garg · 2024-07-24T10:17:09Z

pkg/reconciler/openshift/tektonconfig/rbac.go

 		}
+	} else {
+		logger.Errorf("failed to reconcile namespace %s, %w", ns.Name, err)


just a small thought like, dont we need to requeue the request or something to process it again?

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

aah ohk fine

piyush-garg · 2024-07-24T10:17:25Z

pkg/reconciler/openshift/tektonconfig/rbac_test.go

@@ -0,0 +1,179 @@
+package tektonconfig


add license header

tekton-robot · 2024-07-24T11:58:00Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	43.1%	43.1

tekton-robot · 2024-07-24T13:16:43Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	45.0%	45.0

Diliz · 2024-07-25T06:51:00Z

Changes

Process rbac resource creation for openshift namespace reconciliation concurrently to avoid slowness on clusters with high number of namespace

Avoid blocking the reconciliation loop on error, when errors happen on namespace rbac resource reconciliation for openshift

I am in this situation, thanks for this pull request 👍

tekton-robot · 2024-07-26T07:16:28Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	45.0%	45.0

tekton-robot · 2024-07-26T07:50:04Z

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/common.go	0.0%	54.8%	54.8
pkg/reconciler/openshift/tektonconfig/rbac.go	0.0%	45.2%	45.2

piyush-garg · 2024-07-29T18:49:59Z

/approve

cc @jkandasa

jkandasa

/lgtm

tekton-robot · 2024-07-30T02:35:53Z

[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:

~~OWNERS~~ [jkandasa,piyush-garg,vdemeester]

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

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 19, 2024

tekton-robot requested review from piyush-garg and PuneetPunamiya June 19, 2024 08:26

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 19, 2024

jkhelil force-pushed the SRVKP-2279_2 branch from b326db8 to 4802823 Compare June 21, 2024 08:42

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2024

jkhelil force-pushed the SRVKP-2279_2 branch from 4802823 to e21a15d Compare June 24, 2024 08:49

jkandasa approved these changes Jul 2, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2024

vdemeester reviewed Jul 3, 2024

View reviewed changes

jkhelil force-pushed the SRVKP-2279_2 branch from e21a15d to 3a7df95 Compare July 3, 2024 12:04

vdemeester approved these changes Jul 5, 2024

View reviewed changes

piyush-garg reviewed Jul 5, 2024

View reviewed changes

jkhelil force-pushed the SRVKP-2279_2 branch 2 times, most recently from 094536f to 1aeb77c Compare July 5, 2024 13:44

jkhelil force-pushed the SRVKP-2279_2 branch from 1aeb77c to dc4e7ac Compare July 10, 2024 06:43

missing run go mod vendor

f350354

jkhelil force-pushed the SRVKP-2279_2 branch from dc4e7ac to 7ee6c1d Compare July 22, 2024 07:26

piyush-garg reviewed Jul 24, 2024

View reviewed changes

jkhelil force-pushed the SRVKP-2279_2 branch from 7ee6c1d to 8b144c7 Compare July 24, 2024 11:51

jkhelil force-pushed the SRVKP-2279_2 branch from 8b144c7 to aa223ce Compare July 24, 2024 13:10

jkhelil force-pushed the SRVKP-2279_2 branch from aa223ce to 2ec9190 Compare July 26, 2024 07:10

Run rbac createresources for namespaces concurrently to avoid slowness

f753a79

jkhelil force-pushed the SRVKP-2279_2 branch from 2ec9190 to f753a79 Compare July 26, 2024 07:44

jkandasa approved these changes Jul 30, 2024

View reviewed changes

tekton-robot assigned jkandasa Jul 30, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2024

tekton-robot merged commit ea1a142 into tektoncd:main Jul 30, 2024
8 checks passed

Run rbac createresources for namespaces concurrently to avoid slowness #2218

Run rbac createresources for namespaces concurrently to avoid slowness #2218

Conversation

jkhelil commented Jun 19, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

jkandasa commented Jun 21, 2024

tekton-robot commented Jun 21, 2024

jkhelil commented Jun 21, 2024

tekton-robot commented Jun 24, 2024

jkhelil commented Jun 26, 2024

jkandasa left a comment

Choose a reason for hiding this comment

jkandasa commented Jul 3, 2024

vdemeester Jul 3, 2024

Choose a reason for hiding this comment

jkhelil Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

jkhelil commented Jul 3, 2024

piyush-garg Jul 5, 2024

Choose a reason for hiding this comment

jkhelil Jul 5, 2024

Choose a reason for hiding this comment

jkhelil Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jul 5, 2024

jkhelil commented Jul 5, 2024

jkhelil commented Jul 5, 2024

jkhelil commented Jul 5, 2024

jkhelil commented Jul 5, 2024

jkhelil commented Jul 6, 2024

jkhelil commented Jul 7, 2024

jkhelil commented Jul 8, 2024

tekton-robot commented Jul 10, 2024

piyush-garg commented Jul 13, 2024

tekton-robot commented Jul 22, 2024

piyush-garg Jul 24, 2024

Choose a reason for hiding this comment

jkhelil Jul 24, 2024

Choose a reason for hiding this comment

piyush-garg Jul 25, 2024

Choose a reason for hiding this comment

piyush-garg Jul 24, 2024

Choose a reason for hiding this comment

tekton-robot commented Jul 24, 2024

tekton-robot commented Jul 24, 2024

Diliz commented Jul 25, 2024

Changes

tekton-robot commented Jul 26, 2024

tekton-robot commented Jul 26, 2024

piyush-garg commented Jul 29, 2024

jkandasa left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 30, 2024

jkhelil commented Jun 19, 2024 •

edited

Loading

jkhelil Jul 3, 2024 •

edited

Loading

jkhelil Jul 5, 2024 •

edited

Loading