[WIP] Gke integration tests #319

hantaowang · 2019-06-25T19:12:33Z

This PR clarifies versioning with the deployment strategies gce and gke.

test-version is used to determine the k8s version to test with.
kube-version and gke-cluster-version is used to determine the cluster version to boot, depeonding on the deployment strategy.
local-k8s-dir points to a rebuilt kubernetes directory. If this flag is set, then neither kube-version nor test-version should be because the local version is used for both cluster and tests.
if using GKE: either set (gke-cluster-version and test-version) or (gke-cluster-version and local-k8s-dir)
if using GCE: either set (local-k8s-dir) or (kube-version and test-version)

It also cleans up some code with the ensureVariable, ensureFlag, getTempDir, removeTempDir helper functions.

It also makes certain time consuming tasks run concurrently. Tell me if this is not desired.

Changed how versioning flags work with the deployment strategy and adds a test-version flag to separate from test and cluster versioning.

k8s-ci-robot · 2019-06-25T19:12:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hantaowang
To complete the pull request process, please assign saad-ali
You can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2019-06-25T19:12:47Z

Hi @hantaowang. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

davidz627 · 2019-06-25T19:14:34Z

/ok-to-test

kube version

hantaowang · 2019-06-25T21:46:37Z

/assign @davidz627

davidz627

Thanks for going above and beyond and coming up with ways to speed up the testing infra. However, unless we are able to abstract away the dependency chain blocking and error channel handling I don't think we'll be able to merge this as is.

The main concern I have is "what if I need to add an extra step in the integration test process, how many separate pieces of code not related to my own function do I need to change and how easy it is to shoot myself in the foot".

I think there is probably a way to abstract away these things with contexts and wait groups but no obvious elegant solution immediately comes to mind.

It looks like several non-related improvements were made in the same PR. In general that will slow down the entire PR (if there is an issue/concern with one improvement it will block all other changes from getting in). Small, targeted PRs are your friend. This monolith could be broken down into:

Flag handling improvements PR
Split functions to files PR
Parallelism PR
New flags/functionality for testing version PR

In that way I think all but the parallelism PR would be no-brainers to merge, we could move forward with other things and bikeshed/work on the parallelism PR to our hearts content.

test/k8s-integration/main.go

davidz627 · 2019-06-26T00:17:38Z

test/k8s-integration/main.go

+	defer removeTempDir(k8sParentDir)
+	defer removeTempDir(testParentDir)
+
+	numTasks := 4


Can it be a const? Why is the numTasks=4, could probably use a comment

davidz627 · 2019-06-26T00:20:24Z

test/k8s-integration/main.go

