kubetest: add "KubectlCommand" to "deployer" interface #10216

gyuho · 2018-11-22T09:17:50Z

For kubernetes/kubernetes#71322 and https://github.com/aws/aws-k8s-tester.

Update EKS test plugin vendor to configure kubectl download path
Use that download path to configure "kubectl version" commands in e2e tests

Our issue was that all e2e tests were failing as below:

W1121 23:44:50.939] 2018/11/21 23:44:50 main.go:312: Something went wrong: encountered 2 errors: [error during ./cluster/kubectl.sh --match-server-version=false version: exit status 1 error during ./hack/ginkgo-e2e.sh --ginkgo.focus=[Conformance] --ginkgo.skip=[Slow]|[Serial]|[Disruptive]|[Flaky]|[Feature:.+] --minStartupPods=8 --report-dir=/workspace/_artifacts --disable-log-dump=true: exit status 1]

And this Something went wrong error happens when

test-infra/kubetest/main.go

Lines 406 to 408 in db6a12c

    
           if err := run(deploy, *o); err != nil { 
        
           	return err 
        
           }

ref. #9814

gyuho · 2018-11-26T18:27:39Z

@BenTheElder @krzyzacy Please review. Had to disable EKS jobs (by deleting release binaries) because getKubectlVersion kept failing for eks provider :)

krzyzacy · 2018-11-26T18:35:45Z

how about make it into the deployer interface?

gyuho · 2018-11-27T00:30:44Z

/hold cancel

krzyzacy · 2018-11-27T00:47:54Z

kubetest/conformance/conformance.go

+		}
+		log.Print("Failed to reach api. Sleeping for 10 seconds before retrying...")
+		time.Sleep(10 * time.Second)
+	}


I would say conformance deployer should do noop?

also @BenTheElder we can probably get rid of the old dind conformance deployer?

@krzyzacy @BenTheElder Just removed and made it no-op in kubetest/conformance. Thanks!

yeah I don't think anyone is using the conformance deployer, I'll figure out something else for kind very soon...

krzyzacy · 2018-11-27T00:56:14Z

kubetest/kubeadmdind/kubeadm_dind.go

+		}
+		log.Print("Failed to reach api. Sleeping for 10 seconds before retrying...")
+		time.Sleep(10 * time.Second)
+	}


this is duplicated logic.. maybe move the getKubectlVersion to util package so you can reuse it here?

@krzyzacy Found out that would make util package import process package, thus cyclic imports. Do you have any other suggestion?

we should probably break those up into more specific sub-packages

@BenTheElder Can't think of better place to put getKubectlVersion other than kubetest/e2e package. Or maybe kubetest/client?

or simply make a new package.. other deployers will need some cleanup to move to their own packages as well

BenTheElder · 2018-11-27T18:00:55Z

Thought: what if we made the path to kubectl or a method to obtain an exec.Command to it instead? GetKubectlVersion seems like an awfully specific thing for deployers to re-implement.

gyuho · 2018-11-27T18:14:59Z

@BenTheElder Good idea. Let me change GetKubectlVersion to something else and let each provider provide parameters to kubectl.

gyuho · 2018-11-27T19:45:34Z

@BenTheElder @krzyzacy Now this adds KubectlArgs to deployer interface, so that each provider configures its own parameters to test API reachability.

gyuho · 2018-11-28T18:59:47Z

@BenTheElder @krzyzacy Kindly ping. Thanks!

krzyzacy · 2018-11-28T19:09:43Z

looks sane, punt to @BenTheElder see if this matches his thoughts :-)

BenTheElder · 2018-11-29T07:27:38Z

kubetest/anywhere.go

@@ -291,6 +291,10 @@ func (k *kubernetesAnywhere) GetClusterCreated(gcpProject string) (time.Time, er
 	return time.Time{}, errors.New("not implemented")
 }

+func (_ *kubernetesAnywhere) KubectlArgs() ([]string, error) {


seems that KubectlArgs actually returns one of:

nil

[]string{cmd, args...}

Maybe call this KubectlCommand and return an *exec.Cmd or nil
If it's nil, then kubetest can use a default via a wrapper like runKubectl, and if it's non-nil kubetest can still append to the args with Cmd.Args = append(Cmd.Args, ...)

Good idea. Let me change it :)

thanks 👍

BenTheElder · 2018-11-29T07:28:59Z

kubetest/e2e.go

