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

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

Merged
merged 1 commit into from
Aug 16, 2022
Merged

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

merged 1 commit into from
Aug 16, 2022

Conversation

austinzhao-go
Copy link
Contributor

@austinzhao-go austinzhao-go commented Jul 5, 2022 •
edited by lbernick
Loading

Changes

/kind feature

Closes #4470

The related TEP:
TEP-0104: Task-level Resource Requirements

The related impl PRs:
#4877 - Fields Addition & Validation w/ Docs Updates
#5054 - Add Validation for Step-level Resource Requirements
#5212 - Populate Task-level Resource Requirements from PipelineRun to TaskRun

This PR will include logic and tests as:

  • Update the containers in the Pod with the specified task-level resource requirements

Submitter Checklist

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

Release Notes

Supported Task-level Resource Requirements

@tekton-robot tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. 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 Jul 5, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 5, 2022
@austinzhao-go
Copy link
Contributor Author

/hold

till #5054 got merged

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2022
@austinzhao-go austinzhao-go mentioned this pull request Jul 5, 2022
Closed
7 tasks
@tekton-robot
Copy link
Collaborator

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/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 85.9% 0.2
pkg/apis/pipeline/v1beta1/taskrun_validation.go 100.0% 98.8% -1.2
pkg/pod/pod.go 88.3% 88.5% 0.2

@austinzhao-go
Copy link
Contributor Author

/release-note-none

@tekton-robot 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 Jul 5, 2022
@austinzhao-go
Copy link
Contributor Author

/retest

@lbernick lbernick self-assigned this Jul 6, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 7877302 to 68ebb96 Compare July 6, 2022 14:58
@tekton-robot
Copy link
Collaborator

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/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 85.9% 0.2
pkg/apis/pipeline/v1beta1/taskrun_validation.go 100.0% 98.8% -1.2
pkg/pod/pod.go 88.3% 88.5% 0.2

lbernick
lbernick reviewed Jul 7, 2022
View reviewed changes
pkg/pod/pod.go Outdated Show resolved Hide resolved
pkg/pod/pod.go Outdated Show resolved Hide resolved
pkg/pod/pod_test.go Outdated Show resolved Hide resolved
@austinzhao-go
Copy link
Contributor Author

/retest

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 68ebb96 to 7dd3c71 Compare July 23, 2022 13:19
@tekton-robot
Copy link
Collaborator

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/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 7dd3c71 to 00e276f Compare July 24, 2022 14:09
@tekton-robot
Copy link
Collaborator

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/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 85.9% 0.2
pkg/apis/pipeline/v1beta1/taskrun_validation.go 100.0% 98.8% -1.2
pkg/pod/pod.go 88.8% 89.9% 1.1

lbernick
lbernick reviewed Jul 25, 2022
View reviewed changes
Copy link
Member

@lbernick lbernick left a comment

Choose a reason for hiding this comment

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

I know this needs to be rebased onto #5054, but could you keep the scope of this PR to just the changes required to apply the computeResources of a taskrun to a pod? i.e. validation and passing spec from pipelinerun -> taskrun should be covered in #5054.
Also, I think a better place for some of this logic might be pkg/internal/computeresources.

pkg/pod/pod.go Outdated Show resolved Hide resolved
pkg/pod/pod_test.go Outdated Show resolved Hide resolved
@austinzhao-go
Copy link
Contributor Author

yap. let me keep validation related in #5054.

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 00e276f to 9f19d23 Compare July 25, 2022 17:47
@tekton-robot
Copy link
Collaborator

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/pod/pod.go 88.8% 89.8% 1.0

@austinzhao-go austinzhao-go mentioned this pull request Jul 25, 2022
Closed
@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 9f19d23 to 0204f00 Compare July 25, 2022 19:14
@tekton-robot
Copy link
Collaborator

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/pod/pod.go 88.8% 89.8% 1.0

@austinzhao-go austinzhao-go mentioned this pull request Jul 26, 2022
Merged
7 tasks
@tekton-robot
Copy link
Collaborator

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 93.8%
pkg/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from dee7e64 to 7549113 Compare August 9, 2022 17:42
@tekton-robot
Copy link
Collaborator

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 93.8%
pkg/pod/pod.go 88.8% 89.0% 0.2

lbernick
lbernick approved these changes Aug 9, 2022
View reviewed changes
Copy link
Member

@lbernick lbernick left a comment

Choose a reason for hiding this comment

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

This looks great!
A few notes:

pkg/internal/computeresources/tasklevel/tasklevel.go Outdated Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 7549113 to 1f77488 Compare August 9, 2022 19:54
@tekton-robot
Copy link
Collaborator

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 94.1%
pkg/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go
Copy link
Contributor Author

/retest

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 1f77488 to a49767e Compare August 11, 2022 13:40
@tekton-robot
Copy link
Collaborator

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 94.1%
pkg/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go
Copy link
Contributor Author

/retest

@austinzhao-go
Copy link
Contributor Author

/unhold

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from a49767e to 1740dca Compare August 12, 2022 16:23
lbernick
lbernick reviewed Aug 12, 2022
View reviewed changes
docs/compute-resources.md Outdated Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

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 94.1%
pkg/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from 1740dca to c5c4005 Compare August 12, 2022 16:51
@tekton-robot
Copy link
Collaborator

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 94.1%
pkg/pod/pod.go 88.8% 89.0% 0.2

lbernick
lbernick reviewed Aug 12, 2022
View reviewed changes
docs/compute-resources.md Outdated Show resolved Hide resolved
@austinzhao-go
[TEP-0104] Update Pod with Task-level Requirements
8308f10 
The task-level compute resource requirements will be applied on the containers created
by the Pod.
@austinzhao-go austinzhao-go force-pushed the tep-0104/update-pod-with-task-level-config branch from c5c4005 to 8308f10 Compare August 12, 2022 18:10
@tekton-robot
Copy link
Collaborator

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 94.1%
pkg/pod/pod.go 88.8% 89.0% 0.2

@austinzhao-go
Copy link
Contributor Author

/retest

@abayer
Copy link
Contributor

abayer commented Aug 16, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2022
@tekton-robot tekton-robot merged commit 34c5af6 into tektoncd:main Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbernick lbernick lbernick approved these changes

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@abayer abayer

@lbernick lbernick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. 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.

FR: Task-level (and maybe Pipeline-level) resource requests and limits
4 participants
@austinzhao-go @tekton-robot @abayer @lbernick