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] Support Task-level Resource Requirements for TaskRun: Part #1 Fields Addition & Validation w/ Docs Updates #4877

Merged
merged 1 commit into from
Jun 28, 2022
Merged

[TEP-0104] Support Task-level Resource Requirements for TaskRun: Part #1 Fields Addition & Validation w/ Docs Updates #4877

merged 1 commit into from
Jun 28, 2022

Conversation

austinzhao-go
Copy link
Contributor

@austinzhao-go austinzhao-go commented May 16, 2022 •
edited
Loading

Changes

/kind feature

The related TEP:
TEP-0104: Support Task-level resource limits
TEP-0104: Scope Updates to Runtime
TEP-0104: Rename "resources" field as "computeResources"

The related FR thread:
FR: Task-level (and maybe Pipeline-level) resource requests and limits

This PR will include changes as:

  • Add the required "computeResources" field for TaskRun
  • Add the related validations
  • Update the user guide for this new feature

A follow-up PR will include changes to update compute resources configuration by the specified task-level resource requirements:

TODO

  • Update this PR only with field addition and webhook validation
  • Add alpha FF to guard changes over webhook
  • Update related docs
  • Confirm approval and merge on TEP-0104, then start merging implementations

After the scope update & field name update:

  • Update field validation
  • Update the related user guidance

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
    (if there are no user facing changes, use release note "NONE")

Release Notes

As changes will be delivered in 2/3 PRs, the release notes will be added in ref-to-be-added.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 16, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 16, 2022
@austinzhao-go austinzhao-go marked this pull request as draft May 16, 2022 15: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/reconciler/taskrun/taskrun.go 80.1% 78.9% -1.2
pkg/reconciler/taskrun/validate_resources.go 93.7% 92.4% -1.3

@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from fcd18c3 to 00a3b81 Compare May 17, 2022 00: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/reconciler/taskrun/taskrun.go 80.1% 78.9% -1.2
pkg/reconciler/taskrun/validate_resources.go 93.7% 92.4% -1.3

@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 00a3b81 to f71b082 Compare May 17, 2022 01:00
@tekton-robot 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 May 17, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from f71b082 to 18f89e6 Compare May 17, 2022 01:01
@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/reconciler/taskrun/taskrun.go 80.1% 78.2% -1.9
pkg/reconciler/taskrun/validate_resources.go 93.7% 92.0% -1.7

@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/reconciler/taskrun/taskrun.go 80.1% 78.2% -1.9
pkg/reconciler/taskrun/validate_resources.go 93.7% 92.0% -1.7

lbernick
lbernick reviewed May 17, 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.

Thanks Austin! I would suggest just adding the fields, docs, and webhook validation in the initial PR, and updating the reconciler in a subsequent PR.

/hold

until tektoncd/community#703 is accepted

pkg/apis/pipeline/v1beta1/resource_types.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/resource_types.go Outdated Show resolved Hide resolved
pkg/reconciler/taskrun/taskrun.go Outdated Show resolved Hide resolved
@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 May 17, 2022
@austinzhao-go
Copy link
Contributor Author

austinzhao-go commented May 17, 2022 •
edited
Loading

agree @lbernick. thinking of a bit the complexity, was planning to complete the logic Task-level first, then for "runtime overwriting" from TaskRunSpec. but let me do this perhaps (for an easier review):

  • field addition and validation on current PR
  • Task-level update on 2nd PR
  • "runtime overwriting" on 3rd PR

@lbernick
Copy link
Member

ok sounds good! Just one more note, new fields should be guarded behind the alpha feature flag

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels May 17, 2022
@austinzhao-go
Copy link
Contributor Author

got this. So the feature flag checks will be mainly/only needed in reconciler code right?
(not changes wekbook code before, let me have another check. perhaps also needed there)

@austinzhao-go austinzhao-go reopened this May 17, 2022
@austinzhao-go
Copy link
Contributor Author

/kind feature

@lbernick
Copy link
Member

got this. So the feature flag checks will be mainly/only needed in reconciler code right? (not changes wekbook code before, let me have another check. perhaps also needed there)

The feature flag checks should be included in the webhook validation, and optionally the reconciler validation, for the same reason as before (fail as fast as possible).

@austinzhao-go
Copy link
Contributor Author

/retest

@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/reconciler/taskrun/taskrun.go 80.1% 78.2% -1.9
pkg/reconciler/taskrun/validate_resources.go 93.7% 92.0% -1.7

@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 18f89e6 to 6847b3f Compare May 19, 2022 14:20
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 19, 2022
@austinzhao-go austinzhao-go mentioned this pull request Jun 15, 2022
Closed
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 58b5fea to a274242 Compare June 16, 2022 00:53
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 16, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch 2 times, most recently from 3c668a9 to 445529f Compare June 16, 2022 02:35
lbernick
lbernick reviewed Jun 16, 2022
View reviewed changes
docs/taskruns.md Show resolved Hide resolved
docs/compute-resources.md Show resolved Hide resolved
docs/compute-resources.md Outdated Show resolved Hide resolved
docs/compute-resources.md Outdated Show resolved Hide resolved
docs/compute-resources.md Outdated Show resolved Hide resolved
docs/compute-resources.md Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/taskrun_validation.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/taskrun_validation.go Outdated Show resolved Hide resolved
pkg/apis/pipeline/v1beta1/taskrun_validation.go Outdated Show resolved Hide resolved
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 445529f to 9ce0894 Compare June 16, 2022 17:36
@austinzhao-go
Copy link
Contributor Author

austinzhao-go commented Jun 16, 2022 •
edited
Loading

/assign @lbernick

/assign @dibyom

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 9ce0894 to 030d627 Compare June 17, 2022 13:12
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2022
lbernick
lbernick approved these changes Jun 17, 2022
View reviewed changes
pkg/apis/pipeline/v1beta1/taskrun_validation.go Outdated Show resolved Hide resolved
docs/compute-resources.md 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 Jun 17, 2022
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from 030d627 to e67fb1c Compare June 17, 2022 15:40
@austinzhao-go
Copy link
Contributor Author

/retest

@austinzhao-go
Support Task-level Resources Requirements on TaskRun: Part #1
2e80113 
Required fields and related webhook validations are added to support
a user to configure compute resources for TaskRun which will significantly
reduce the over-asked resources amount configured by the Step-level.
@austinzhao-go austinzhao-go force-pushed the tep-0104-per-task-resources-requirements branch from e67fb1c to 2e80113 Compare June 20, 2022 12:56
@austinzhao-go
Copy link
Contributor Author

/meow

@tekton-robot
Copy link
Collaborator

@austinzhao-go: cat image

In response to this:

/meow

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.

@dibyom
Copy link
Member

dibyom commented Jun 27, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2022
@austinzhao-go
Copy link
Contributor Author

/retest

@tekton-robot tekton-robot merged commit c6ff981 into tektoncd:main Jun 28, 2022
This was referenced Jul 4, 2022
Closed
Merged
@austinzhao-go austinzhao-go mentioned this pull request Jul 26, 2022
Merged
7 tasks
@austinzhao-go austinzhao-go mentioned this pull request Aug 3, 2022
Closed
7 tasks
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

@pritidesai pritidesai Awaiting requested review from pritidesai

Assignees

@dibyom dibyom

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 Denotes a PR that will be considered when it comes time to generate release notes. 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.
4 participants
@austinzhao-go @tekton-robot @lbernick @dibyom