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.

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

e2e: add timeout for pvc deletion in ephemeral e2e #4058

Merged
merged 1 commit into from
Aug 25, 2023
Merged

e2e: add timeout for pvc deletion in ephemeral e2e #4058

merged 1 commit into from
Aug 25, 2023

Conversation

riya-singhal31
Copy link
Contributor

@riya-singhal31 riya-singhal31 commented Aug 17, 2023

this commit adds the timeout to wait for pvc
deletion after the deletion of pod in a test.

fixes: #3910

@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.27

@mergify mergify bot added the component/testing Additional test cases or CI work label Aug 17, 2023
@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.27

1 similar comment
@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.27

Comment on lines +461 to +462
if isRetryableAPIError(err) {
framework.Logf("failed to verify deletion of PVC %s (status: %s): %v", pvcName, pvc.Status, err)

return false, nil
}
if !apierrs.IsNotFound(err) {
return false, fmt.Errorf("get on deleted PVC %v failed with error other than \"not found\": %w", pvcName, err)
}

// PVC has been successfully deleted
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riya-singhal31 Can you please check your logic flow here again?

This part of the code will never be executed since both err != nil and err == nil conditions are checked above and a exit from loop occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Rakshith for reviewing.
Updated, Can you please have a re-look.

Rakshith-R
Rakshith-R previously approved these changes Aug 22, 2023
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

nixpanic
nixpanic previously approved these changes Aug 22, 2023
@nixpanic
Copy link
Member

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2023

queue

🛑 The pull request rule doesn't match anymore

This action has been cancelled.

e2e/cephfs.go Outdated
@@ -399,6 +399,13 @@ var _ = Describe(cephfsType, func() {
if err != nil {
framework.Failf("failed to delete application: %v", err)
}

// wait for the associated PVC to be deleted
err = waitForPVCToBeDeleted(f.ClientSet, app.Namespace, "mypvc", deployTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not pass the hardcoded pvc name, read it from the app pod

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Madhu, done. PTAL

e2e/rbd.go Outdated
@@ -817,6 +817,13 @@ var _ = Describe("RBD", func() {
if err != nil {
framework.Failf("failed to delete application: %v", err)
}

// wait for the associated PVC to be deleted
err = waitForPVCToBeDeleted(f.ClientSet, app.Namespace, "mypvc", deployTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not pass the hardcoded pvc name, read it from the app pod

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Madhu-1, done. PTAL

@mergify mergify bot dismissed stale reviews from nixpanic and Rakshith-R August 22, 2023 14:57

Pull request has been modified.

@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.27

1 similar comment
@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.27

@riya-singhal31
Copy link
Contributor Author

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Aug 23, 2023

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Aug 23, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@nixpanic
Copy link
Member

Add a comment @mergifyio rebase once #4074 has been merged. After rebasing add the ok-to-test label.

@riya-singhal31
Copy link
Contributor Author

@Mergifyio rebase

this commit adds the timeout to wait for pvc
deletion after the deletion of pod in test.

Signed-off-by: riya-singhal31 <rsinghal@redhat.com>
@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2023

rebase

✅ Branch has been successfully rebased

@riya-singhal31 riya-singhal31 added the ok-to-test Label to trigger E2E tests label Aug 25, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 25, 2023
@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e/k8s-1.25

@riya-singhal31
Copy link
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.25

@riya-singhal31
Copy link
Contributor Author

/test ci/centos/k8s-e2e-external-storage/1.25

@nixpanic nixpanic added the ci/skip/multi-arch-build skip building on multiple architectures label Aug 25, 2023
@mergify mergify bot merged commit d435b59 into ceph:devel Aug 25, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/multi-arch-build skip building on multiple architectures component/testing Additional test cases or CI work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check/wait for PVC deletion after pod deletion in ephemeral volume support e2e case
5 participants