TEP-0135: Refactor Affinity Assistant PVC creation

[QuanZhang-William]

Part of #6740 and based on @lbernick's prototype. 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.

Before this commit, the PipelineRun reconciler creates PVC for each VolumeClaimTemplate backed workspace, and mount the PVCs to the AA to avoid PV availability zone conflict. This implementation works for AffinityAssistantPerWorkspace but introduces availability zone conflict issue in the AffinityAssistantPerPipelineRun mode since we cannot enforce all the PVC are created in the same availability zone.

Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate, this commit sets one VolumeClaimTemplate per PVC workspace in the affinity assistant StatefulSet spec, which enforces all VolumeClaimTemplates in StatefulSets are all provisioned on the same node/availability zone.

This commit just refactors the current implementation in favor of the AffinityAssistantPerPipelineRun feature. There is no functionality change. The AffinityAssistantPerPipelineRun feature will be added in the follow up PRs.

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: Update the owner of `PVCs` created by `pipelinerun VolumeClaimTemplate` to the affinity assistant `StatefulSet` when affinity assistant is enabled. The `PVCs` bounded to the `pipelinerun` is now in `terminating` state when the `pipelinerun` is completed but not deleted (when affinity assistant is enabled).

[tekton-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[tekton-robot]

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/reconciler/pipelinerun/affinity_assistant.go	94.5%	96.4%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.0%	-0.3

[QuanZhang-William]

/kind feature

[QuanZhang-William]

/test all

[tekton-robot]

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/pipelinerun/affinity_assistant.go	94.5%	96.4%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.0%	-0.3

[QuanZhang-William]

/test all

[tekton-robot]

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/pipelinerun/affinity_assistant.go	94.5%	96.5%	2.0
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.3%	-0.0

[tekton-robot]

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/reconciler/pipelinerun/affinity_assistant.go	94.5%	96.5%	2.0
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.3%	-0.0

[tekton-robot]

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/pipelinerun/affinity_assistant.go	94.5%	96.5%	2.0
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.3%	-0.0

[lbernick]

What integration tests do we currently have using VolumeClaimTemplate and PersistentVolumeClaim for workspace bindings?

[tekton-robot]

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/pipelinerun/affinity_assistant.go	94.5%	96.4%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.4%	-0.0

[tekton-robot]

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/reconciler/pipelinerun/affinity_assistant.go	94.5%	96.4%	1.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.4%	-0.0

[tekton-robot]

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/pipelinerun/affinity_assistant.go	94.5%	95.5%	0.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.4%	-0.0

[tekton-robot]

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/reconciler/pipelinerun/affinity_assistant.go	94.5%	95.5%	0.9
pkg/reconciler/pipelinerun/pipelinerun.go	90.4%	90.4%	-0.0

[QuanZhang-William]

/test pull-tekton-pipeline-go-coverage-df

[tekton-robot]

@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-go-coverage-df

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.

[tekton-robot]

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/reconciler/pipelinerun/affinity_assistant.go	94.5%	95.5%	1.0
pkg/reconciler/pipelinerun/pipelinerun.go	90.5%	90.6%	0.1

[pritidesai]

Hey @QuanZhang-William @lbernick I am trying to understand the changes made in this PR mainly focusing on no functionality changes. Please help understand!

Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate, this commit sets one VolumeClaimTemplate per PVC workspace in the affinity assistant StatefulSet spec, which enforces all VolumeClaimTemplates in StatefulSets are all provisioned on the same node/availability zone.

Do you have an example pipelineRun demonstrating impact of these changes? How does this impact the affinity assistant StatefulSet specification?

Let's say I have three workspaces (source1, source2, and source3). What would be the StatefulSet specification with and without these changes?

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  generateName: pipeline-run-
spec:
  workspaces:
    - name: source1
      volumeClaimTemplate:
        spec:
          ...
    - name: source2
      volumeClaimTemplate:
        spec:
          ...
    - name: source3
      volumeClaimTemplate:
        spec:
          ...

[pritidesai]

Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate,

There can only be a single workspace in a pipelineRun backed by VolumeClaimTemplate based on the validation at https://github.com/tektoncd/pipeline/blob/main/pkg/workspace/validate.go#L81

[QuanZhang-William]

Hey @QuanZhang-William @lbernick I am trying to understand the changes made in this PR mainly focusing on no functionality changes. Please help understand!

Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate, this commit sets one VolumeClaimTemplate per PVC workspace in the affinity assistant StatefulSet spec, which enforces all VolumeClaimTemplates in StatefulSets are all provisioned on the same node/availability zone.

Hi @pritidesai . In the PR, we preserve the current bahavior that PVCs will be created from pipelinerun'sVolumeClaimTemplate, so from user's perspective, there is no functionality change. However, we did change the way that how do these PVCs are created, which is an implementation detail and should be abstracted away from the users.

Do you have an example pipelineRun demonstrating impact of these changes? How does this impact the affinity assistant StatefulSet specification?

Taking an example from the unit test, before this change, the VolumeClaimTemplates field in the StatefulSetSpec is nil (as the PVCs are created irrelevant of AA before). After the change, the PVCs are created via AA's VolumeClaimTemplates, so the VolumeClaimTemplates is what is given in the test.

More details can be found in this section of TEP design

Let's say I have three workspaces (source1, source2, and source3). What would be the StatefulSet specification with and without these changes?

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  generateName: pipeline-run-
spec:
  workspaces:
    - name: source1
      volumeClaimTemplate:
        spec:
          ...
    - name: source2
      volumeClaimTemplate:
        spec:
          ...
    - name: source3
      volumeClaimTemplate:
        spec:
          ...

Taking an simplified yaml file with 1 VolumeClaimTemplate:

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  generateName: demo-pipeline-run-one-workspaces-only-
spec:
  pipelineSpec:
    workspaces:
    - name: git-source
    tasks:
     ...
  workspaces:
  - name: git-source
    volumeClaimTemplate:
      spec:
      ...

The AA Statefulset is created as:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: affinity-assistant-e6744f7e3a
spec:
  ...
  volumeClaimTemplates:
  - apiVersion: v1
    kind: PersistentVolumeClaim
    metadata:
      creationTimestamp: null
      name: pvc-45d7ac1f34
    spec:
      accessModes:
      - ReadWriteOnce
      resources:
        requests:
          storage: 1Gi
      volumeMode: Filesystem
...

The final PVC created via AA StatefulSet's volumeClaimTemplates is

k get pvc
NAME                                             STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
pvc-45d7ac1f34-affinity-assistant-e6744f7e3a-0   Terminating   pvc-5b671726-c277-49ed-a9b9-63101d65b589   1Gi        RWO            standard-rwo   5m33s

pipelierun with 3 VolumeClaimTemplates will result in 3 AA StatefulSets in the coschedule-workspaces mode (the current AA behavior)

[QuanZhang-William]

Instead of directly creating a PVC for each PipelineRun workspace backed by a VolumeClaimTemplate,

There can only be a single workspace in a pipelineRun backed by VolumeClaimTemplate based on the validation at https://github.com/tektoncd/pipeline/blob/main/pkg/workspace/validate.go#L81

We will keep this validation for coschedule-worspaces mode (current behavior), but we will lift this restriction if it is coschedule-pipelineruns or isolate-pipelineruns mode.

[pritidesai]

Thank you @QuanZhang-William for getting back, appreciate it!

k get pvc
NAME                                             STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
pvc-45d7ac1f34-affinity-assistant-e6744f7e3a-0   Terminating   pvc-5b671726-c277-49ed-a9b9-63101d65b589   1Gi        RWO            standard-rwo   5m33s

Please note, the state of the PVC, terminating compared to bound without these changes. Since now, the pvc is part of the StatefulSet which is deleted after the pipelineRun is compete and this StatefulSet deletion is invoking pvc deletion. But the pvc is protected by the k8s finalizer. Will this cause any impact on the end user? I am investigating as well.

[pritidesai]

k get pv   
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM                                                    STORAGECLASS   REASON   AGE
pvc-1d5c6dbf-8ee5-47f5-ab56-09a23c032cff   10Mi       RWO            Delete           Bound    default/pvc-ad8f4572f8                                   standard                77m
pvc-47698e7a-8fef-4725-8d72-312e0626e383   10Mi       RWO            Delete           Bound    default/pvc-ca83477b41-affinity-assistant-f3ae413abb-0   standard                65m


k get pvc
NAME                                             STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
pvc-ad8f4572f8                                   Bound         pvc-1d5c6dbf-8ee5-47f5-ab56-09a23c032cff   10Mi       RWO            standard       77m
pvc-ca83477b41-affinity-assistant-f3ae413abb-0   Terminating   pvc-47698e7a-8fef-4725-8d72-312e0626e383   10Mi       RWO            standard       66m

Here is the difference between the status of pvcs before and now.

I am thinking this state difference might not matter or impact any use case except the cluster will now have a tons of terminating pvcs instead of bound and which could confuse the cluster operator. The pvc along with pv are created from scratch for every pipelineRun. The pv is created with a reclaim policy of delete so once pvc is deleted the pv can be deleted as well. Now, I am not sure if there was any reuse use case supported i.e. if the pvc was left in a bound state to the pv, the same pvc can be reused if needed to access the respective pv but with terminating pvc, it is not possible any more.

[pritidesai]

Given this change in behavior, please update the release notes to reflect the status change. I humbly request to be cautious next time with these kind of changes, as they can go unnoticed until they are reported by the users.

[QuanZhang-William]

k get pv   
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM                                                    STORAGECLASS   REASON   AGE
pvc-1d5c6dbf-8ee5-47f5-ab56-09a23c032cff   10Mi       RWO            Delete           Bound    default/pvc-ad8f4572f8                                   standard                77m
pvc-47698e7a-8fef-4725-8d72-312e0626e383   10Mi       RWO            Delete           Bound    default/pvc-ca83477b41-affinity-assistant-f3ae413abb-0   standard                65m


k get pvc
NAME                                             STATUS        VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
pvc-ad8f4572f8                                   Bound         pvc-1d5c6dbf-8ee5-47f5-ab56-09a23c032cff   10Mi       RWO            standard       77m
pvc-ca83477b41-affinity-assistant-f3ae413abb-0   Terminating   pvc-47698e7a-8fef-4725-8d72-312e0626e383   10Mi       RWO            standard       66m

Here is the difference between the status of pvcs before and now.

I am thinking this state difference might not matter or impact any use case except the cluster will now have a tons of >terminating pvcs instead of bound and which could confuse the cluster operator.

Thanks, I agree this is a side effect of this change, and I will update the release note for it. Once the completed pipelineruns are deleted, the associated pvcs will be deleted as well.

The pvc along with pv are created from scratch for every pipelineRun. The pv is created with a reclaim policy of delete so once pvc is deleted the pv can be deleted as well. Now, I am not sure if there was any reuse use case supported i.e. if the pvc was left in a bound state to the pv, the same pvc can be reused if needed to access the respective pv but with terminating pvc, it is not possible any more.

Yeah, this is a good point, but I'm not sure if there is such a scenario (or is it right) for a user to rely on a pvc/pv created by a VolumeClaimTemplate in a pipelinerun 🤔 , as the way it is created is an implementation detail and there is no contract on how it is created.

/cc @lbernick

[QuanZhang-William]

Given this change in behavior, please update the release notes to reflect the status change. I humbly request to be cautious next time with these kind of changes, as they can go unnoticed until they are reported by the users.

Thanks for the reminder! I have updated the release note!

[lbernick]

Thanks @pritidesai for looking into this in more detail-- I did not realize the end state here would be "terminating" rather than "bound". It sounds like we should improve our integration test coverage for affinity assistant.
I also forgot that we actually previously cleaned up PVCs when the PipelineRun was deleted rather than when it was completed.

There are two decisions we need to make:

1. What's the ideal end state of this feature? I think it's probably correct for the PVC to be deleted when the PipelineRun is completed, since these PVCs are intended to be ephemeral storage for the duration of the PipelineRun, and to merge delete PVCs created by the controller using volumeClaimTemplate once pipelineRun is completed #6635 so that PVCs are actually deleted rather than just "terminating". Our integration tests should verify that PVCs are actually deleted when the PipelineRun is complete.
2. What's the best way to mitigate the impact of this change right now? Priti made a great point that having all the PVCs in "terminating" mode is confusing. If we don't have consensus on what the end state should be, we may want to go back to the previous behavior of deleting PVCs when the PipelineRun is deleted, not when it is completed, in order to have no functional changes while we are resolving this question. (I think we can keep the behavior of specifying volumeclaimtemplates on the statefulset though.)

@pritidesai @QuanZhang-William would love to hear your thoughts.

[QuanZhang-William]

Thanks @pritidesai for looking into this in more detail-- I did not realize the end state here would be "terminating" rather than "bound". It sounds like we should improve our integration test coverage for affinity assistant. I also forgot that we actually previously cleaned up PVCs when the PipelineRun was deleted rather than when it was completed.

There are two decisions we need to make:

1. What's the ideal end state of this feature? I think it's probably correct for the PVC to be deleted when the PipelineRun is completed, since these PVCs are intended to be ephemeral storage for the duration of the PipelineRun, and to merge delete PVCs created by the controller using volumeClaimTemplate once pipelineRun is completed #6635 so that PVCs are actually deleted rather than just "terminating". Our integration tests should verify that PVCs are actually deleted when the PipelineRun is complete.
2. What's the best way to mitigate the impact of this change right now? Priti made a great point that having all the PVCs in "terminating" mode is confusing. If we don't have consensus on what the end state should be, we may want to go back to the previous behavior of deleting PVCs when the PipelineRun is deleted, not when it is completed, in order to have no functional changes while we are resolving this question. (I think we can keep the behavior of specifying volumeclaimtemplates on the statefulset though.)

@pritidesai @QuanZhang-William would love to hear your thoughts.

To update the discussion on API WG:

We will support backwards compatibility for existing affinity assistant (coschedule-workspace) scenario, which means we will need to revert the PVC creation behavior (i.e. the owners of pvcs are prs instead of statefulset) so that the pvcs are kept in bound status when pipelinerun is completed but not deleted. In addition, we will provide an option for users to use if they choose to clean up the pvcs when the pr is completed, as required: #5776

For the new coscheduling mode pipelineruns and isolate-pipelinerun, we will enforce the deletion of such pvcs when pr is completed, as we do not recommend users to rely on such pvcs after the pr is completed.