TEP-0135: Introduce coschedule feature flags #6790

QuanZhang-William · 2023-06-07T19:55:18Z

Part of #6740. TEP-0135 introduces a feature that allows a cluster operator to ensure that all of a PipelineRun's pods are scheduled to the same node.

This commit introduces a new feature flag coschedule which works together with the disable-affinity-assistant feature flag to determine the Affinity Assistant behavior. The usage of the new feature flag will be added in the follow-up PRs.

The details of the coschedule feature flag can be found in the Configuration section of TEP-0135. The details of the disable-affinity-assistant feature flag can be found in the Upgrade and Migration Strategy section of TEP-0135.

NOTE: this feature is WIP, please do not use on this feature.

/kind feature

Changes

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

tep-0135: introduce `coschedule` feature flag

tekton-robot · 2023-06-07T19:59: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/apis/config/feature_flags.go	92.6%	93.4%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	88.9%

tekton-robot · 2023-06-07T20:02:09Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	92.6%	93.4%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	88.9%

lbernick · 2023-06-07T20:06:40Z

config/config-feature-flags.yaml

+  # Setting this flag will determine how PipelineRun Pods are scheduled with Affinity Assistant.
+  # Acceptable values are "disabled" (default), "coschedule-workspaces", "coschedule-pipelineruns", or "isolate-pipelineruns".
+  #
+  # See more in the "Configuration" section of tep-0135: coscheduling-pipelinerun-pods for more info about the options:


Rather than linking to the TEP, I think it would be good to write a section of docs explaining the configuration options for the feature. the TEP audience is developers, while our docs audience is cluster operators.

SGTM, I have added a section for explaination.

Can you please also add some markdown docs explaining in more detail what this feature is and why someone would want to use it, in addition to the feature flags chart?

I think you mean adding these details in a doc instead of as a comment in the code here right?

Yes. we already have some docs here on the affinity assistant but I think it's worth writing a docs section for the affinity assistant specifically https://github.com/tektoncd/pipeline/blob/main/docs/workspaces.md#specifying-workspace-order-in-a-pipeline-and-affinity-assistants

I see, I'm planning to put the documentation to the followup PR where I add end-to-end support for the new settings of AA, and I have put a TODO in the code comment. Can we defer the doc to the later PR instead?

TBH I think the docs make the most sense in this PR, as this PR is where the user facing configuration options have been introduced. However I'd also be fine with deferring it.

config/config-feature-flags.yaml

pkg/apis/config/feature_flags.go

pkg/apis/config/testing/featureflags.go

pkg/internal/affinityassistant/affinityassistant_types_test.go

pkg/internal/affinityassistant/affinityassistant_types.go

tekton-robot · 2023-06-16T17:42:37Z

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/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	87.5%

tekton-robot · 2023-06-16T17:44:03Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	87.5%

QuanZhang-William · 2023-06-16T18:02:24Z

/test pull-tekton-pipeline-build-tests

QuanZhang-William · 2023-06-16T19:02:35Z

/test pull-tekton-pipeline-unit-tests

tekton-robot · 2023-06-16T19:02:37Z

@QuanZhang-William: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-beta-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test pull-tekton-pipeline-unit-tests

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.

QuanZhang-William · 2023-06-16T19:03:20Z

/test tekton-pipeline-unit-tests

tekton-robot · 2023-06-16T19:20:11Z

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/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	87.5%

tekton-robot · 2023-06-16T19:36:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	87.5%

lbernick

@pritidesai did you want to weigh in on naming of this feature flag?

I think "coscheduling" is more accurate than "coexisting", since this feature affects scheduling behavior, but we can definitely play around with options that are more concise!

pkg/apis/config/feature_flags.go

lbernick · 2023-06-16T20:51:41Z

config/config-feature-flags.yaml

+  # Setting this flag will determine how PipelineRun Pods are scheduled with Affinity Assistant.
+  # Acceptable values are "disabled" (default), "coschedule-workspaces", "coschedule-pipelineruns", or "isolate-pipelineruns".
+  #
+  # See more in the "Configuration" section of tep-0135: coscheduling-pipelinerun-pods for more info about the options:


Can you please also add some markdown docs explaining in more detail what this feature is and why someone would want to use it, in addition to the feature flags chart?

pkg/internal/affinityassistant/affinityassistant_types_test.go

tekton-robot · 2023-06-19T21:04:34Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T17:56: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/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T17:57:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

QuanZhang-William · 2023-06-27T17:57:41Z

tektoncd/community#1031 is merged.

/hold cancel

