Ginkgo: Add a option to run test in different vms #3120

eloycoto · 2018-03-13T10:27:39Z

This commit added a new flag --cilium.SSHConfig that it's a subcommand
that change vagrant ssh-config ${VM} to something that we want to have.

For example, if we want to run ginkgo test in a different VM, we can use
the following:

vagrant ssh-config ${ID} > ssh-config

And in that file, we should update the Host line to something that
matchs, like:

Host runtime --> For Runtime test
Host k8s1-1.9 --> For Kubernetes test
Host k8s2-1.9 --> For Kubernetes test

In the case of Kubernetes the value is 1.9, but that should match with
the value from ${K8S_VERSION}. (As described in the contributing guide)

This commit also updates helpers.BasePath, if the flag is defined, the
variable will change to ssh-host $GOPATH value or ${HOME}/go if the
GOPATH is not enabled.

This change fixes multiple approaches that ginkgo should cover:

@aanm Can dump the output of vagrant shh-config from his VMs and use
those servers to run the test.
@nebril Can use the test in minukube, a custom ssh-config can be created
file where host and ssh-key can be specified.
@jrfastab can create a custom ssh-config to run the test in baremetal.
ssh -G $(whoami)@127.0.0.1 can be used and added Host runtime in the
top
@joestringer can create a custom ssh-config and use on their laptop and
the system will pickup the $GOPATH environment variable and bazel
cache.(Feedback from issue #2900)
@me to run some test in Google Cloud.

Example:

ginkgo --focus "RuntimeValidatedLB" -v -- --cilium.provision=false --cilium.SSHConfig="cat ssh-config"

Signed-off-by: Eloy Coto [email protected]

eloycoto · 2018-03-13T10:28:22Z

test-me-please

aanm · 2018-03-13T11:42:23Z

test/helpers/node.go

+}
+
+// setBasePath if the SSHConfig is defined we set the BasePath to the GOPATH,
+// from golang 1.9 GOPATH is by default $HOME/go so we also check that.


from golang 1.8

aanm · 2018-03-13T11:42:46Z

test/helpers/cons.go

+const (
+
+	//CiliumPath the path where cilium test code is located.
+	CiliumPath = "github.com/cilium/cilum/test/"


remove trailing /

aanm · 2018-03-13T11:43:17Z

test/helpers/node.go

+		return
+	}
+
+	BasePath = filepath.Join(home, "go", CiliumPath)


Does BasePath needs to be public?

Yep, it's used by the PolicyGen in Nightly.

Seems like policygen.TestSpec and policygen.Target should be initialised with proper BasePath , and refer to it in their code. Also seems like other places using BasePath are already using SSHMeta, maybe it should also be stored in it? I think we need an issue to track this if this is too big to work on in this PR.

I think the above BasePath cleanup should be done in a separate PR this PR is useful even without the cleanup.

jrfastab · 2018-03-13T16:30:16Z

test/config/config.go

@@ -32,4 +33,6 @@ func (c *CiliumTestConfigType) ParseFlags() {
 		"Provision Vagrant boxes and Cilium before running test")
 	flag.BoolVar(&c.HoldEnvironment, "cilium.holdEnvironment", false,
 		"On failure, hold the environment in its current state")
+	flag.StringVar(&c.SSHConfig, "cilium.SSHConfig", "",
+		"Command to get a different a different SSH-config")


typo? "Command to get a different a different" should probably be "Command to get a different..."

ianvernon

Looks good overall, some minor typos.

ianvernon · 2018-03-13T21:40:38Z

test/helpers/vagrant.go

 	"github.com/onsi/ginkgo"
 	"github.com/sirupsen/logrus"
 )

-//Create a new vagrant server. Receives and scope that it's the target server that need to be created.
-// In case of any error on vagrant [provision|up|ssh-config] error will be returned.
+// CreateVM a new vagrant server. Receives and scope that it's the target


creates a new...

Receives a scope which indicates the target server that needs to be created.

