[TEP-0135] Update Affinity Assistant documentation #6892

QuanZhang-William · 2023-06-30T14:14:14Z

Part of #6740. This commit updates the Affinity Assistant related documentation to reflect recent changes in the Affinity Assistant modes. This commit also adds a chart summarizing how to configure the Affinity Assistant modes using coschedule and disabled-affinity-assistant feature flags.

/kind documentation

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

NONE

docs/workspaces.md

config/config-feature-flags.yaml

QuanZhang-William · 2023-06-30T18:21:10Z

docs/affinityassistants.md

+<!--
+---
+linkTitle: "Affinity Assistants"
+weight: 405


I'm not sure why do we need it 😂

docs/affinityassistants.md

lbernick · 2023-06-30T18:34:42Z

docs/affinityassistants.md

+
+**Note:** For users who previously accepting default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting `coschedule` flag. For users who previously disabling the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.
+
+**Note:** **DO NOT** change the default value of the `coschedule` feature flag since the feature is work in progress and not yet functional. Please **ONLY** toggle the `disabled-affinity-assistant` feature flag to switch between **"disabled"** and **"coschedule workspace"** modes.


coscheduling = disabled is OK right?

Yeah this is fine. And disable-affinity-assistant : true and coschedule: workspaces will also disabled AA during migration. I put it here just for simplicity as this feature will be completed soon and this note will be removed.

docs/affinityassistants.md

wlynch · 2023-07-05T14:55:20Z

docs/affinityassistants.md

+* **coschedule pipelineruns (WIP)** - All `TaskRun` pods within the `PipelineRun` will be scheduled to the same Node.
+
+* **isolate PipelineRun (WIP)** - All `TaskRun` pods within the `PipelineRun` will be scheduled to the same Node,
+and only one PipelineRun is allowed to run on a node at a time.


If this functionality isn't available yet, perhaps comment this out for now?

(this gets published as user facing documentation on tekton.dev)

If this is meant to be more contributor focused, consider moving this to a README in the corresponding package or godoc.

The implementation for this feature is about to be completed. Maybe we can merge this PR after the feature is fully supported and update this section.

/hold

wlynch · 2023-07-05T15:01:55Z

docs/affinityassistants.md

