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

Question: Is there a plan to completely remove the deprecated disable-affinity-assistant configuration option? #8587

Open
l-qing opened this issue Feb 24, 2025 · 3 comments · May be fixed by #8603
Open

Question: Is there a plan to completely remove the deprecated disable-affinity-assistant configuration option? #8587

l-qing opened this issue Feb 24, 2025 · 3 comments · May be fixed by #8603
Labels
area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/question Issues or PRs that are questions around the project or a particular feature
Milestone
Pipeline v1.0.0

Comments

@l-qing
Copy link
Member

l-qing commented Feb 24, 2025

Currently, if I need to allow TaskRun to mount 2 PVCs, I must set the deprecated disable-affinity-assistant to true, which is quite strange.

As comment, this configuration item is planned to be removed in v0.60, and v0.68 has already been released.

pipeline/config/config-feature-flags.yaml

Lines 23 to 34 in 30f7cce

data:
# Setting this flag to "true" will prevent Tekton to create an
# Affinity Assistant for every TaskRun sharing a PVC workspace
#
# The default behaviour is for Tekton to create Affinity Assistants
#
# See more in the Affinity Assistant documentation
# https://github.com/tektoncd/pipeline/blob/main/docs/affinityassistants.md
# or https://github.com/tektoncd/pipeline/pull/2630 for more info.
#
# Note: This feature flag is deprecated and will be removed in release v0.60. Consider using `coschedule` feature flag to configure Affinity Assistant behavior.
disable-affinity-assistant: "false"

Related validation code

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 549 to 558 in 30f7cce

aaBehavior, err := affinityassistant.GetAffinityAssistantBehavior(ctx)
if err != nil {
return nil, nil, controller.NewPermanentError(err)
}
if aaBehavior == affinityassistant.AffinityAssistantPerWorkspace {
if err := workspace.ValidateOnlyOnePVCIsUsed(tr.Spec.Workspaces); err != nil {
logger.Errorf("TaskRun %q workspaces incompatible with Affinity Assistant: %v", tr.Name, err)
tr.Status.MarkResourceFailed(v1.TaskRunReasonFailedValidation, err)
return nil, nil, controller.NewPermanentError(err)
}

If disable-affinity-assistant is set to false by default, GetAffinityAssistantBehavior will inevitably exit early.

pipeline/pkg/internal/affinityassistant/affinityassistant_types.go

Lines 32 to 44 in 30f7cce

// 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
}

The ValidateOnlyOnePVCIsUsed check will definitely be executed.

pipeline/pkg/workspace/validate.go

Lines 66 to 86 in 30f7cce

// ValidateOnlyOnePVCIsUsed checks that a list of WorkspaceBinding uses only one
// persistent volume claim.
//
// This is only useful to validate that WorkspaceBindings in TaskRuns are compatible
// with affinity rules enforced by the AffinityAssistant.
func ValidateOnlyOnePVCIsUsed(wb []v1.WorkspaceBinding) error {
workspaceVolumes := make(map[string]bool)
for _, w := range wb {
if w.PersistentVolumeClaim != nil {
workspaceVolumes[w.PersistentVolumeClaim.ClaimName] = true
}
if w.VolumeClaimTemplate != nil {
workspaceVolumes[w.Name] = true
}
}
if len(workspaceVolumes) > 1 {
return pipelineErrors.WrapUserError(errors.New("more than one PersistentVolumeClaim is bound"))
}
return nil
}

In addition, the document mentions that setting disable-affinity-assistant to false and coschedule to disabled results in a disabled behavior, but currently this combination cannot be configured at all, as it has already been rejected in the webhook.

Affinity Assistants

pipeline/pkg/apis/config/feature_flags.go

Lines 343 to 356 in 30f7cce

// setCoschedule sets the "coschedule" flag based on the content of a given map.
// If the feature gate is invalid or incompatible with `disable-affinity-assistant`, then an error is returned.
func setCoschedule(cfgMap map[string]string, defaultValue string, disabledAffinityAssistant bool, feature *string) error {
value := defaultValue
if cfg, ok := cfgMap[coscheduleKey]; ok {
value = strings.ToLower(cfg)
}
switch value {
case CoscheduleDisabled, CoscheduleWorkspaces, CoschedulePipelineRuns, CoscheduleIsolatePipelineRun:
// validate that "coschedule" is compatible with "disable-affinity-assistant"
// "coschedule" must be set to "workspaces" when "disable-affinity-assistant" is false
if !disabledAffinityAssistant && value != CoscheduleWorkspaces {
return fmt.Errorf("coschedule value %v is incompatible with %v setting to false", value, disableAffinityAssistantKey)

@afrittoli
Copy link
Member

Thanks @l-qing for bringing this up.

We should definitely remove the deprecated flag.
I believe the current behaviour matches the docs - until the flag is removed.

The docs include a table that describes the "behavior during migration", which is the current state.
Right now, the configuration you mentioned, both disabled, is marked as invalid:

Image

@l-qing would you be interested in working on phasing out the deprecated flag?
@vdemeester any thoughts?

@vdemeester
Copy link
Member

100% agree @afrittoli and @l-qing. We should work towards removing it, ideally before 1.0 ? 👼🏼

@vdemeester vdemeester added this to the Pipeline v1.0.0 milestone Feb 24, 2025
@l-qing
Copy link
Member Author

l-qing commented Feb 24, 2025

would you be interested in working on phasing out the deprecated flag?

Yes, I'll take some time to handle it this week.

@l-qing l-qing linked a pull request Mar 1, 2025 that will close this issue
Open
8 tasks
@vdemeester vdemeester added kind/question Issues or PRs that are questions around the project or a particular feature area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) labels Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/question Issues or PRs that are questions around the project or a particular feature
Projects
Tekton Community Roadmap
Status: Todo
Milestone
Pipeline v1.0.0
Development

Successfully merging a pull request may close this issue.

cleanup: [TEP-0135] remove deprecated disable-affinity-assistant feature flag l-qing/pipeline
3 participants
@vdemeester @afrittoli @l-qing