test: to check the status of pvc as bound and pod as running #162

riya-singhal31 · 2022-04-20T13:20:33Z

this test creates pvc and pod and verifies the status of pvc to be bound and pod to be running, and cleans them up.

Signed-off-by: riya-singhal31 [email protected]

riya-singhal31 · 2022-04-20T13:25:34Z

Output:
[riyasinghal@fedora lvm-operator]$ oc get pvc,pods
NAME STATUS VOLUME CAPACITY ACCESS MODES STORAGECLASS AGE
persistentvolumeclaim/lvmblockpvc Bound pvc-8d8a4385-99c0-46b5-a0ba-235983d2aed9 5Gi RWO odf-lvm-vg1 10s
persistentvolumeclaim/lvmfilepvc Bound pvc-244885b9-7e9d-40b4-88cb-956bb040bdd6 5Gi RWO odf-lvm-vg1 12s

NAME READY STATUS RESTARTS AGE
pod/lvmblockpod 1/1 Running 0 10s
pod/lvmfilepod 1/1 Running 0 12s

Madhu-1 · 2022-04-21T03:25:03Z

e2e/common.go

+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "PersistentVolumeClaim",
+		},


You dont need to add the API version and kind

Madhu-1 · 2022-04-21T03:25:16Z

e2e/common.go

+		Spec: k8sv1.PersistentVolumeClaimSpec{
+			StorageClassName: &storageClass,
+			AccessModes:      []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce},
+


new line not required.

Madhu-1 · 2022-04-21T03:25:20Z

e2e/common.go

+			AccessModes:      []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce},
+
+			VolumeMode: &volumemode,
+


new line not required.

Madhu-1 · 2022-04-21T03:26:29Z

e2e/common.go

+
+			Resources: k8sv1.ResourceRequirements{
+				Requests: k8sv1.ResourceList{
+					"storage": storageQuantity,


Suggested change

"storage": storageQuantity,

v1.ResourceStorage: resource.MustParse(quantity),

Madhu-1 · 2022-04-21T03:28:03Z

e2e/common.go

+			APIVersion: "v1",
+			Kind:       "Pod",


same here as well.

Madhu-1 · 2022-04-21T03:30:01Z

e2e/common.go

+	if err != nil && !errors.IsAlreadyExists(err) {
+		return err
+	}
+	pod, err = DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{})


Suggested change

pod, err = DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{})

_, err = DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{})

Madhu-1 · 2022-04-21T03:32:48Z

e2e/common.go