ianvernon · 2018-03-13T21:40:50Z

test/helpers/vagrant.go

@@ -37,7 +40,7 @@ func CreateVM(scope string) error {
 		case "not_created":
 			createCMD = "vagrant up %s --provision"
 		default:
-			//Sometimes server are stoped and not destroyed. DestroyVM just in case
+			// Sometimes server are stoped and not destroyed. DestroyVM just in case


servers are stopped... Destroy VM...

joestringer · 2018-03-13T22:09:53Z

test/config/config.go

@@ -32,4 +33,6 @@ func (c *CiliumTestConfigType) ParseFlags() {
 		"Provision Vagrant boxes and Cilium before running test")
 	flag.BoolVar(&c.HoldEnvironment, "cilium.holdEnvironment", false,
 		"On failure, hold the environment in its current state")
+	flag.StringVar(&c.SSHConfig, "cilium.SSHConfig", "",
+		"Command to get a different a different SSHConfig")


Can we expand the description to state the expected format? ie,

"Specify a custom command to fetch SSH configuration (eg: 'vagrant ssh-config')"

Would be nice to add this to the contributing docs too.

eloycoto · 2018-03-14T07:50:11Z

test-me-please

nebril · 2018-03-14T14:07:34Z

test/helpers/cons.go

+
+const (
+
+	//CiliumPath the path where cilium test code is located.


Nitpick: s/CiliumPath the/CiliumPath is the

nebril · 2018-03-14T14:19:07Z

test/helpers/node.go

+		return
+	}
+
+	BasePath = filepath.Join(home, "go", CiliumPath)


Seems like policygen.TestSpec and policygen.Target should be initialised with proper BasePath , and refer to it in their code. Also seems like other places using BasePath are already using SSHMeta, maybe it should also be stored in it? I think we need an issue to track this if this is too big to work on in this PR.

jrfastab · 2018-03-14T14:52:08Z

test/helpers/cons.go

+const (
+
+	//CiliumPath the path where cilium test code is located.
+	CiliumPath = "/src/github.com/cilium/cilum/test"


typo ;) cilum -> cilium

eloycoto · 2018-03-16T08:46:51Z

Seems like policygen.TestSpec and policygen.Target should be initialised with proper BasePath , and refer to it in their code. Also seems like other places using BasePath are already using SSHMeta, maybe it should also be stored in it? I think we need an issue to track this if this is too big to work on in this PR.

@nebril The node is always used, and the BasePath will be updated, it has been used in initialize function before the test.

eloycoto · 2018-03-16T08:47:17Z

test-me-please

@aanm

This commit added a new flag --cilium.SSHConfig that it's a subcommand that change `vagrant ssh-config ${VM}` to something that we want to have. For example, if we want to run ginkgo test in a different VM, we can use the following: `vagrant ssh-config ${ID} > ssh-config` And in that file, we should update the Host line to something that matchs, like: `Host runtime` --> For Runtime test `Host k8s1-1.9` --> For Kubernetes test `Host k8s2-1.9` --> For Kubernetes test In the case of Kubernetes the value is 1.9, but that should match with the value from ${K8S_VERSION}. (As described in the contributing guide) This commit also updates helpers.BasePath, if the flag is defined, the variable will change to ssh-host $GOPATH value or ${HOME}/go if the GOPATH is not enabled. This change fixes multiple approaches that ginkgo should cover: @aanm Can dump the output of `vagrant shh-config` from his VMs and use those servers to run the test. @nebril Can use the test in minukube, a custom ssh-config can be created file where host and ssh-key can be specified. @jrfastab can create a custom ssh-config to run the test in baremetal. `ssh -G $(whoami)@127.0.0.1` can be used and added `Host runtime` in the top @joestringer can create a custom ssh-config and use on their laptop and the system will pickup the `$GOPATH` environment variable and bazel cache.(Feedback from issue cilium#2900) Signed-off-by: Eloy Coto <[email protected]>

Signed-off-by: Eloy Coto <[email protected]>

eloycoto · 2018-03-16T16:16:10Z

test-me-please

joestringer

This LGTM, I think we should merge it if it's not breaking anything and follow up on any tidyups/usage improvements once it's in.

eloycoto added kind/enhancement This would improve or streamline existing functionality. area/CI Continuous Integration testing issue or flake labels Mar 13, 2018

eloycoto requested review from joestringer, jrfastab and aanm March 13, 2018 10:27

eloycoto requested a review from a team as a code owner March 13, 2018 10:27

aanm reviewed Mar 13, 2018

View reviewed changes

eloycoto added the pending-review label Mar 13, 2018

jrfastab reviewed Mar 13, 2018

View reviewed changes

eloycoto force-pushed the SSHConfig branch from bc8becc to a59ecc5 Compare March 13, 2018 18:30

ianvernon reviewed Mar 13, 2018

View reviewed changes

joestringer reviewed Mar 13, 2018

View reviewed changes

eloycoto force-pushed the SSHConfig branch from a59ecc5 to e15da32 Compare March 14, 2018 07:49

nebril reviewed Mar 14, 2018

View reviewed changes

jrfastab reviewed Mar 14, 2018

View reviewed changes

eloycoto force-pushed the SSHConfig branch from e15da32 to 15e8128 Compare March 14, 2018 14:54

eloycoto added 2 commits March 16, 2018 17:15

Test: Fix typo in comment

c3d93e5

Signed-off-by: Eloy Coto <[email protected]>

eloycoto force-pushed the SSHConfig branch from e9ce4fd to c3d93e5 Compare March 16, 2018 16:15

joestringer approved these changes Mar 16, 2018

View reviewed changes

tgraf merged commit d535577 into cilium:master Mar 17, 2018

tgraf added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 17, 2018

tgraf changed the title ~~RFC: Ginkgo: Add a option to run test in different vms~~ Ginkgo: Add a option to run test in different vms Mar 17, 2018

eloycoto mentioned this pull request Mar 23, 2018

Ginkgo CI: ability to run ginkgo locally outside of Vagrant environment #1933

Closed

2 tasks

ianvernon mentioned this pull request Mar 23, 2018

Add ginkgo option to run ginkgo tests against developer VMs #2792

Closed

ianvernon mentioned this pull request Mar 23, 2018

cilium: ginkgo option 'useVagrant' to run tests without VMs #2780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ginkgo: Add a option to run test in different vms #3120

Ginkgo: Add a option to run test in different vms #3120

eloycoto commented Mar 13, 2018 •

edited

Loading

eloycoto commented Mar 13, 2018

aanm Mar 13, 2018

aanm Mar 13, 2018

aanm Mar 13, 2018

eloycoto Mar 13, 2018

nebril Mar 14, 2018

jrfastab Mar 14, 2018

jrfastab Mar 13, 2018

ianvernon left a comment

ianvernon Mar 13, 2018

ianvernon Mar 13, 2018

joestringer Mar 13, 2018

eloycoto commented Mar 14, 2018

nebril Mar 14, 2018

nebril Mar 14, 2018

jrfastab Mar 14, 2018

eloycoto commented Mar 16, 2018

eloycoto commented Mar 16, 2018

eloycoto commented Mar 16, 2018

joestringer left a comment


		const (

		//CiliumPath the path where cilium test code is located.

Ginkgo: Add a option to run test in different vms #3120

Ginkgo: Add a option to run test in different vms #3120

Conversation

eloycoto commented Mar 13, 2018 • edited Loading

eloycoto commented Mar 13, 2018

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

ianvernon 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

eloycoto commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloycoto commented Mar 16, 2018

eloycoto commented Mar 16, 2018

eloycoto commented Mar 16, 2018

joestringer left a comment

Choose a reason for hiding this comment

eloycoto commented Mar 13, 2018 •

edited

Loading