+	if len(*testVersion) != 0 && *testVersion != *kubeVersion {
+		go func() {
+			// TODO: Build only the tests
+			err := downloadKubernetesSource(pkgDir, testParentDir, *testVersion)


can we download only one version of the source and just copy it if we need it for test dir

I think it makes it a lot more complicated dependency wise, so I'll think about it.

davidz627 · 2019-06-26T00:21:07Z

test/k8s-integration/main.go

+	// Otherwise, either kube version is set (which implies GCE) or a local K8s dir is being used
+	if len(*testVersion) != 0 && *testVersion != *kubeVersion {
+		go func() {
+			// TODO: Build only the tests


make WHAT=test/e2e/e2e.test

davidz627 · 2019-06-26T00:30:12Z

test/k8s-integration/main.go

+		}()
+	} else {
+		testDir = k8sDir
+		errChan <- nil


kind of odd to me that we need to push nil errors onto the errChan like this. Feel like we should be able to have something watch the errChan for any actual errors and halt the entire test suite (and execute all defers for cleanup).

Maybe something like a watcher that watches the error channel that pushes a cancel signal to a context that all goroutines take in. Let me know if this would make it more complicated, but I feel like it's more extensible so we don't have to do an error prone counting of errChan signals as we add/remove things from the integration testing

davidz627 · 2019-06-26T00:31:41Z

test/k8s-integration/main.go

+
+	numTasks := 4
+	errChan := make(chan error, numTasks)
+	k8sDependencyChan := make(chan bool, 1)


can we use named waitgroups to capture dependency chains where certain operations have to wait for groups of other operations to complete before we can proceed.

davidz627 · 2019-06-26T00:43:27Z

/cc @msau42 @jingxu97

hantaowang · 2019-06-26T01:30:39Z

I'll let this PR sit for now, but I've already moved some of the small changes into other PRs. See #322 for the utility changes and #323 for the test-version flag related changes. When those are merged I will open a PR to break apart the huge main.go. Then I will look into the concurrency changes - if time permits.

k8s-ci-robot · 2019-06-28T04:49:59Z

@hantaowang: PR needs rebase.

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.

hantaowang · 2019-06-28T23:36:16Z

/close

k8s-ci-robot · 2019-06-28T23:36:17Z

@hantaowang: Closed this PR.

In response to this:

/close

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.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 25, 2019

k8s-ci-robot requested review from davidz627 and verult June 25, 2019 19:12

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 25, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2019

hantaowang force-pushed the gke-integration-tests branch from 8a4e02b to c26d342 Compare June 25, 2019 19:13

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2019

hantaowang force-pushed the gke-integration-tests branch from c26d342 to 7be1b1d Compare June 25, 2019 21:07

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2019

hantaowang force-pushed the gke-integration-tests branch from 7be1b1d to ec898fb Compare June 25, 2019 21:08

Hantao (Will) Wang added 2 commits June 25, 2019 14:32

add background building and flag to specify test version spearate from

319631b

kube version

break up large file into smaller pieces

2a91a29

hantaowang force-pushed the gke-integration-tests branch from 325d779 to 2a91a29 Compare June 25, 2019 21:32

k8s-ci-robot assigned davidz627 Jun 25, 2019

davidz627 suggested changes Jun 26, 2019

View reviewed changes

k8s-ci-robot requested review from jingxu97 and msau42 June 26, 2019 00:43

hantaowang mentioned this pull request Jun 26, 2019

add flag checking and temp dir utilities #322

Merged

hantaowang mentioned this pull request Jun 26, 2019

Separate test k8s #323

Merged

davidz627 changed the title ~~Gke integration tests~~ [WIP] Gke integration tests Jun 26, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2019

hantaowang mentioned this pull request Jun 27, 2019

move helper functions to more specific files #326

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2019

k8s-ci-robot closed this Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Gke integration tests #319

[WIP] Gke integration tests #319

hantaowang commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

davidz627 commented Jun 25, 2019

hantaowang commented Jun 25, 2019

davidz627 left a comment

davidz627 Jun 26, 2019

davidz627 Jun 26, 2019

hantaowang Jun 26, 2019

davidz627 Jun 26, 2019

davidz627 Jun 26, 2019

davidz627 Jun 26, 2019

davidz627 commented Jun 26, 2019

hantaowang commented Jun 26, 2019

k8s-ci-robot commented Jun 28, 2019

hantaowang commented Jun 28, 2019

k8s-ci-robot commented Jun 28, 2019

[WIP] Gke integration tests #319

[WIP] Gke integration tests #319

Conversation

hantaowang commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

k8s-ci-robot commented Jun 25, 2019

davidz627 commented Jun 25, 2019

hantaowang commented Jun 25, 2019

davidz627 left a comment

Choose a reason for hiding this comment

davidz627 Jun 26, 2019

Choose a reason for hiding this comment

davidz627 Jun 26, 2019

Choose a reason for hiding this comment

hantaowang Jun 26, 2019

Choose a reason for hiding this comment

davidz627 Jun 26, 2019

Choose a reason for hiding this comment

davidz627 Jun 26, 2019

Choose a reason for hiding this comment

davidz627 Jun 26, 2019

Choose a reason for hiding this comment

davidz627 commented Jun 26, 2019

hantaowang commented Jun 26, 2019

k8s-ci-robot commented Jun 28, 2019

hantaowang commented Jun 28, 2019

k8s-ci-robot commented Jun 28, 2019