+			return false, nil
+		}
+
+		if pvc.Status.Phase != k8sv1.ClaimBound {


if the error is not found you need to continue of else this case cause panic as the Status might be nil

This should never be NotFound as the PVC was just created.

👍 above notFound error check need to be removed.

Madhu-1 · 2022-04-21T03:36:01Z

e2e/lvm/lvm_suite_test.go

+				Expect(err).To(BeNil())
+
+				err = tests.WaitForPVCBound(blockpvc, namespace, blockpod)
+				fmt.Printf("%v", err)


not required

Madhu-1 · 2022-04-21T03:36:11Z

e2e/lvm/lvm_suite_test.go

+			It("and verify bound status", func() {
+				By("Should be bound")
+				err := tests.WaitForPVCBound(filepvc, namespace, filepod)
+				fmt.Printf("%v", err)


printf not required.

Madhu-1 · 2022-04-21T03:37:09Z

e2e/lvm/lvm_suite_test.go

+			err = tests.DeployManagerObj.GetK8sClient().CoreV1().PersistentVolumeClaims(namespace).Delete(context.TODO(), filepvc.Name, metav1.DeleteOptions{})
+			if err != nil && !errors.IsNotFound(err) {
+				Expect(err).To(BeNil())
+			}


PVC deletion will not ensure PV is deleted. you might need to add a check to validate that PV is also deleted.

I think we can take that up in a later PR. For now, the LVMCluster cleanup will fail if the PV still exists.

riya-singhal31 · 2022-04-21T05:25:12Z

/test lvm-operator-bundle-e2e-aws

riya-singhal31 · 2022-04-21T06:10:44Z

Updated the PR to address suggestions, thanks.

Madhu-1

small nits.

Madhu-1 · 2022-04-21T06:24:01Z

e2e/common.go

+	_, err := resource.ParseQuantity(quantity)
+	gomega.Expect(err).To(gomega.BeNil())


this is not required. as we are directly using quantity at line 38

Madhu-1 · 2022-04-21T06:24:22Z

e2e/common.go

 )

 //nolint:errcheck
 func debug(msg string, args ...interface{}) {
 	ginkgo.GinkgoWriter.Write([]byte(fmt.Sprintf(msg, args...)))
 }
+
+// GetSamplePVC returns a sample pvc.
+func GetSamplePVC(storageClass string, quantity string, name string, volumemode k8sv1.PersistentVolumeMode) *k8sv1.PersistentVolumeClaim {


Suggested change

func GetSamplePVC(storageClass string, quantity string, name string, volumemode k8sv1.PersistentVolumeMode) *k8sv1.PersistentVolumeClaim {

func GetSamplePVC(storageClass , quantity , name string, volumemode k8sv1.PersistentVolumeMode) *k8sv1.PersistentVolumeClaim {

Madhu-1 · 2022-04-21T06:24:38Z

e2e/common.go

+}
+
+// GetSamplePod returns a sample pod.
+func GetSamplePod(name string, pvcName string) *k8sv1.Pod {


Suggested change

func GetSamplePod(name string, pvcName string) *k8sv1.Pod {

func GetSamplePod(name , pvcName string) *k8sv1.Pod {

Madhu-1 · 2022-04-21T06:26:14Z

e2e/common.go

+	if err != nil {
+		return fmt.Errorf("%v: %s", err, lastReason)
+	}
+


You also need to check pod is in a running the state?

Thanks for the suggestion,
Added the function for same.

leelavg · 2022-04-22T03:47:10Z

e2e/lvm/lvm_suite_test.go

@@ -33,3 +36,59 @@ var _ = Describe("Validation test", func() {
 		})
 	})
 })
+
+// Test to verify the PVC status
+var _ = Describe("PVC Status check", VerifyPVCStatus)


IMHO refactoring it like below will be good as we are going to expand the test coverage for components

in e2e/lvm/lvm_suite_test.go everything upto line 28 will stay as it is

after that it'll take below form

var _ = Describe("LVMO", func() { Context("lvmcluster", <funcWhichValidatesResources>) Contextt("pvc", <funcWhichValidatesPVCops>) })

and the functions will be in package e2e (existing or new file) but not in lvm_suite_test.go, makes sense?

Thanks for pointing this out Leela,
I've now made separate files for separate tests now, as lvm_suite_test.go will makes more sense if it contains only setup and teardown part.

leelavg · 2022-04-22T03:52:36Z

e2e/common.go

 )

 //nolint:errcheck
 func debug(msg string, args ...interface{}) {
 	ginkgo.GinkgoWriter.Write([]byte(fmt.Sprintf(msg, args...)))
 }
+
+// GetSamplePVC returns a sample pvc.


if there's a possibility that the GetSample* methods should return different fields and values it's better to consider moving templates into a separate folder (let's say testdata to confirm with go recommendations) and manipulate those templates?

it'll be like, you have a PVC template and deep copy that at call site and set the fields

leelavg · 2022-04-22T03:54:57Z

e2e/common.go

+
+// WaitForPVCBound waits for a pvc with a given name and namespace to reach BOUND phase.
+func WaitForPVCBound(pvc *k8sv1.PersistentVolumeClaim, namespace string, pod *k8sv1.Pod) error {
+	pvc, err := DeployManagerObj.GetK8sClient().CoreV1().PersistentVolumeClaims(namespace).Create(context.TODO(), pvc, metav1.CreateOptions{})


we can define a single context or declare a pkg level context and use the same?

Yes, thanks for the suggestion!
That will need to be done in pkg/deploymanager.
Will add this while changing multiple clients to single client and will create a separate PR for that concern.

leelavg · 2022-04-22T04:03:47Z

e2e/lvm/lvm_suite_test.go

+		})
+
+		AfterEach(func() {
+			err := tests.DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Delete(context.TODO(), filepod.Name, metav1.DeleteOptions{})


if we are going to use k8sClient multiple times in multiple places why not register that in BeforeSuite (create a var at pkg level) if SuiteFailed to False and use that throughout?

iirc there was also an effort to unify all clients used in the test and having that at pkg level might be helpful?

Yes, will unify all clients in the next PR.

riya-singhal31 · 2022-04-25T05:48:16Z

/test lvm-operator-bundle-e2e-aws

leelavg

some more refactoring can be done however this pr is good enough for the functionality it's testing

nbalacha · 2022-05-06T10:29:49Z

pkg/deploymanager/LVMCluster.go

+	err = utilwait.PollImmediate(interval, timeout, func() (done bool, err error) {
+		err = t.crClient.Get(context.TODO(), types.NamespacedName{Name: lvmClusterRes.Name, Namespace: InstallNamespace}, cluster)
+		if err != nil && errors.IsNotFound(err) {
+			//lastReason = fmt.Sprintf("Error talking to k8s apiserver: %v", err)


please remove this.

Sorry didn't notice that. Updated.

nbalacha · 2022-05-06T10:29:56Z

pkg/deploymanager/LVMCluster.go

+			return true, nil
+		}
+		if err == nil {
+			//lastReason = "Waiting on namespace to be deleted"


Please remove this.

this test creates pvc and pod and verifies the status of pvc to be bound, and deletes them. Signed-off-by: riya-singhal31 <[email protected]>

riya-singhal31 · 2022-05-06T12:20:07Z

/retest

openshift-ci · 2022-05-06T12:25:20Z

@riya-singhal31: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lvm-operator-bundle-e2e-aws	`ebae206`	link	false	`/test lvm-operator-bundle-e2e-aws`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci · 2022-05-09T10:19:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leelavg, nbalacha, riya-singhal31

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

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

riya-singhal31 requested a review from nbalacha April 20, 2022 13:39

Madhu-1 suggested changes Apr 21, 2022

View reviewed changes

openshift-ci bot assigned Madhu-1 Apr 21, 2022

riya-singhal31 force-pushed the pvcbound branch from 67d2af4 to 9f18504 Compare April 21, 2022 06:06

Madhu-1 reviewed Apr 21, 2022

View reviewed changes

leelavg reviewed Apr 22, 2022

View reviewed changes

riya-singhal31 force-pushed the pvcbound branch 3 times, most recently from 9053d43 to 6ab87b2 Compare April 22, 2022 11:58

riya-singhal31 changed the title ~~test: to check the status of pvc as bound~~ test: to check the status of pvc as bound and pod as running Apr 25, 2022

leelavg approved these changes Apr 25, 2022

View reviewed changes

openshift-ci bot assigned leelavg Apr 25, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2022

riya-singhal31 force-pushed the pvcbound branch from 6ab87b2 to 973cbac Compare April 25, 2022 11:29

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2022

riya-singhal31 force-pushed the pvcbound branch from 973cbac to 1486ee3 Compare April 27, 2022 09:17

riya-singhal31 force-pushed the pvcbound branch from 1486ee3 to 169216e Compare May 6, 2022 07:03

nbalacha reviewed May 6, 2022

View reviewed changes

test: to check the status of pvc as bound

ebae206

this test creates pvc and pod and verifies the status of pvc to be bound, and deletes them. Signed-off-by: riya-singhal31 <[email protected]>

riya-singhal31 force-pushed the pvcbound branch from 169216e to ebae206 Compare May 6, 2022 11:26

nbalacha approved these changes May 9, 2022

View reviewed changes

openshift-ci bot assigned nbalacha May 9, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2022

openshift-merge-robot merged commit 4e44ceb into openshift:main May 9, 2022

		AccessModes: []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce},

		VolumeMode: &volumemode,

	"storage": storageQuantity,
	v1.ResourceStorage: resource.MustParse(quantity),

	pod, err = DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{})
	_, err = DeployManagerObj.GetK8sClient().CoreV1().Pods(namespace).Create(context.TODO(), pod, metav1.CreateOptions{})

		_, err := resource.ParseQuantity(quantity)
		gomega.Expect(err).To(gomega.BeNil())

	func GetSamplePVC(storageClass string, quantity string, name string, volumemode k8sv1.PersistentVolumeMode) *k8sv1.PersistentVolumeClaim {
	func GetSamplePVC(storageClass , quantity , name string, volumemode k8sv1.PersistentVolumeMode) *k8sv1.PersistentVolumeClaim {

	func GetSamplePod(name string, pvcName string) *k8sv1.Pod {
	func GetSamplePod(name , pvcName string) *k8sv1.Pod {

test: to check the status of pvc as bound and pod as running #162

test: to check the status of pvc as bound and pod as running #162

Conversation

riya-singhal31 commented Apr 20, 2022 • edited Loading

riya-singhal31 commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Apr 21, 2022

riya-singhal31 commented Apr 21, 2022

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Apr 25, 2022

leelavg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented May 6, 2022

openshift-ci bot commented May 6, 2022

openshift-ci bot commented May 9, 2022

riya-singhal31 commented Apr 20, 2022 •

edited

Loading

riya-singhal31 Apr 22, 2022 •

edited

Loading

riya-singhal31 Apr 22, 2022 •

edited

Loading