Fix race condition in PVC deletion #7149

QuanZhang-William · 2023-09-25T17:30:17Z

Changes

fixes #7138. In coschedule:pipelineruns mode, the pvcs created by VolumeClaimTemplate should be automatically deleted when the owning pipelinerun is completed. To delete a pvc used by a pod (pipelinerun), the kubernetes.io/pvc-protection finalizer must be removed.

Prior to this commit, the Tekton reconciler attempted to remove the kubernetes.io/pvc-protection finalizer first and then deletes the created pvcs. This results in race condition since PVCProtectionController tries to add back the finalizer if the pvc is in bounded status . If the add back action happens before the PVC deletion call is completed, the PVC will be left in terminating status.

This commit changes the reconciler to delete the PVC first and then removes the finalizer to fix the race condition. This commit also adds integration test to validate pvc status when a pipelinerun is completed.

/kind bug

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

Bug fix: delete PVCs created by VolumeClaimTemplates when the owning PipelineRun is completed

tekton-robot · 2023-09-25T18:37:35Z

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/volumeclaim/pvchandler.go	82.6%	54.3%	-28.3

tekton-robot · 2023-09-25T18:38:27Z

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/volumeclaim/pvchandler.go	82.6%	54.3%	-28.3

pkg/reconciler/volumeclaim/pvchandler.go

dibyom · 2023-09-26T17:05:14Z

pkg/reconciler/volumeclaim/pvchandler_test.go

@@ -243,35 +242,3 @@ func TestCreateExistPersistentVolumeClaims(t *testing.T) {
 		t.Fatalf("unexpected PVC name on created PVC; expected: %s got: %s", expectedPVCName, pvcList.Items[0].Name)
 	}
 }
