Redid test framework to bring up driver on instance and expose TCP socket with SSH Tunneling #71

davidz627 · 2018-07-17T22:12:16Z

This framework change should allow for better test result viewing as well as multi-zonal tests.
Necessary to write tests for: #13

Any refactoring advice is appreciated, I realize that the interfaces are not exactly "clean" but they work. I might leave it like this for now and refactor when I understand what the needs are more when more tests start getting written.

The single e2e test is also gutted right now but I will add the necessary parts back in in a follow-up PR, didn't want to bloat this one even more.

/assign @msau42 @saad-ali

…cket with SSH Tunneling

k8s-ci-robot · 2018-07-17T22:12:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627

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

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

msau42

The .tar.gz should probably not be checked in

msau42 · 2018-07-18T20:43:06Z

Makefile

@@ -23,7 +23,7 @@ all: gce-pd-driver
 gce-pd-driver:
 	mkdir -p bin
 	go build -ldflags "-X main.vendorVersion=${STAGINGVERSION}" -o bin/gce-pd-csi-driver ./cmd/
-	go test -c sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/e2e -o bin/e2e.test
+	#go test -c sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/e2e -o bin/e2e.test


Is the intent to uncomment this eventually?

dont need this bin anymore, removed

msau42 · 2018-07-18T20:44:30Z

pkg/gce-pd-csi-driver/server.go