+	if err != nil {
+		return err
+	}
+	if len(args) < 1 { // no-op


probably we should default to just "kubectl" instead?

gyuho · 2018-11-30T00:08:28Z

@BenTheElder All addressed. PTAL. Thanks!

Signed-off-by: Gyuho Lee <[email protected]>

BenTheElder · 2018-11-30T07:28:56Z

kubetest/e2e.go

+		return err
+	}
+	if cmd == nil {
+		cmd = exec.Command("./cluster/kubectl.sh")


note to self / @krzyzacy : eventually we should stop using cluster/*.sh

I think this is indeed correct for now though of course :-)

yeah... I wonder if we should just dump the extracted kubectl into PATH or something...

@krzyzacy Can we get this merged first? Please let me know if there's anything else to be done here :)

BenTheElder

/lgtm
/hold
thanks -- @krzyzacy any last comments?

k8s-ci-robot · 2018-11-30T07:30:02Z

LGTM label has been added.

Git tree hash: 3815fe5bf7dde51367e90b7fba851139f5f60654

k8s-ci-robot · 2018-11-30T07:30:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, gyuho

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

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

krzyzacy · 2018-11-30T18:58:05Z

/hold cancel

gyuho · 2018-11-30T19:08:17Z

/test pull-test-infra-verify-deps

k8s-ci-robot · 2018-11-30T19:17:04Z

@gyuho: Updated the job-config configmap using the following files:

key k8s-aws-eks-1.10.yaml using file config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-1.10.yaml

In response to this:

For kubernetes/kubernetes#71322 and https://github.com/aws/aws-k8s-tester.

Update EKS test plugin vendor to configure kubectl download path

Use that download path to configure "kubectl version" commands in e2e tests

Our issue was that all e2e tests were failing as below:

W1121 23:44:50.939] 2018/11/21 23:44:50 main.go:312: Something went wrong: encountered 2 errors: [error during ./cluster/kubectl.sh --match-server-version=false version: exit status 1 error during ./hack/ginkgo-e2e.sh --ginkgo.focus=[Conformance] --ginkgo.skip=[Slow]|[Serial]|[Disruptive]|[Flaky]|[Feature:.+] --minStartupPods=8 --report-dir=/workspace/_artifacts --disable-log-dump=true: exit status 1]

And this Something went wrong error happens when

test-infra/kubetest/main.go

Lines 406 to 408 in db6a12c

if err := run(deploy, *o); err != nil {

return err

}

ref. #9814

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2018

k8s-ci-robot requested review from cjwagner and d-nishi November 22, 2018 09:18

k8s-ci-robot added the area/config Issues or PRs related to code in /config label Nov 22, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2018

gyuho mentioned this pull request Nov 26, 2018

ekstester: add "GetKubectlVersion" aws/aws-k8s-tester#16

Merged

gyuho changed the title ~~kubetest/e2e.go: make "getKubectlVersion" configurable per provider~~ kubetest: add "GetKubectlVersion" to deployer interface Nov 27, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2018

krzyzacy reviewed Nov 27, 2018

View reviewed changes

gyuho changed the title ~~kubetest: add "GetKubectlVersion" to deployer interface~~ kubetest: add "KubectlArgs" to "deployer" interface Nov 27, 2018

BenTheElder reviewed Nov 29, 2018

View reviewed changes

gyuho changed the title ~~kubetest: add "KubectlArgs" to "deployer" interface~~ kubetest: add "KubectlCommand" to "deployer" interface Nov 29, 2018

gyuho added 5 commits November 29, 2018 16:47

config/.../eks: set download paths for kubectl, aws-iam=authenticator

437273a

Signed-off-by: Gyuho Lee <[email protected]>

kubetest/eks: use "AWSK8sTesterPath" to download test plugins

2a76821

Signed-off-by: Gyuho Lee <[email protected]>

kubetest/eks: add "KubectlCommand" to EKS provider

9af8afc

Signed-off-by: Gyuho Lee <[email protected]>

kubetest: implement "KubectlCommand"

d6e2af9

Signed-off-by: Gyuho Lee <[email protected]>

vendor: upgrade "aws/aws-k8s-tester"

f6b86a0

Signed-off-by: Gyuho Lee <[email protected]>

BenTheElder reviewed Nov 30, 2018

View reviewed changes

BenTheElder approved these changes Nov 30, 2018

View reviewed changes

k8s-ci-robot assigned BenTheElder Nov 30, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 30, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2018

k8s-ci-robot merged commit 0587c47 into kubernetes:master Nov 30, 2018

gyuho deleted the eks-kubectl-setting branch November 30, 2018 19:17

This was referenced Nov 30, 2018

cluster/eks: initial commit kubernetes/kubernetes#71322

Closed

kubernetes/kubernetes.cluster/kubectl.sh fails aws/aws-k8s-tester#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubetest: add "KubectlCommand" to "deployer" interface #10216

kubetest: add "KubectlCommand" to "deployer" interface #10216

gyuho commented Nov 22, 2018 •

edited

Loading

gyuho commented Nov 26, 2018

krzyzacy commented Nov 26, 2018

gyuho commented Nov 27, 2018

krzyzacy Nov 27, 2018

krzyzacy Nov 27, 2018

gyuho Nov 27, 2018

BenTheElder Nov 27, 2018

krzyzacy Nov 27, 2018

gyuho Nov 27, 2018

BenTheElder Nov 27, 2018

gyuho Nov 27, 2018

krzyzacy Nov 27, 2018

BenTheElder commented Nov 27, 2018

gyuho commented Nov 27, 2018 •

edited

Loading

gyuho commented Nov 27, 2018

gyuho commented Nov 28, 2018

krzyzacy commented Nov 28, 2018

BenTheElder Nov 29, 2018

gyuho Nov 29, 2018

BenTheElder Nov 30, 2018

BenTheElder Nov 29, 2018

gyuho commented Nov 30, 2018

BenTheElder Nov 30, 2018

krzyzacy Nov 30, 2018

gyuho Nov 30, 2018

BenTheElder left a comment

k8s-ci-robot commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

krzyzacy commented Nov 30, 2018

gyuho commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

kubetest: add "KubectlCommand" to "deployer" interface #10216

kubetest: add "KubectlCommand" to "deployer" interface #10216

Conversation

gyuho commented Nov 22, 2018 • edited Loading

gyuho commented Nov 26, 2018

krzyzacy commented Nov 26, 2018

gyuho commented Nov 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Nov 27, 2018

gyuho commented Nov 27, 2018 • edited Loading

gyuho commented Nov 27, 2018

gyuho commented Nov 28, 2018

krzyzacy commented Nov 28, 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

gyuho commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

krzyzacy commented Nov 30, 2018

gyuho commented Nov 30, 2018

k8s-ci-robot commented Nov 30, 2018

gyuho commented Nov 22, 2018 •

edited

Loading

gyuho commented Nov 27, 2018 •

edited

Loading