-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
cleanup: [TEP-0135] remove deprecated disable-affinity-assistant feature flag #8603
cleanup: [TEP-0135] remove deprecated disable-affinity-assistant feature flag #8603
Conversation
/cc @vdemeester |
configMap := &corev1.ConfigMap{ | ||
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, | ||
Data: map[string]string{ | ||
featureFlagDisableAffinityAssistantKey: "true", | ||
"coschedule": "disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the action required, if you want to maintain the execution behavior, you may need to set the coschedule
to disabled.
The focus is on the line of code below: (line: 53)
pipeline/pkg/internal/affinityassistant/affinityassistant_types.go
Lines 34 to 55 in aec465e
// GetAffinityAssistantBehavior returns an AffinityAssistantBehavior based on the | |
// combination of "disable-affinity-assistant" and "coschedule" feature flags | |
// TODO(#6740)(WIP): consume this function in the PipelineRun reconciler to determine Affinity Assistant behavior. | |
func GetAffinityAssistantBehavior(ctx context.Context) (AffinityAssistantBehavior, error) { | |
cfg := config.FromContextOrDefaults(ctx) | |
disableAA := cfg.FeatureFlags.DisableAffinityAssistant | |
coschedule := cfg.FeatureFlags.Coschedule | |
// at this point, we have validated that "coschedule" can only be "workspaces" | |
// when "disable-affinity-assistant" is false | |
if !disableAA { | |
return AffinityAssistantPerWorkspace, nil | |
} | |
switch coschedule { | |
case config.CoschedulePipelineRuns: | |
return AffinityAssistantPerPipelineRun, nil | |
case config.CoscheduleIsolatePipelineRun: | |
return AffinityAssistantPerPipelineRunWithIsolation, nil | |
case config.CoscheduleDisabled, config.CoscheduleWorkspaces: | |
return AffinityAssistantDisabled, nil | |
} |
The following is the coverage report on the affected files.
|
"test-pipeline-run-variable-substitution", "test-pipeline", "b-task", false), | ||
taskRunObjectMetaWithAnnotations("test-pipeline-run-variable-substitution-b-task", "foo", | ||
"test-pipeline-run-variable-substitution", "test-pipeline", "b-task", false, map[string]string{ | ||
"pipeline.tekton.dev/affinity-assistant": "affinity-assistant-0358aabfa2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the previous setting of disable-affinity-assistant
was true, the default behavior was not to add the relevant annotations. Now that the feature switch has been removed, the behavior has changed somewhat.
Theoretically, the previous effect can also be achieved by setting other feature switches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has this been documented ?
- The behavior change
- what the previous effect is and how to achieve it be achieved with other feature switches ?
This would be good to have in the release notes as well
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
e365ea3
to
ec80a75
Compare
The following is the coverage report on the affected files.
|
ec80a75
to
25d7a0c
Compare
The following is the coverage report on the affected files.
|
config/300-crds/300-pipelinerun.yaml
Outdated
disableCredsInit: | ||
description: DisableAffinityAssistant bool `json:"disableAffinityAssistant,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those descriptions seems weird, don't they ?
pkg/apis/config/feature_flags.go
Outdated
@@ -188,7 +185,7 @@ var ( | |||
// FeatureFlags holds the features configurations | |||
// +k8s:deepcopy-gen=true | |||
type FeatureFlags struct { | |||
DisableAffinityAssistant bool `json:"disableAffinityAssistant,omitempty"` | |||
// DisableAffinityAssistant bool `json:"disableAffinityAssistant,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we keep it as no-op (in order to not fail existing configuration), or we remove it, but we shouldn't keep the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the strange annotations generated by hack/update-codegen.sh
are due to the comments below. I removed the comments and regenerated it, and it worked fine.
Please have a look when you have time. Thank you!
25d7a0c
to
b39da59
Compare
The following is the coverage report on the affected files.
|
…ure flag This field has been deprecated for about a year and half. So this is "removing" this feature flag from the codebase. The field is kept in the go code to provide a backward compatibility for client code (like chains, …) but it will be disallowed by the webhook. It will also be completely ignore by the rest of the code.
b39da59
to
2c9f859
Compare
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester, waveywaves 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
just a few questions
@@ -46,6 +45,8 @@ The features listed below have been removed but may still be supported in releas | |||
| [The `PipelineRun.Status.TaskRuns` and `PipelineRun.Status.Runs` fields and the `embedded-status` feature flag along with their functionalities have been tombstoned since v0.45.](https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md) | [[TEP100] Remove Taskruns and Runs Fields for PipelineRunStatus](https://github.com/tektoncd/pipeline/pull/6099) | Jan 25, 2023 | v0.44.0 | Jan 24, 2024 | | |||
| PipelineResources are removed, along with the components of the API that rely on them as proposed in [TEP-0074](https://github.com/tektoncd/community/blob/main/teps/0074-deprecate-pipelineresources.md). See [Removed `PipelineResources` related features](#removed-pipelineresources-related-features) for more info. | [[TEP074] Remove Generic PipelineResources with Rest of Resources Types](https://github.com/tektoncd/pipeline/pull/6150) | Mar 8, 2023 | v0.44.0 | Jan 24, 2024 | | |||
| v1alpha1 Runs are removed, as proposed in [TEP-0114](https://github.com/tektoncd/community/blob/main/teps/0114-custom-tasks-beta.md), along with the feature flags `enable-custom-task` and `custom-task-version`. | [TEP-0114: Remove support for v1alpha1.Run](https://github.com/tektoncd/pipeline/pull/6508) | April 7, 2023 | v0.44.0 | Jan 24, 2024 | | |||
| [The `disable-affinity-assistant` feature flag is deprecated in favor of the `coschedule` feature flag](https://github.com/tektoncd/community/blob/main/teps/0135-coscheduling-pipelinerun-pods.md#tep-0135-coscheduling-pipelinerun-pods) | [cleanup: [TEP-0135] remove deprecated disable-affinity-assistant feature flag](https://github.com/tektoncd/pipeline/pull/8603) | Mar 1, 2025 | v0.68.0 | Jan 30, 2026 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
@@ -188,12 +185,9 @@ var ( | |||
// FeatureFlags holds the features configurations | |||
// +k8s:deepcopy-gen=true | |||
type FeatureFlags struct { | |||
DisableAffinityAssistant bool `json:"disableAffinityAssistant,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
switch coschedule { | ||
case config.CoschedulePipelineRuns: | ||
return AffinityAssistantPerPipelineRun, nil | ||
case config.CoscheduleIsolatePipelineRun: | ||
return AffinityAssistantPerPipelineRunWithIsolation, nil | ||
case config.CoscheduleDisabled, config.CoscheduleWorkspaces: | ||
case config.CoscheduleWorkspaces: | ||
return AffinityAssistantPerWorkspace, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
"test-pipeline-run-variable-substitution", "test-pipeline", "b-task", false), | ||
taskRunObjectMetaWithAnnotations("test-pipeline-run-variable-substitution-b-task", "foo", | ||
"test-pipeline-run-variable-substitution", "test-pipeline", "b-task", false, map[string]string{ | ||
"pipeline.tekton.dev/affinity-assistant": "affinity-assistant-0358aabfa2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has this been documented ?
- The behavior change
- what the previous effect is and how to achieve it be achieved with other feature switches ?
This would be good to have in the release notes as well
I understand that this is the information displayed in the table of the Release Notes link document.
Specifically, the previous behavior was |
@l-qing thank you for your patience with this review |
fix #8587
This field has been deprecated for about a year and half. So this is "removing" this feature flag from the codebase.
The field is kept in the go code to provide a backward compatibility for client code (like chains, …) but it will be disallowed by the webhook. It will also be completely ignore by the rest of the code.
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind cleanup