e2e: test for PVC with volumeBindingMode on helm installation #5243

ShravaniVangur · 2025-03-27T17:20:52Z

Describe what this PR does

Test PVC binding with WaitForFirstConsumer in Helm installation.

Is there anything that requires special attention

Do you have any questions?

Is the change backward compatible?

Are there concerns around backward compatibility?

Provide any external context for the change, if any.

For example:

Kubernetes links that explain why the change is required
CSI spec related changes/catch-up that necessitates this patch
golang related practices that necessitates this change

Related issues

Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.

Fixes: #5127

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

ShravaniVangur · 2025-03-27T17:30:38Z

/test ci/centos/mini-e2e-operator/k8s-1.31

iPraveenParihar · 2025-03-28T05:12:22Z

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

ShravaniVangur · 2025-04-01T10:14:06Z

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

ShravaniVangur · 2025-04-01T14:27:05Z

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

nixpanic

Looks quite complete, just added some general notes and suggestions for improving.

nixpanic · 2025-04-03T08:15:17Z

e2e/cephfs.go

 					}
 				})
+
+				By("verify PVC with volumeBindingMode on helm installation", func() {


What makes this special or unique for a Helm installation? This probably runs on non-Helm installations too? And, it should work on non-Helm installations as well.

This test was added as a follow-up to PR #5126, which introduced changes to the CephFS charts. However, since the functionality should also be compatible with non-Helm installations, the test has been moved out of helmTest.

nixpanic · 2025-04-03T08:18:47Z

e2e/cephfs_helper.go

+	return createStorageClass(c, sc)
+}
+
+func updateStorageClassParameters(sc *scv1.StorageClass, params map[string]string, enablePool bool, f *framework.Framework) (*scv1.StorageClass, error) {


Why return the StorageClass? The passed sc is updated, just as the function name indicates. The returned sc is not new (or a copy). Just return an error only to prevent any confusion.

nixpanic · 2025-04-03T08:20:59Z

e2e/cephfs_helper.go

 			if isRetryableAPIError(err) {
 				return false, nil
 			}
-


Here, an in other places, you removed an empty line before return. The coding style in Ceph-CSI (and golangci-lint?) expects these. Please add them back.

nixpanic · 2025-04-03T08:23:27Z

e2e/utils.go

+		return err
+	}
+
+	//verifies that the pvc is in pending state


Comments are mostly written with a space after //.

This comment here does not add much value, the function is pretty much self explaining. It would be better to add a comment above the function itself, which explains that validatePVCAndAppWaitForFirstConsumer 1st waits for pending, then starts an app and then makes sure the claim is bound.

nixpanic · 2025-04-03T08:27:24Z

e2e/utils.go

+	return wait.PollUntilContextTimeout(context.TODO(), poll, timeout, true, func(ctx context.Context) (bool, error) {
+		pvc, err := c.CoreV1().PersistentVolumeClaims(namespace).Get(context.TODO(), name, metav1.GetOptions{})
+		if err != nil {
+			return false, fmt.Errorf("error fetching PVC %s: %w", name, err)


nit: I have a preference to use %q for names of objects, instead of %s. It reads nicer when variables in error and log messages are quoted.

ShravaniVangur · 2025-04-07T10:13:07Z

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

Madhu-1

can we add the same for RBD as well (a follow up).

e2e/utils.go

e2e/pvc.go

e2e/cephfs_helper.go

e2e/utils.go

e2e/cephfs.go

Madhu-1

LGTM. can you please add the same for RBD and NFS if its not present in follow up PR.

nixpanic · 2025-04-23T14:54:32Z

@Mergifyio queue

mergify · 2025-04-23T14:54:55Z

queue

🛑 The pull request has been removed from the queue `default`

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details about the failure.

mergify · 2025-04-23T16:50:35Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Test PVC binding with WaitForFirstConsumer in Helm installation. Signed-off-by: ShravaniVangur <[email protected]>

Pull request has been modified.

ShravaniVangur · 2025-04-24T06:51:27Z

@nixpanic @Madhu-1 Rebase was necessary due to recent changes in the Mergify.io configuration file. Kindly requesting reviews.

Madhu-1 · 2025-04-24T07:44:41Z

@Mergifyio queue

nixpanic · 2025-04-24T07:44:45Z

@Mergifyio queue

mergify · 2025-04-24T07:44:51Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c19f472

ceph-csi-bot · 2025-04-24T07:45:05Z

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

ceph-csi-bot · 2025-04-24T07:45:05Z

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

ceph-csi-bot · 2025-04-24T07:45:06Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-04-24T07:45:06Z

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

ceph-csi-bot · 2025-04-24T07:45:06Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-04-24T07:45:06Z

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

ceph-csi-bot · 2025-04-24T07:45:07Z

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

ceph-csi-bot · 2025-04-24T07:45:07Z

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

ceph-csi-bot · 2025-04-24T07:45:08Z

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

ceph-csi-bot · 2025-04-24T07:45:08Z

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

ceph-csi-bot · 2025-04-24T07:45:09Z

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

mergify · 2025-04-24T09:46:39Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

mergify bot added component/testing Additional test cases or CI work component/deployment Helm chart, kubernetes templates and configuration Issues/PRs labels Mar 27, 2025

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from f578c07 to cfd8f7b Compare April 1, 2025 09:54

ShravaniVangur marked this pull request as ready for review April 1, 2025 14:52

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from cfd8f7b to b777b4c Compare April 2, 2025 09:22

nixpanic reviewed Apr 3, 2025

View reviewed changes

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from b777b4c to 2662ef9 Compare April 7, 2025 10:05

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from 2662ef9 to 4eb795e Compare April 7, 2025 13:54

ShravaniVangur requested a review from nixpanic April 7, 2025 14:15

Madhu-1 reviewed Apr 8, 2025

View reviewed changes

e2e/utils.go Outdated Show resolved Hide resolved

e2e/pvc.go Outdated Show resolved Hide resolved

e2e/cephfs_helper.go Outdated Show resolved Hide resolved

e2e/cephfs_helper.go Outdated Show resolved Hide resolved

e2e/utils.go Outdated Show resolved Hide resolved

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from 4eb795e to 771b690 Compare April 14, 2025 10:56

ShravaniVangur requested a review from Madhu-1 April 14, 2025 11:32

Madhu-1 reviewed Apr 14, 2025

View reviewed changes

e2e/cephfs.go Show resolved Hide resolved

Madhu-1 reviewed Apr 14, 2025

View reviewed changes

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from 771b690 to 3823b42 Compare April 16, 2025 08:50

ShravaniVangur requested a review from Madhu-1 April 17, 2025 05:35

Madhu-1 previously approved these changes Apr 17, 2025

View reviewed changes

nixpanic previously approved these changes Apr 23, 2025

View reviewed changes

e2e: test for PVC with volumeBindingMode on helm installation

00505e4

Test PVC binding with WaitForFirstConsumer in Helm installation. Signed-off-by: ShravaniVangur <[email protected]>

ShravaniVangur force-pushed the e2eVolumeBindingMode branch from 3823b42 to 00505e4 Compare April 24, 2025 05:54

ShravaniVangur requested review from Madhu-1 and nixpanic April 24, 2025 06:51

nixpanic approved these changes Apr 24, 2025

View reviewed changes

nixpanic requested a review from a team April 24, 2025 07:30

Madhu-1 approved these changes Apr 24, 2025

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Apr 24, 2025

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 24, 2025

mergify bot merged commit c19f472 into ceph:devel Apr 24, 2025
36 of 37 checks passed

Uh oh!

e2e: test for PVC with volumeBindingMode on helm installation #5243

e2e: test for PVC with volumeBindingMode on helm installation #5243

Uh oh!

Conversation

ShravaniVangur commented Mar 27, 2025

Describe what this PR does

Is there anything that requires special attention

Related issues

Future concerns

Uh oh!

ShravaniVangur commented Mar 27, 2025

Uh oh!

iPraveenParihar commented Mar 28, 2025

Uh oh!

ShravaniVangur commented Apr 1, 2025

Uh oh!

ShravaniVangur commented Apr 1, 2025

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

ShravaniVangur Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

nixpanic Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

ShravaniVangur commented Apr 7, 2025

Uh oh!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Madhu-1 left a comment

Choose a reason for hiding this comment

Uh oh!

nixpanic commented Apr 23, 2025

Uh oh!

mergify bot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

mergify bot commented Apr 23, 2025

Uh oh!

ShravaniVangur commented Apr 24, 2025

Uh oh!

Madhu-1 commented Apr 24, 2025

Uh oh!

nixpanic commented Apr 24, 2025

Uh oh!

mergify bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

ceph-csi-bot commented Apr 24, 2025

Uh oh!

ceph-csi-bot commented Apr 24, 2025

Uh oh!

ceph-csi-bot commented Apr 24, 2025

Uh oh!

ceph-csi-bot commented Apr 24, 2025

Uh oh!

mergify bot commented Apr 23, 2025 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Apr 24, 2025 •

edited

Loading