Fix Existing Requests and Limits with LimitRange #5269

austinzhao-go · 2022-08-03T19:02:05Z

Changes

This PR is to update limitrange logic to ensure container's requests and limits under the min and max range which is configured by limitrange.

/kind bug

Submitter Checklist

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

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Fixed Existing Requests and Limits with LimitRange

austinzhao-go · 2022-08-03T19:04:28Z

/hold

till #5082 is merged

tekton-robot · 2022-08-03T19:06:41Z

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/computeresources/tasklevel/tasklevel.go	Do not exist	100.0%
pkg/internal/computeresources/transformer.go	91.9%	92.5%	0.6
pkg/pod/pod.go	88.8%	88.2%	-0.6

pkg/internal/computeresources/transformer.go

lbernick

Thanks Austin! Would you mind splitting the limitrange commit into a separate PR?

pkg/internal/computeresources/transformer_test.go

lbernick · 2022-08-03T20:43:42Z

pkg/internal/computeresources/transformer_test.go

@@ -310,7 +310,7 @@ func TestTransformerOneContainer(t *testing.T) {
 			Containers: []corev1.Container{{
 				Resources: corev1.ResourceRequirements{
 					Limits: corev1.ResourceList{
-						corev1.ResourceMemory:           resource.MustParse("4Gi"),
+						corev1.ResourceMemory:           resource.MustParse("2Gi"),


Thanks Austin! Nice job catching this bug, it looks like the behavior here wasn't consistent with our documentation. @vdemeester could you double check me here?

pkg/internal/computeresources/transformer_test.go

pkg/internal/computeresources/transformer.go

austinzhao-go · 2022-08-05T14:35:58Z

/release-note-none

austinzhao-go · 2022-08-05T14:36:21Z

/kind feature

tekton-robot · 2022-08-05T14:38:09Z

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/computeresources/tasklevel/tasklevel.go	Do not exist	100.0%
pkg/internal/computeresources/transformer.go	91.9%	92.3%	0.4
pkg/pod/pod.go	88.8%	88.2%	-0.6

tekton-robot · 2022-08-05T16:35:39Z

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/computeresources/tasklevel/tasklevel.go	Do not exist	100.0%
pkg/internal/computeresources/transformer.go	91.9%	92.3%	0.4
pkg/pod/pod.go	88.8%	88.2%	-0.6

austinzhao-go · 2022-08-05T16:37:52Z

/unhold

tekton-robot · 2022-08-05T16:40:17Z

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/computeresources/transformer.go	91.9%	92.3%	0.4

lbernick · 2022-08-08T15:09:46Z

Fixes #5278
Please also add a release note!

tekton-robot · 2022-08-09T14:24:54Z

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/computeresources/transformer.go	91.9%	92.3%	0.4

austinzhao-go · 2022-08-09T14:47:02Z

/retest

lbernick

We also need to address the test cases "limitRange with min and limits on containers < min" and "limitRange with max and requests on containers > max". These test cases are outputting invalid pods (requests > limits).

pkg/internal/computeresources/transformer_test.go

lbernick · 2022-08-09T19:47:35Z

/hold cancel

tekton-robot · 2022-08-09T19:50: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/computeresources/transformer.go	91.9%	93.0%	1.1

abayer · 2022-08-10T14:22:17Z

/approve

tekton-robot · 2022-08-10T14:22:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer

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~~ [abayer]

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

austinzhao-go · 2022-08-10T18:48:42Z

thanks @abayer for the review!

pushed an update to ensure requests got "capped" by limits finally

tekton-robot · 2022-08-10T18:51:17Z

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/computeresources/transformer.go	91.9%	93.5%	1.6

austinzhao-go · 2022-08-11T13:39:21Z

/retest

pkg/internal/computeresources/transformer_test.go

austinzhao-go · 2022-08-12T15:32:20Z

/hold

till collecting further feedback from community

LimitRange logic were updated to ensure the requests and limits of containers will be under the min and max range of LimitRange.

tekton-robot · 2022-08-16T14:07: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/computeresources/transformer.go	91.9%	94.1%	2.2

tekton-robot · 2022-08-16T14:29:42Z

@austinzhao-go: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-tekton-pipeline-integration-tests	`580012b`	link	true	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

austinzhao-go · 2022-08-19T19:11:05Z

closed the PR for this moment after a check with @lbernick

the expected fix will be updated in another PR.

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 3, 2022

tekton-robot requested review from jerop and lbernick August 3, 2022 19:02

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2022

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2022

austinzhao-go commented Aug 3, 2022

View reviewed changes

pkg/internal/computeresources/transformer.go Outdated Show resolved Hide resolved

austinzhao-go mentioned this pull request Aug 3, 2022

[TEP-0104] Update Pod with Task-level Resource Requirements #5082

Merged

5 tasks

lbernick self-assigned this Aug 3, 2022

lbernick reviewed Aug 3, 2022

View reviewed changes

lbernick assigned vdemeester Aug 3, 2022

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch 3 times, most recently from 89ae4d1 to be0d72c Compare August 5, 2022 14:33

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 5, 2022

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 5, 2022

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from be0d72c to c9be8d9 Compare August 5, 2022 16:31

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from c9be8d9 to 59cef74 Compare August 5, 2022 16:35

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 Aug 5, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2022

lbernick removed the kind/feature Categorizes issue or PR as related to a new feature. label Aug 5, 2022

austinzhao-go changed the title ~~[TEP-0104] Update Container's Requests and Limits with LimitRange~~ Fix Container's Requests and Limits with LimitRange Aug 8, 2022

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2022

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from 59cef74 to 337590a Compare August 9, 2022 14:18

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2022

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from 337590a to 4c77588 Compare August 9, 2022 14:19

austinzhao-go changed the title ~~Fix Container's Requests and Limits with LimitRange~~ Fix Existing Requests and Limits with LimitRange Aug 9, 2022

lbernick reviewed Aug 9, 2022

View reviewed changes

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

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from 4c77588 to 141aa00 Compare August 9, 2022 19:45

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2022

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2022

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from 141aa00 to 10ebfd5 Compare August 10, 2022 18:47

lbernick reviewed Aug 11, 2022

View reviewed changes

pkg/internal/computeresources/transformer_test.go Show resolved Hide resolved

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022

Fix Requests and Limits with LimitRange

580012b

LimitRange logic were updated to ensure the requests and limits of containers will be under the min and max range of LimitRange.

austinzhao-go force-pushed the tep-0104/update-requests-limits-with-limitrange branch from 10ebfd5 to 580012b Compare August 16, 2022 14:03

austinzhao-go closed this Aug 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Existing Requests and Limits with LimitRange #5269

Fix Existing Requests and Limits with LimitRange #5269

austinzhao-go commented Aug 3, 2022 •

edited

Loading

austinzhao-go commented Aug 3, 2022

tekton-robot commented Aug 3, 2022

lbernick left a comment

lbernick Aug 3, 2022

austinzhao-go commented Aug 5, 2022

austinzhao-go commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

austinzhao-go commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

lbernick commented Aug 8, 2022 •

edited

Loading

tekton-robot commented Aug 9, 2022

austinzhao-go commented Aug 9, 2022

lbernick left a comment

lbernick commented Aug 9, 2022

tekton-robot commented Aug 9, 2022

abayer commented Aug 10, 2022

tekton-robot commented Aug 10, 2022

austinzhao-go commented Aug 10, 2022

tekton-robot commented Aug 10, 2022

austinzhao-go commented Aug 11, 2022

austinzhao-go commented Aug 12, 2022

tekton-robot commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

austinzhao-go commented Aug 19, 2022

Fix Existing Requests and Limits with LimitRange #5269

Fix Existing Requests and Limits with LimitRange #5269

Conversation

austinzhao-go commented Aug 3, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

austinzhao-go commented Aug 3, 2022

tekton-robot commented Aug 3, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick Aug 3, 2022

Choose a reason for hiding this comment

austinzhao-go commented Aug 5, 2022

austinzhao-go commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

austinzhao-go commented Aug 5, 2022

tekton-robot commented Aug 5, 2022

lbernick commented Aug 8, 2022 • edited Loading

tekton-robot commented Aug 9, 2022

austinzhao-go commented Aug 9, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick commented Aug 9, 2022

tekton-robot commented Aug 9, 2022

abayer commented Aug 10, 2022

tekton-robot commented Aug 10, 2022

austinzhao-go commented Aug 10, 2022

tekton-robot commented Aug 10, 2022

austinzhao-go commented Aug 11, 2022

austinzhao-go commented Aug 12, 2022

tekton-robot commented Aug 16, 2022

tekton-robot commented Aug 16, 2022

austinzhao-go commented Aug 19, 2022

austinzhao-go commented Aug 3, 2022 •

edited

Loading

lbernick commented Aug 8, 2022 •

edited

Loading