+This means that Affinity Assistant is incompatible with e.g. other affinity rules
+ configured for the `TaskRun` pods. If the `PipelineRun` has a custom
+[PodTemplate](pipelineruns.md#specifying-a-pod-template) configured, the `NodeSelector` and `Tolerations` fields
+will also be set on the Affinity Assistant pod. The Affinity Assistant
+is deleted when the `PipelineRun` is completed. 


"Incompatible" is a bit ambiguous here - could you explain this a bit more?

Typically when I think of incompatibility I expect either an error to be returned or one field to take precedence over the other. But in this case it doesn't sound like either is what happens, and Affinity Assistants do coexist with PodTemplates? (though I could be misunderstanding something)

I have added some explanation for it, Affinity term in PodTemplate will be overwritten by Affinity Assistant if specified:

https://github.com/tektoncd/pipeline/blob/4f6c74a33b59d2bfc5d5ee14e2a9aa67d74b22f9/pkg/reconciler/taskrun/taskrun.go#L752C1-L756C3

wlynch · 2023-07-05T15:03:40Z

docs/affinityassistants.md

+
+The following chart summarizes the Affinity Assistant Modes with different combinations of the `disable-affinity-assistant` and `coschedule` feature flags during migration (when both feature flags are present) and after the migration (when only the `coschedule` flag is present).
+
+**Note:** For users who previously accepted the default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting the `coschedule` flag. For users who previously disabled the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.


How can I figure out if I previously accepted the default behavior?

I have update this part to show the default configmap setting explicitly!

wlynch · 2023-07-05T15:07:15Z

docs/affinityassistants.md

+
+The following chart summarizes the Affinity Assistant Modes with different combinations of the `disable-affinity-assistant` and `coschedule` feature flags during migration (when both feature flags are present) and after the migration (when only the `coschedule` flag is present).
+
+**Note:** For users who previously accepted the default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting the `coschedule` flag. For users who previously disabled the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.


Side note: it's a bit confusing that to turn on the coscheduling affinity assistant feature, you need to set disable-affinity-assistant=true - the flag names seem to conflict each other. idk if it's too late for this, but you may want to look into renaming these flags.

Hi @wlynch. Yeah, there was quite a long discussion about the naming, default value and how to make the migration less confusing to users while maintaining backward compatibility: tektoncd/community#1025 (comment) and tektoncd/community#1031.

Based on the discussion, it appeared that turning disable-affinity-assistant on or off to honour coschedule is quite personal. This feature is added as an alpha feature, I think we can collect more user feedback before promoting it to beta.

wlynch · 2023-07-05T15:12:39Z

docs/affinityassistants.md

+
+**Note:** For users who previously accepted the default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting the `coschedule` flag. For users who previously disabled the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.
+
+**Note:** **DO NOT** change the default value of the `coschedule` feature flag since the feature is work in progress and not yet functional. Please **ONLY** toggle the `disabled-affinity-assistant` feature flag to switch between **"disabled"** and **"coschedule workspace"** modes.


You may want to make this warning more prominent - moving this up to the top + ⚠️ / 🚨 emojis + bigger font / bold would help.

We should also try and classify all of these with their stability level to set expectations around breaking changes (by default users will assume stable).

Good point! Will add the stability levels for different mode!

Hi @wlynch. I have removed all the warnings and WIP in the documentation, and we will hold this PR until this feature is fully implemented (#6927 is merged).

I have also added the stability levels for the feature, including adding it to the alpha feature list. PTAL 😄

wlynch · 2023-07-05T15:15:12Z

docs/affinityassistants.md

+In the future, the `disable-affinity-assistant` feature flag will be deprecated, and the Affinity Assistant Modes will be only determined by 
+the `coschedule` feature flag. 
+
+The following chart summarizes the Affinity Assistant Modes with different combinations of the `disable-affinity-assistant` and `coschedule` feature flags during migration (when both feature flags are present) and after the migration (when only the `coschedule` flag is present).


This is the first mention of migration - when is this happening?

Added 9 month deprecation time to this part

wlynch · 2023-07-05T15:18:14Z

docs/affinityassistants.md

+
+**Note:** **DO NOT** change the default value of the `coschedule` feature flag since the feature is work in progress and not yet functional. Please **ONLY** toggle the `disabled-affinity-assistant` feature flag to switch between **"disabled"** and **"coschedule workspace"** modes.
+
+<table>


As a user / someone who isn't really familiar with this feature, I don't know what this table is supposed to be telling me 😅

Are these the config map values?

Yeah. I switched the order of the chart and the notes, so that the chart is now right below its explanations. PTAL

wlynch · 2023-07-05T15:21:20Z

docs/affinityassistants.md

+will also be set on the Affinity Assistant pod. The Affinity Assistant
+is deleted when the `PipelineRun` is completed. 
+
+Currently, the Affinity Assistant Modes can be configured by the `disable-affinity-assistant` and `coschedule` feature flags.


Might be worth including the expected config map values somewhere and how they map to the modes above.

Please see the above comment

Part of [tektoncd#6740][tektoncd#6740]. This commit updates the Affinity Assistant related documentation to reflect recent changes in the Affinity Assistant modes. This commit also adds a chart summarizing how to configure the Affinity Assistant modes using `coschedule` and `disabled-affinity-assistant` feature flags. /kind documentation [tektoncd#6740]: tektoncd#6740

vdemeester

/lgtm

tekton-robot · 2023-07-24T07:36:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, vdemeester

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~~ [lbernick,vdemeester]

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

QuanZhang-William · 2023-07-26T20:50:26Z

/hold cancel

#6927 is merged

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/documentation Categorizes issue or PR as related to documentation. labels Jun 30, 2023

tekton-robot requested review from pritidesai and wlynch June 30, 2023 14:14

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 30, 2023

lbernick self-assigned this Jun 30, 2023

lbernick reviewed Jun 30, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch from ccd6bb0 to 7598323 Compare June 30, 2023 18: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 Jun 30, 2023

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch from 7598323 to b4cbfa3 Compare June 30, 2023 18:20

QuanZhang-William commented Jun 30, 2023

View reviewed changes

lbernick approved these changes Jun 30, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 30, 2023

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch 3 times, most recently from fcddb4a to ebe86a6 Compare June 30, 2023 20:35

wlynch reviewed Jul 5, 2023

View reviewed changes

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch from ebe86a6 to d8c299e Compare July 10, 2023 21:27

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2023

QuanZhang-William mentioned this pull request Jul 21, 2023

[TEP-0135] Coschedule per (Isolated) PipelineRun e2e support #6927

Merged

7 tasks

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch 2 times, most recently from 23530e0 to 561f6f4 Compare July 21, 2023 21:05

QuanZhang-William force-pushed the tep-0135-coschedule-documentation branch from 561f6f4 to fd4aa5f Compare July 21, 2023 21:08

QuanZhang-William requested a review from wlynch July 21, 2023 21:11

vdemeester approved these changes Jul 24, 2023

View reviewed changes

tekton-robot assigned vdemeester Jul 24, 2023

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

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

tekton-robot merged commit c38c220 into tektoncd:main Jul 26, 2023

QuanZhang-William mentioned this pull request Jul 27, 2023

TEP-0135: Coscheduling PipelineRun pods Implementation #6740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0135] Update Affinity Assistant documentation #6892

[TEP-0135] Update Affinity Assistant documentation #6892

QuanZhang-William commented Jun 30, 2023

QuanZhang-William Jun 30, 2023

lbernick Jun 30, 2023

QuanZhang-William Jun 30, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023 •

edited

Loading

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

QuanZhang-William Jul 21, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

wlynch Jul 5, 2023

QuanZhang-William Jul 10, 2023

vdemeester left a comment

tekton-robot commented Jul 24, 2023

QuanZhang-William commented Jul 26, 2023


		Note: For users who previously accepting default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting `coschedule` flag. For users who previously disabling the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.

		Note: DO NOT change the default value of the `coschedule` feature flag since the feature is work in progress and not yet functional. Please ONLY toggle the `disabled-affinity-assistant` feature flag to switch between "disabled" and "coschedule workspace" modes.


		The following chart summarizes the Affinity Assistant Modes with different combinations of the `disable-affinity-assistant` and `coschedule` feature flags during migration (when both feature flags are present) and after the migration (when only the `coschedule` flag is present).

		Note: For users who previously accepted the default behavior (enabling affinity assistant) but now want one of the new features, you need to set `disable-affinity-assistant` to "true" and then turn on the new behavior by setting the `coschedule` flag. For users who previously disabled the affinity assistant but want one of the new features, just set the `coschedule` flag accordingly.


		Note: DO NOT change the default value of the `coschedule` feature flag since the feature is work in progress and not yet functional. Please ONLY toggle the `disabled-affinity-assistant` feature flag to switch between "disabled" and "coschedule workspace" modes.

		<table>

[TEP-0135] Update Affinity Assistant documentation #6892

[TEP-0135] Update Affinity Assistant documentation #6892

Conversation

QuanZhang-William commented Jun 30, 2023

Changes

Submitter Checklist

Release Notes

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

Choose a reason for hiding this comment

QuanZhang-William Jul 10, 2023 • edited Loading

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

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

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 24, 2023

QuanZhang-William commented Jul 26, 2023

QuanZhang-William Jul 10, 2023 •

edited

Loading