Use an Informer to list LimitRanges 🥼 #4234

vdemeester · 2021-09-16T09:50:38Z

Changes

Using a LimitRange lister here instead, so this doesn't end up hitting
the real API server on each call.

Taking into account a review :
#4176 (comment).

Signed-off-by: Vincent Demeester [email protected]

/kind cleanup
/cc @imjasonh @bobcatfish @pritidesai @sbwsg @afrittoli

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

NONE

tekton-robot · 2021-09-16T10:25:47Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	90.9%	-1.9

tekton-robot · 2021-09-16T10:34:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.2%	0.3
test/controller.go	24.5%	23.3%	-1.2

vdemeester · 2021-09-16T11:11:27Z

I am missing something very obvious in the unit tests.. but.. for some reason I don't see it.. 😅

imjasonh

/approve

pkg/internal/limitrange/transformer_test.go

tekton-robot · 2021-09-16T13:58:16Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.2%	0.3

tekton-robot · 2021-09-16T14:19:22Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	90.9%	-1.9
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2

tekton-robot · 2021-09-16T14:29:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.2%	0.3

tekton-robot · 2021-09-16T15:08:45Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.2%	0.3

tekton-robot · 2021-09-16T15:11:12Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.2%	0.3

tekton-robot · 2021-09-16T15:35:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imjasonh, sbwsg

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~~ [imjasonh,sbwsg]

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

afrittoli

Thanks for this!
/lgtm

mattmoor · 2021-09-16T16:08:07Z

pkg/internal/limitrange/limitrange.go

 )

 func getVirtualLimitRange(ctx context.Context, namespace string, c kubernetes.Interface) (*corev1.LimitRange, error) {
-	limitRanges, err := c.CoreV1().LimitRanges(namespace).List(ctx, metav1.ListOptions{})
+	limitrangeInformer := limitrangeinformer.Get(ctx)


@imjasonh poked me that this PR was having trouble with the injection stuff. I'm guessing it's because this typically gets the context passed to ReconcileKind, which is NOT infused with clients and informers.

To echo my other comment, generally you would grab this off of the context passed to NewController and stash it in a field, to avoid the more "expensive" access during each reconciliation.

TBH I've debated infusing this context with things as well in the past, but until now hadn't really seen a place where it caused confusion. I think allowing that's sort of a mixed bag, and would be hard to walk back, but if you'd like to discuss it I'm open to chatting.

.... but hopefully knowing this at least unblocks you. If not, feel free to ping me!

vdemeester · 2021-09-16T16:09:51Z

/hold

vdemeester · 2021-09-16T16:39:41Z

/hold cancel

tekton-robot · 2021-09-16T16:42:21Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	90.7%	-2.2
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2

mattmoor · 2021-09-16T16:42:46Z

pkg/internal/limitrange/transformer_test.go

+		}
+	}
+	kubeclient.ClearActions()
+	return ctx, limitRangeInformer, cancel


Could consumers just use fakelimitrangeinformer.Get(ctx) instead of returning it here?

c'est pas faux 😝

c'est pas faux stuck_out_tongue_closed_eyes

Man I am tired.. "c'est pas faux" means "it's not wrong".. speaking in french while commenting.. makes this.. 😅

mattmoor

/lgtm

This LGTM, just a nit about setup if you want to preserve the signature.

mattmoor · 2021-09-16T16:44:36Z

/hold

In case you want to do the nit, otherwise feel free to remove

Using a LimitRange lister here instead, so this doesn't end up hitting the real API server on each call. Taking into account a review : tektoncd#4176 (comment). Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2021-09-16T16:56:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/internal/limitrange/limitrange.go	92.9%	93.0%	0.2
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2

mattmoor

/lgtm
/hold cancel

vdemeester added this to the Pipelines v0.28 milestone Sep 16, 2021

tekton-robot requested review from afrittoli, bobcatfish, imjasonh, pritidesai and a user September 16, 2021 09:50

tekton-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch 2 times, most recently from 069745c to 71b49ab Compare September 16, 2021 10:23

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch from 71b49ab to 5002741 Compare September 16, 2021 10:31

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2021

imjasonh reviewed Sep 16, 2021

View reviewed changes

pkg/internal/limitrange/transformer_test.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch from 5002741 to b1375f4 Compare September 16, 2021 13:51

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch from b1375f4 to db91367 Compare September 16, 2021 14:17

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch from db91367 to 38384ac Compare September 16, 2021 14:26

ghost approved these changes Sep 16, 2021

View reviewed changes

afrittoli reviewed Sep 16, 2021

View reviewed changes

tekton-robot assigned afrittoli Sep 16, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021

mattmoor reviewed Sep 16, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2021

vdemeester force-pushed the limitrange-use-lister branch from 9108729 to 183111f Compare September 16, 2021 16:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2021

mattmoor reviewed Sep 16, 2021

View reviewed changes

tekton-robot assigned mattmoor Sep 16, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2021

Use an Informer to list LimitRanges 🥼

9cd860b

Using a LimitRange lister here instead, so this doesn't end up hitting the real API server on each call. Taking into account a review : tektoncd#4176 (comment). Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the limitrange-use-lister branch from 183111f to 9cd860b Compare September 16, 2021 16:54

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2021

mattmoor reviewed Sep 16, 2021

View reviewed changes

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 16, 2021

tekton-robot merged commit adaae90 into tektoncd:main Sep 16, 2021

vdemeester deleted the limitrange-use-lister branch September 16, 2021 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an Informer to list LimitRanges 🥼 #4234

Use an Informer to list LimitRanges 🥼 #4234

vdemeester commented Sep 16, 2021 •

edited

Loading

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

vdemeester commented Sep 16, 2021

imjasonh left a comment

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

afrittoli left a comment

mattmoor Sep 16, 2021

vdemeester commented Sep 16, 2021

vdemeester commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

mattmoor Sep 16, 2021

vdemeester Sep 16, 2021

vdemeester Sep 16, 2021

mattmoor left a comment

mattmoor commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

mattmoor left a comment

Use an Informer to list LimitRanges 🥼 #4234

Use an Informer to list LimitRanges 🥼 #4234

Conversation

vdemeester commented Sep 16, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

vdemeester commented Sep 16, 2021

imjasonh left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

afrittoli left a comment

Choose a reason for hiding this comment

mattmoor Sep 16, 2021

Choose a reason for hiding this comment

vdemeester commented Sep 16, 2021

vdemeester commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

mattmoor Sep 16, 2021

Choose a reason for hiding this comment

vdemeester Sep 16, 2021

Choose a reason for hiding this comment

vdemeester Sep 16, 2021

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Sep 16, 2021

tekton-robot commented Sep 16, 2021

mattmoor left a comment

Choose a reason for hiding this comment

vdemeester commented Sep 16, 2021 •

edited

Loading