tekton-robot · 2023-06-27T18:02: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/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:03:49Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:13:10Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:13:44Z

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/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:15:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	89.8%	-1.9
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:23: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/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T18:24:02Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

Part of [tektoncd#6740][tektoncd#6740]. [TEP-0135][tep-0135] introduces a feature that allows a cluster operator to ensure that all of a PipelineRun's pods are scheduled to the same node. This commit introduces a new feature flag `coschedule` which works together with the `disable-affinity-assistant` feature flag to determine the Affinity Assistant behavior. The usage of the new feature flag will be added in the follow-up PRs. The details of the `coschedule` feature flag can be found in the [Configuration][configuration] section of TEP-0135. The details of the `disable-affinity-assistant` feature flag can be found in the [Upgrade and Migration Strategy][strategy] section of TEP-0135. NOTE: this feature is WIP, please do not use on this feature. /kind feature [tektoncd#6740]: tektoncd#6740 [tep-0135]: https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md [configuration]: https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md#configuration [strategy]: https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md#configuration

tekton-robot · 2023-06-27T19:22:52Z

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/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T19:23:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

tekton-robot · 2023-06-27T19:25:45Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/feature_flags.go	91.8%	92.6%	0.8
pkg/internal/affinityassistant/affinityassistant_types.go	Do not exist	90.0%

jerop · 2023-06-28T14:10:54Z

@QuanZhang-William please add one more test case to cover the uncovered error 🙏🏾

QuanZhang-William · 2023-06-28T14:15:20Z

uncovered error

Hi @jerop! This line is actually not reachable because I cannot pass in an invalid feature flag combination to this function which we have validation in cfgtesting.SetFeatureFlags().

Please see the discussion here: #6790 (comment)

Yongxuanzhang · 2023-06-28T16:52:47Z

/lgtm

tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 7, 2023

tekton-robot requested review from pritidesai and wlynch June 7, 2023 19:55

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

lbernick self-assigned this Jun 7, 2023

lbernick reviewed Jun 7, 2023

View reviewed changes

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 8, 2023

lbernick mentioned this pull request Jun 14, 2023

TEP-0135: implement per-pipelinerun coscheduling #6819

Merged

7 tasks

QuanZhang-William mentioned this pull request Jun 15, 2023

TEP-0135: update coscheduling default value tektoncd/community#1025

Merged

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch 3 times, most recently from 45371d4 to 75d62cb Compare June 16, 2023 17:36

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2023

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from 75d62cb to 7983a34 Compare June 16, 2023 19:13

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

lbernick reviewed Jun 16, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from 7983a34 to 40dc263 Compare June 19, 2023 20:56

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from 40dc263 to a516bc3 Compare June 27, 2023 17:50

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from a516bc3 to f8bca17 Compare June 27, 2023 17:56

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2023

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch 2 times, most recently from 0ac3f79 to 7488677 Compare June 27, 2023 18:08

QuanZhang-William changed the title ~~TEP-0135: Introduce coscheduling feature flags~~ TEP-0135: Introduce coschedule feature flags Jun 27, 2023

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from 7488677 to f5fb801 Compare June 27, 2023 18:16

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from f5fb801 to 246a651 Compare June 27, 2023 19:16

QuanZhang-William force-pushed the tep-0135-add-coscheduling-featureflag branch from 246a651 to 27595f6 Compare June 27, 2023 19:17

tekton-robot assigned Yongxuanzhang Jun 28, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023

tekton-robot merged commit 21828f3 into tektoncd:main Jun 28, 2023

QuanZhang-William mentioned this pull request Jul 25, 2023

TEP-0135: Coscheduling PipelineRun pods Implementation #6740

Closed

TEP-0135: Introduce coschedule feature flags #6790

TEP-0135: Introduce coschedule feature flags #6790

Conversation

QuanZhang-William commented Jun 7, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jun 7, 2023

tekton-robot commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 16, 2023

tekton-robot commented Jun 16, 2023

QuanZhang-William commented Jun 16, 2023

QuanZhang-William commented Jun 16, 2023

tekton-robot commented Jun 16, 2023

QuanZhang-William commented Jun 16, 2023

tekton-robot commented Jun 16, 2023

tekton-robot commented Jun 16, 2023

lbernick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 19, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

QuanZhang-William commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

tekton-robot commented Jun 27, 2023

jerop commented Jun 28, 2023

QuanZhang-William commented Jun 28, 2023 • edited Loading

Yongxuanzhang commented Jun 28, 2023

QuanZhang-William commented Jun 7, 2023 •

edited

Loading

QuanZhang-William commented Jun 28, 2023 •

edited

Loading