@@ -111,6 +113,8 @@ func (s *nonBlockingGRPCServer) serve(endpoint string, ids csi.IdentityServer, c

 	glog.Infof("Listening for connections on address: %#v", listener.Addr())

-	server.Serve(listener)
+	if err := server.Serve(listener); err != nil {
+		glog.Warningf("Failed to serve: %v", err)


just a warning?

msau42 · 2018-07-18T20:49:20Z

test/binremote/binremote/archiver.go

+	binPath := path.Join(goPath, "src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/bin/gce-pd-csi-driver")
+
+	// TODO make the driver but from the base dir
+	out, err := exec.Command("go", "build", "-ldflags", "-X main.vendorVersion=latest", "-o", binPath, "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/cmd").CombinedOutput()


can you invoke 'make' instead so that it will use the makefile rules instead of recopying them here?

msau42 · 2018-07-18T20:49:56Z

test/binremote/binremote/archiver.go

+	if !ok {
+		return fmt.Errorf("Could not find environment variable GOPATH")
+	}
+	binPath := path.Join(goPath, "src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/bin/gce-pd-csi-driver")


Can you make these paths and "gce-pd" names configurable? (ie i want to use this too)

msau42 · 2018-07-18T20:51:40Z

test/binremote/binremote/instance.go

+
+const (
+	// workspaceDirPrefix is the string prefix used in the workspace directory name.
+	workspaceDirPrefix = "gce-pd-e2e-"


make this configurable

msau42 · 2018-07-18T20:53:05Z

test/binremote/binremote/runner.go

+
+	glog.V(2).Infof("Starting driver on %q", i.name)
+	// When the process is killed the driver should close the TCP endpoint, then we want to download the logs
+	output, err := i.SSH(fmt.Sprintf("sh -c '/usr/bin/nohup %s/gce-pd-csi-driver --endpoint=%s --nodeid=%s > %s/prog.out 2> %s/prog.err < /dev/null &'", workspace, endpoint, instanceName, workspace, workspace))


Can you make the driver command configurable?

msau42 · 2018-07-18T20:57:07Z

test/e2e/single_zone_e2e_test.go

+	runInProw      = flag.Bool("run-in-prow", false, "If true, use a Boskos loaned project and special CI service accounts and ssh keys")
+)
+
+func TestE2E(t *testing.T) {


Is the intention that you will be able to run multiple test files in one run? If so, you may want to move this "once per process" setup into its own file

msau42 · 2018-07-18T20:57:39Z

test/e2e/utils.go

+	}
+
+	// Create Driver Archive
+	archiveName := "e2e_gce_pd_test.tar.gz"


can you make this configurable

this is the configuration (?). I don't think the person running the test needs to worry about setting the name for the tarball that gets uploaded. In fact the argument could be made that this should just be not configurable at all and just called thepackage.tar.gz or something

msau42 · 2018-07-18T20:59:16Z

test/binremote/binremote/archiver.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Fix copyright.

msau42 · 2018-07-18T20:59:35Z

test/binremote/binremote/archiver.go

@@ -0,0 +1,82 @@
+/*


Can you flatten all these files down one directory? ie, get rid of the second binremote

…cgcepd specific code into variables

msau42 · 2018-07-19T00:22:31Z

test/e2e/single_zone_e2e_test.go

-	RunSpecs(t, "Google Compute Engine Persistent Disk Container Storage Interface Driver Tests")
-
-}
-
 var _ = BeforeSuite(func() {


should the before/after suite also go in the common e2e?

I don't think so. Because these before and after are specific to single zonal case. I envision another test file that has mutlizonal tests that will need its own separate setup and probably some refactoring :)

msau42 · 2018-07-19T00:28:40Z

test/e2e/utils.go

+	return instance, nil
+}
+
+func setupProwConfig() (project, serviceAccount string) {


Are all of these util methods being used?

can you separate out utils into a separate e2e/util pkg? Then that would make it easy for me to import

msau42 · 2018-07-19T00:29:15Z

test/e2e/client_wrappers.go

+limitations under the License.
+*/
+
+package e2e


Can you put this file into a e2e/util pkg?

Not sure if I would recommend importing these, the interfaces are going to change a whole bunch based on my needs. They're not full wrappers, you can see some of the calls are missing some parameters and substitute default ones. If a test ends up needing to supply those parameters at some point I will just change the interface as needed

msau42 · 2018-07-19T18:15:51Z

/lgtm

…ncy-openshift-4.18-ose-gcp-pd-csi-driver OCPBUGS-41071: Updating ose-gcp-pd-csi-driver-container image to be consistent with ART for 4.18

Redid test framework to bring up driver on instance and expose TCP so…

f01b25e

…cket with SSH Tunneling

k8s-ci-robot assigned msau42 and saad-ali Jul 17, 2018

k8s-ci-robot requested review from msau42 and saad-ali July 17, 2018 22:12

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 17, 2018

Added some file reading/writing with sketchy permissions hacks to test

fe136c9

msau42 reviewed Jul 18, 2018

View reviewed changes

Made framework more configurable by removing constants and gce-pd spe…

6b630c4

…cgcepd specific code into variables

davidz627 force-pushed the feature/e2eOverTCP branch from 89e4fb2 to 6b630c4 Compare July 19, 2018 00:19

msau42 reviewed Jul 19, 2018

View reviewed changes

Refacted e2e utils out to own package

6934153

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2018

k8s-ci-robot merged commit 8d2bd1a into kubernetes-sigs:master Jul 19, 2018

davidz627 deleted the feature/e2eOverTCP branch July 19, 2018 18:49

davidz627 mentioned this pull request Jul 19, 2018

E2E Framework Overhaul #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redid test framework to bring up driver on instance and expose TCP socket with SSH Tunneling #71

Redid test framework to bring up driver on instance and expose TCP socket with SSH Tunneling #71

davidz627 commented Jul 17, 2018 •

edited

Loading

k8s-ci-robot commented Jul 17, 2018

msau42 left a comment

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 19, 2018

msau42 Jul 18, 2018

davidz627 Jul 19, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 18, 2018

davidz627 Jul 18, 2018

msau42 Jul 19, 2018

davidz627 Jul 19, 2018

msau42 Jul 19, 2018

msau42 Jul 19, 2018

msau42 Jul 19, 2018

davidz627 Jul 19, 2018

msau42 commented Jul 19, 2018

Redid test framework to bring up driver on instance and expose TCP socket with SSH Tunneling #71

Redid test framework to bring up driver on instance and expose TCP socket with SSH Tunneling #71

Conversation

davidz627 commented Jul 17, 2018 • edited Loading

k8s-ci-robot commented Jul 17, 2018

msau42 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

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

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

msau42 commented Jul 19, 2018

davidz627 commented Jul 17, 2018 •

edited

Loading