-
-func TestPurgeFinalizerAndDeletePVCForWorkspace(t *testing.T) {


why was this test deleted? it should still be valid no?

It seems like the simple client set cannot mock the pvc with finalizer scenario. Specifically, the pvcs are already successfully deleted in the first API call, so the second call to remove finalizer will fail since the pvc is already deleted.

I could potentially use some expected errors in the test case to compare, but I feel it is quite confusing. And the functionality is already covered by integration test.

got it, one way to do this could be to use something like PrependReactor for the delete call and have it set the deletionTimestamp instead of deleting the object

not exactly the same thing, but similar:

pipeline/pkg/reconciler/pipelinerun/affinity_assistant_test.go

Lines 435 to 438 in f5578a8

c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("create", "persistentvolumeclaims",

func(action testing2.Action) (handled bool, ret runtime.Object, err error) {

return true, &corev1.PersistentVolumeClaim{}, errors.New("error creating persistentvolumeclaims")

})

Thanks for the advice, I have added the test back with your suggestion. PTAL!

JeromeJu · 2023-09-27T14:57:00Z

test/wait.go

+	return pollImmediateWithContext(ctx, func() (bool, error) {
+		pvcList, err := c.KubeClient.CoreV1().PersistentVolumeClaims(namespace).List(ctx, metav1.ListOptions{})
+		if err != nil {
+			return true, err


Would you mind adding docString here for the indications of the bool error being returned?

I think it just follows the expected function signature, and k8s does have docstring for it:

pipeline/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go

Line 209 in c2858d8

// ConditionFunc returns true if the condition is satisfied, or an error

JeromeJu · 2023-10-02T15:44:05Z

/lgtm

QuanZhang-William · 2023-10-02T16:21:38Z

/test pull-tekton-pipeline-go-coverage

fixes [tektoncd#7138][tektoncd#7138]. In `coschedule:pipelineruns` mode, the `pvcs` created by `VolumeClaimTemplate` should be automatically deleted when the owning `pipelinerun` is completed. To delete a `pvc` used by a pod (pipelinerun), the [kubernetes.io/pvc-protection][finalizer] finalizer must be removed. Prior to this commit, the Tekton reconciler attempted to remove the `kubernetes.io/pvc-protection` finalizer first and then deletes the created pvcs. This results in race condition since `PVCProtectionController` tries to add back the finalizer if the `pvc` is in `bounded` status . If the add back action happens before the PVC deletion call is completed, the `PVC` will be left in `terminating` status. This commit changes the reconciler to delete the `PVC` first and then removes the `finalizer` to fix the race condition. This commit removes `TestPurgeFinalizerAndDeletePVCForWorkspace` UT, since the finalizer behavior cannot be mocked when a PVC is deleted. This commit also adds integration test to cover this scenario. /kind bug [tektoncd#7138]: tektoncd#7138 [finalizer]: https://kubernetes.io/docs/concepts/storage/persistent-volumes/#storage-object-in-use-protection

QuanZhang-William · 2023-10-02T17:57:09Z

@dibyom @JeromeJu . I have further improved unit test, can you please take another look?

dibyom · 2023-10-03T18:24:29Z

Nicely done! thanks @QuanZhang-William
/approve

tekton-robot · 2023-10-03T18:24:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, Yongxuanzhang

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~~ [dibyom]

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

Yongxuanzhang · 2023-10-03T18:50:19Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2023

tekton-robot requested review from jerop and pritidesai September 25, 2023 17:30

QuanZhang-William force-pushed the fix-purge-pvc-order branch from 2b52afc to 5e40cff Compare September 25, 2023 18:30

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 Sep 25, 2023

dibyom reviewed Sep 26, 2023

View reviewed changes

JeromeJu reviewed Sep 27, 2023

View reviewed changes

QuanZhang-William force-pushed the fix-purge-pvc-order branch from 5e40cff to 097cfd0 Compare September 28, 2023 15:50

tekton-robot assigned JeromeJu Oct 2, 2023

dibyom assigned dibyom and unassigned JeromeJu Oct 2, 2023

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

QuanZhang-William force-pushed the fix-purge-pvc-order branch from 097cfd0 to c5a2c80 Compare October 2, 2023 17:11

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2023

Yongxuanzhang approved these changes Oct 2, 2023

View reviewed changes

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

tekton-robot assigned Yongxuanzhang Oct 3, 2023

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

tekton-robot merged commit 5a68859 into tektoncd:main Oct 3, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in PVC deletion #7149

Fix race condition in PVC deletion #7149

QuanZhang-William commented Sep 25, 2023

tekton-robot commented Sep 25, 2023

tekton-robot commented Sep 25, 2023

dibyom Sep 26, 2023

QuanZhang-William Sep 27, 2023

dibyom Sep 27, 2023 •

edited

Loading

QuanZhang-William Sep 28, 2023

JeromeJu Sep 27, 2023

QuanZhang-William Sep 28, 2023

JeromeJu commented Oct 2, 2023

QuanZhang-William commented Oct 2, 2023

QuanZhang-William commented Oct 2, 2023

dibyom commented Oct 3, 2023

tekton-robot commented Oct 3, 2023

Yongxuanzhang commented Oct 3, 2023

	c.KubeClientSet.CoreV1().(*fake.FakeCoreV1).PrependReactor("create", "persistentvolumeclaims",
	func(action testing2.Action) (handled bool, ret runtime.Object, err error) {
	return true, &corev1.PersistentVolumeClaim{}, errors.New("error creating persistentvolumeclaims")
	})

Fix race condition in PVC deletion #7149

Fix race condition in PVC deletion #7149

Conversation

QuanZhang-William commented Sep 25, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 25, 2023

tekton-robot commented Sep 25, 2023

dibyom Sep 26, 2023

Choose a reason for hiding this comment

QuanZhang-William Sep 27, 2023

Choose a reason for hiding this comment

dibyom Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

QuanZhang-William Sep 28, 2023

Choose a reason for hiding this comment

JeromeJu Sep 27, 2023

Choose a reason for hiding this comment

QuanZhang-William Sep 28, 2023

Choose a reason for hiding this comment

JeromeJu commented Oct 2, 2023

QuanZhang-William commented Oct 2, 2023

QuanZhang-William commented Oct 2, 2023

dibyom commented Oct 3, 2023

tekton-robot commented Oct 3, 2023

Yongxuanzhang commented Oct 3, 2023

dibyom Sep 27, 2023 •

edited

Loading