e2etest: adding and cleaning aws disk #188

riya-singhal31 · 2022-05-11T06:56:34Z

adds aws disk in the test namespace for resources to get installed.

Signed-off-by: riya-singhal31 rsinghal@redhat.com

riya-singhal31 · 2022-05-19T05:02:37Z

/test lvm-operator-bundle-e2e-aws

sp98

Added some minor comments. Overall looks good. Need to wait for the CI to pass though.
Mostly the review comments are around the disk count. Also, there are some linting errors that should be fixed.

sp98 · 2022-05-19T05:17:29Z

e2e/disk_setup.go

+		}
+
+		/*
+			// represents the disk layout to setup on the nodes.


Looks like this commented code is not needed. You can remove this.

sp98 · 2022-05-19T05:19:07Z

e2e/aws_disk.go

+
+func createAndAttachAWSVolumesForNode(t *testing.T, nodeEntry nodeDisks, ec2Client *ec2.EC2) {
+	node := nodeEntry.node
+	if len(nodeEntry.disks) > 20 {


nodeEntry.disks count is not a user input and is something that we are setting in the code. So it always be fixed.
So I think there is no need for this check.

sp98 · 2022-05-19T05:20:34Z

e2e/aws_disk.go

+		t.Fatalf("failed to create and attach aws disks for node %q: %+v", nodeEntry.node.Name, err)
+	}
+	volumes := make([]*ec2.Volume, len(nodeEntry.disks))
+	volumeLetters := []string{"g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"}


Do we need all these volume letters? I would keep it same as the disk count that we are using.

Kept 2 letters now, as we are creating 2 disks.

sp98 · 2022-05-19T05:38:30Z

e2e/disk_setup.go

+)
+
+func diskSetup() func(*testing.T) {
+	return func(t *testing.T) {


does this need to be a function inside another function ? For simplicity, I would prefer to keep it a single function if internal function is not needed here.

sp98 · 2022-05-19T05:38:40Z

e2e/disk_setup.go

+}
+
+func diskRemoval() func(*testing.T) {
+	return func(t *testing.T) {


does this need to be a function inside another function ? For simplicity, I would prefer to keep it a single function if internal function is not needed here.

riya-singhal31 · 2022-05-19T07:56:11Z

Added some minor comments. Overall looks good. Need to wait for the CI to pass though. Mostly the review comments are around the disk count. Also, there are some linting errors that should be fixed.

Thanks for the suggestions Santosh.
Updated the PR.

nbalacha · 2022-05-19T15:44:08Z

e2e/disk_setup.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func diskSetup(t *testing.T) {


Do we really need a testing.T argument here? It does not match the way the other functions have been written.
Please try to do this without testing.T

nbalacha · 2022-05-23T05:33:18Z

e2e/aws_disk.go

+	volumeIDs := make([]*string, 0)
+	instanceID, _, zone, err := getAWSNodeInfo(node)
+	if err != nil {
+		t.Errorf("failed to create and attach aws disks for node %q: %+v", nodeEntry.node.Name, err)


This will cause the tests to fail when run against our baremetal clusters.

When would we want to run the e2e tests on baremetal clusters?
Only for testing locally on SNO?

Yes, the e2e-tests are meant to be run against local clusters when required as well.

nbalacha

Please provide a way to skip the AWS disk setup and delete using command line glags or env variables. This is ensure that the tests do not fail when run against a non-aws cluster.
https://onsi.github.io/ginkgo/#supporting-custom-suite-configuration

nbalacha · 2022-05-23T07:43:59Z

e2e/disk_setup.go

+
+	// represents the disk layout to setup on the nodes.
+	var nodeEnv []nodeDisks
+	for _, disks := range nodeList.Items {


Why is the node being referred to by a variable called disk?

This overwrites the entries so only a single node will have disks attached.

nbalacha

Errors should be returned and checked wherever required.

nbalacha · 2022-05-25T05:49:52Z

Makefile

@@ -315,10 +315,12 @@ lvm-must-gather:
 # Variables required to run and build LVM end to end tests.
 LVM_OPERATOR_INSTALL ?= true
 LVM_OPERATOR_UNINSTALL ?= true
+DISK_INSTALL ?= false
+DISK_UNINSTALL ?= false


Not required - if the disk is created, it should be removed.

nbalacha · 2022-05-25T05:50:38Z

e2e/aws_disk.go

+	volumeIDs := make([]*string, 0)
+	instanceID, _, zone, err := getAWSNodeInfo(node)
+	if err != nil {
+		fmt.Printf("failed to create and attach aws disks for node %q: %+v\n", nodeEntry.node.Name, err)


The function should return here.

nbalacha · 2022-05-25T05:51:19Z

e2e/aws_disk.go

+	}
+}
+
+func createAndAttachAWSVolumesForNode(nodeEntry nodeDisks, ec2Client *ec2.EC2) {


This function should return an error.

nbalacha · 2022-05-25T05:54:03Z

e2e/aws_disk.go

+		volume, err := ec2Client.CreateVolume(createInput)
+		if err != nil {
+			err := fmt.Errorf("expect to create AWS volume with input %v: %w", createInput, err)
+			fmt.Printf("failed to create and attach aws disks for node %q: %+v\n", nodeEntry.node.Name, err)


Return on failure. This comment holds for all other errors in the function.

nbalacha · 2022-05-25T05:58:35Z

e2e/config.go

@@ -33,6 +39,8 @@ func init() {
 	flag.StringVar(&LvmSubscriptionChannel, "lvm-subscription-channel", "", "The subscription channel to revise updates from")
 	flag.BoolVar(&lvmOperatorInstall, "lvm-operator-install", true, "Install the LVM operator before starting tests")
 	flag.BoolVar(&lvmOperatorUninstall, "lvm-operator-uninstall", true, "Uninstall the LVM cluster and operator after test completion")
+	flag.BoolVar(&DiskInstall, "disk-install", false, "Do not install disks unless specified")
+	flag.BoolVar(&DiskUninstall, "disk-uninstall", false, "Do not uninstall disks unless specified")


Not required.

e2e/disk_setup.go

nbalacha · 2022-05-25T05:59:51Z

e2e/disk_setup.go

+
+	// represents the disk layout to setup on the nodes.
+	var nodeEnv []nodeDisks
+	for _, disks := range nodeList.Items {


Please rename the variable disks. The entry being referred to in the loop is a node.

nbalacha · 2022-05-25T06:03:04Z

Makefile

@@ -315,10 +315,12 @@ lvm-must-gather:
 # Variables required to run and build LVM end to end tests.
 LVM_OPERATOR_INSTALL ?= true
 LVM_OPERATOR_UNINSTALL ?= true
+DISK_INSTALL ?= false


Add a comment that this only handles AWS for now.

nbalacha · 2022-05-25T06:12:53Z

e2e/aws_disk.go

+}
+
+const (
+	awsPurposeTag       = "lvm-e2etest"


Suggested change

awsPurposeTag = "lvm-e2etest"

awsPurposeTag = odf-lvmo"

nbalacha · 2022-05-25T06:42:39Z

e2e/setup_teardown.go


 // BeforeTestSuiteSetup is the function called to initialize the test environment.
 func BeforeTestSuiteSetup() {

+	if DiskInstall {
+		debug("Creating disk for CI\n")
+		diskSetup()


This should return an error val which should be checked.

riya-singhal31 · 2022-05-25T06:55:37Z

/test lvm-operator-bundle-e2e-aws

nbalacha · 2022-05-25T13:29:25Z

Makefile

@@ -317,8 +317,11 @@ LVM_OPERATOR_INSTALL ?= true
 LVM_OPERATOR_UNINSTALL ?= true
 SUBSCRIPTION_CHANNEL ?= alpha

+# Handles only AWS as of now.
+DISK_INSTALL ?= true


Set this to false by default after confirming that the CI passes with it set to true.

Yes, I did that for checking purpose only.

nbalacha · 2022-05-25T13:31:02Z

e2e/aws_disk.go

+// this function is async
+func createAndAttachAWSVolumes(ec2Client *ec2.EC2, namespace string, nodeEnv []nodeDisks) {
+	for _, nodeEntry := range nodeEnv {
+		go createAndAttachAWSVolumesForNode(nodeEntry, ec2Client)


There needs to be a way to return an error to the calling function. You can skip the goroutine and do this serially for now.

An additional problem here is that the disks may not have been created or attached before the LVMO is deployed.

Yes, I'll do it serially.
That's right because PVC test is getting passed. Problem is that TopoLVM node pod is not ready, so its failing lvm cluster reconciliation test here.

nbalacha · 2022-05-25T13:34:52Z

e2e/setup_teardown.go


 // BeforeTestSuiteSetup is the function called to initialize the test environment.
 func BeforeTestSuiteSetup() {

+	if DiskInstall {
+		debug("Creating disk for CI\n")


Suggested change

debug("Creating disk for CI\n")

debug("Creating disks for e2e tests\n")

nbalacha · 2022-05-25T13:35:47Z

e2e/disk_setup.go

+	err = cleanupAWSDisks(ec2Client)
+	gomega.Expect(err).To(gomega.BeNil())
+
+	return nil


Suggested change

return nil

return err

nbalacha · 2022-05-25T13:37:51Z

e2e/disk_setup.go

+
+	// represents the disk layout to setup on the nodes.
+	var nodeEnv []nodeDisks
+	for _, nodes := range nodeList.Items {


Suggested change

for _, nodes := range nodeList.Items {

for _, node := range nodeList.Items {

riya-singhal31 · 2022-05-26T06:24:29Z

/retest

riya-singhal31 · 2022-05-26T09:46:17Z

/retest

nbalacha · 2022-05-27T05:32:48Z

e2e/config.go

@@ -33,6 +36,7 @@ func init() {
 	flag.StringVar(&LvmSubscriptionChannel, "lvm-subscription-channel", "", "The subscription channel to revise updates from")
 	flag.BoolVar(&lvmOperatorInstall, "lvm-operator-install", true, "Install the LVM operator before starting tests")
 	flag.BoolVar(&lvmOperatorUninstall, "lvm-operator-uninstall", true, "Uninstall the LVM cluster and operator after test completion")
+	flag.BoolVar(&DiskInstall, "disk-install", true, "Do not install disks unless specified")


Suggested change

flag.BoolVar(&DiskInstall, "disk-install", true, "Do not install disks unless specified")

flag.BoolVar(&DiskInstall, "disk-install", true, "Create and attach disks to the nodes. This currently only works with AWS")

e2e/disk_setup.go

nbalacha · 2022-05-27T05:35:51Z

e2e/setup_teardown.go

@@ -44,4 +52,10 @@ func AfterTestSuiteCleanup() {
 		err = DeployManagerObj.UninstallLVM(LvmCatalogSourceImage, LvmSubscriptionChannel)
 		gomega.Expect(err).To(gomega.BeNil(), "error uninstalling LVM: %v", err)


Suggested change

gomega.Expect(err).To(gomega.BeNil(), "error uninstalling LVM: %v", err)

gomega.Expect(err).To(gomega.BeNil(), "error uninstalling the LVM Operator: %v", err)

nbalacha · 2022-05-27T05:37:00Z

e2e/validation.go

+		}
+		return ds.Status.DesiredNumberScheduled == ds.Status.NumberReady
+	}, timeout, interval).Should(BeTrue())
+	debug("Status is ready\n")


Suggested change

debug("Status is ready\n")

debug("TopolvmNode Status is ready\n")

riya-singhal31 · 2022-05-27T08:45:00Z

/retest

nbalacha · 2022-05-27T10:52:41Z

e2e/aws_disk.go

+// do not provide more than 20 disksize
+// do not use more than once per node
+// this function is async
+func createAndAttachAWSVolumes(ec2Client *ec2.EC2, namespace string, nodeEnv []nodeDisks) error {


Looks like the namespace is not required and can be removed.

nbalacha · 2022-05-27T10:53:33Z

e2e/disk_setup.go

+)
+
+func diskSetup() error {
+	namespace := TestNamespace


Looks like the namespace var is not used and can be removed.

nbalacha · 2022-05-27T10:54:20Z

e2e/disk_setup.go

+	gomega.SetDefaultEventuallyTimeout(time.Minute * 10)
+	gomega.SetDefaultEventuallyPollingInterval(time.Second * 2)


Where is this being used?

adds aws disk in the test namespace for resources to get installed. Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

riya-singhal31 · 2022-05-27T11:53:44Z

Thanks @nbalacha for all the optimization suggestions and corrections.
Updated the PR.

openshift-ci · 2022-05-27T13:01:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

iamniting · 2022-06-15T11:46:19Z

This PR also updates prometheus/client_golang which is required to be backported for https://bugzilla.redhat.com/show_bug.cgi?id=2056104

/cherry-pick release-4.11

openshift-cherrypick-robot · 2022-06-15T11:46:57Z

@iamniting: new pull request created: #209

In response to this:

This PR also updates prometheus/client_golang which is required to be backported for https://bugzilla.redhat.com/show_bug.cgi?id=2056104

/cherry-pick release-4.11

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2022

riya-singhal31 force-pushed the aws-disk branch 3 times, most recently from 4ac3307 to 699c483 Compare May 12, 2022 07:48

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2022

riya-singhal31 force-pushed the aws-disk branch from 699c483 to 8e00641 Compare May 12, 2022 07:55

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2022

riya-singhal31 force-pushed the aws-disk branch 2 times, most recently from 7effd2f to 50aa67d Compare May 12, 2022 08:08

riya-singhal31 changed the title ~~[WIP]e2etest: adding and cleaning aws disk~~ e2etest: adding and cleaning aws disk May 19, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2022

riya-singhal31 requested a review from sp98 May 19, 2022 05:11

sp98 suggested changes May 19, 2022

View reviewed changes

openshift-ci bot assigned sp98 May 19, 2022

sp98 suggested changes May 19, 2022

View reviewed changes

riya-singhal31 force-pushed the aws-disk branch from 50aa67d to 9846d7b Compare May 19, 2022 07:54

riya-singhal31 force-pushed the aws-disk branch 2 times, most recently from d55c34a to 3727440 Compare May 19, 2022 14:47

nbalacha reviewed May 19, 2022

View reviewed changes

nbalacha reviewed May 23, 2022

View reviewed changes

nbalacha suggested changes May 23, 2022

View reviewed changes

openshift-ci bot assigned nbalacha May 23, 2022

nbalacha suggested changes May 23, 2022

View reviewed changes

riya-singhal31 force-pushed the aws-disk branch from 3727440 to 892dafb Compare May 24, 2022 14:34

nbalacha suggested changes May 25, 2022

View reviewed changes

riya-singhal31 force-pushed the aws-disk branch from 892dafb to d3a8108 Compare May 25, 2022 13:26

riya-singhal31 changed the title ~~e2etest: adding and cleaning aws disk~~ [WIP]e2etest: adding and cleaning aws disk May 25, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2022

riya-singhal31 force-pushed the aws-disk branch from d3a8108 to 5fda98f Compare May 25, 2022 14:09

nbalacha suggested changes May 25, 2022

View reviewed changes

riya-singhal31 force-pushed the aws-disk branch 2 times, most recently from dad4b48 to bbb018a Compare May 26, 2022 05:56

riya-singhal31 changed the title ~~[WIP]e2etest: adding and cleaning aws disk~~ e2etest: adding and cleaning aws disk May 26, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2022

nbalacha reviewed May 27, 2022

View reviewed changes

e2e/disk_setup.go Show resolved Hide resolved

nbalacha reviewed May 27, 2022

View reviewed changes

riya-singhal31 force-pushed the aws-disk branch from bbb018a to cfa0745 Compare May 27, 2022 06:22

riya-singhal31 force-pushed the aws-disk branch from cfa0745 to 92444c4 Compare May 27, 2022 09:41

nbalacha reviewed May 27, 2022

View reviewed changes

test: adding and cleaning of disk for CI

1352c85

adds aws disk in the test namespace for resources to get installed. Signed-off-by: riya-singhal31 <rsinghal@redhat.com>

riya-singhal31 force-pushed the aws-disk branch from 92444c4 to 1352c85 Compare May 27, 2022 11:52

nbalacha approved these changes May 27, 2022

View reviewed changes

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

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

openshift-merge-robot merged commit 192622d into openshift:main May 27, 2022

openshift-cherrypick-robot mentioned this pull request Jun 15, 2022

Bug 2056104: [release-4.11] e2etest: adding and cleaning aws disk #209

Closed

	debug("Creating disk for CI\n")
	debug("Creating disks for e2e tests\n")

	for _, nodes := range nodeList.Items {
	for _, node := range nodeList.Items {

	flag.BoolVar(&DiskInstall, "disk-install", true, "Do not install disks unless specified")
	flag.BoolVar(&DiskInstall, "disk-install", true, "Create and attach disks to the nodes. This currently only works with AWS")

		@@ -44,4 +52,10 @@ func AfterTestSuiteCleanup() {
		err = DeployManagerObj.UninstallLVM(LvmCatalogSourceImage, LvmSubscriptionChannel)
		gomega.Expect(err).To(gomega.BeNil(), "error uninstalling LVM: %v", err)

	debug("Status is ready\n")
	debug("TopolvmNode Status is ready\n")

		gomega.SetDefaultEventuallyTimeout(time.Minute * 10)
		gomega.SetDefaultEventuallyPollingInterval(time.Second * 2)

e2etest: adding and cleaning aws disk #188

e2etest: adding and cleaning aws disk #188

Conversation

riya-singhal31 commented May 11, 2022 • edited Loading

riya-singhal31 commented May 19, 2022

sp98 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented May 19, 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

nbalacha left a comment

Choose a reason for hiding this comment

nbalacha May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha 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

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

riya-singhal31 commented May 26, 2022

riya-singhal31 commented May 26, 2022

Choose a reason for hiding this comment

nbalacha May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented May 27, 2022

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 27, 2022 • edited Loading

openshift-ci bot commented May 27, 2022

iamniting commented Jun 15, 2022

openshift-cherrypick-robot commented Jun 15, 2022

riya-singhal31 commented May 11, 2022 •

edited

Loading

riya-singhal31 commented May 19, 2022 •

edited

Loading

nbalacha May 23, 2022 •

edited

Loading

nbalacha May 27, 2022 •

edited

Loading

riya-singhal31 commented May 27, 2022 •

edited

Loading