Add kubectlPath flag to e2e_node.test #82308

zhlhahaha · 2019-09-04T03:24:33Z

What this PR does / why we need it:
e2e_node.test does not set default kubectlPath, which lead to test
errors as following:
[Fail] [sig-storage] EmptyDir volumes [It] pod should support
shared volumes between containers [Conformance]

When the test trying to read file in shared volume, it uses
"kubeclt exec namespace -c container_name -- cat file_name".
However, as variable framework.TestContext.KubectlPath not set,
kubectl binary can not be found in the test and the tast fails.

This patch set default value for framework.TestContext.KubectlPath,
which is same to e2e.test. User can also use --kubectl-path flag to
set kubectl path.

Which issue(s) this PR fixes:
Fixes #82418

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:
None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
None

k8s-ci-robot · 2019-09-04T03:24:34Z

@zhlhahaha: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 · 2019-09-04T03:24:40Z

Hi @zhlhahaha. Thanks for your PR.

I'm waiting for a 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.

lubinsz · 2019-09-04T03:25:14Z

/ok-to-test

zhlhahaha · 2019-09-04T05:37:08Z

/assign @tallclair

RainbowMango · 2019-09-06T12:22:35Z

/kind failing-test

tallclair · 2019-09-11T22:39:43Z

test/e2e_node/e2e_node_suite_test.go

@@ -61,6 +61,7 @@ var runServicesMode = flag.Bool("run-services-mode", false, "If true, only run s
 var runKubeletMode = flag.Bool("run-kubelet-mode", false, "If true, only start kubelet, and not run test.")
 var systemValidateMode = flag.Bool("system-validate-mode", false, "If true, only run system validation in current process, and not run test.")
 var systemSpecFile = flag.String("system-spec-file", "", "The name of the system spec file that will be used for node conformance test. If it's unspecified or empty, the default system spec (system.DefaultSysSpec) will be used.")
+var kubectlPath = flag.String("kubectl-path", "kubectl", "The kubectl binary to use. For development, you might use 'cluster/kubectl.sh' here.")


Rather than adding the flag here, move the cluster flag to common flags:

kubernetes/test/e2e/framework/test_context.go

Line 311 in 0c9288e

flags.StringVar(&TestContext.KubectlPath, "kubectl-path", "kubectl", "The kubectl binary to use. For development, you might use 'cluster/kubectl.sh' here.")

zhlhahaha · 2019-09-16T01:15:08Z

/retest

zhlhahaha · 2019-09-16T02:29:59Z

/retest

zhlhahaha · 2019-09-19T10:23:52Z

/retest

zhlhahaha · 2019-10-11T02:20:01Z

/retest

zhlhahaha · 2019-10-12T03:24:45Z

/retest

tallclair · 2019-10-16T23:33:27Z

test/e2e_node/e2e_node_suite_test.go

@@ -66,6 +66,7 @@ func init() {
 	e2econfig.CopyFlags(e2econfig.Flags, flag.CommandLine)
 	framework.RegisterCommonFlags(flag.CommandLine)
 	framework.RegisterNodeFlags(flag.CommandLine)
+	framework.RegisterClusterFlags(flag.CommandLine)


I think you misunderstood my previous comment. ClusterFlags are not relevant to the node e2e suite, so they shouldn't be registered here. The flags that are common to both suites (node & cluster) are in RegisterCommonFlags. My suggestion was to move the specific flag you need from RegisterClusterFlags to RegisterCommonFlags

Thanks, very clear explanation. Updated

e2e_node.test does not set default kubectlPath, which lead to test errors as following: [Fail] [sig-storage] EmptyDir volumes [It] pod should support shared volumes between containers [Conformance] When the test trying to read file in shared volume, it uses "kubeclt exec namespace -c container_name -- cat file_name". However, as variable framework.TestContext.KubectlPath not set, kubectl binary can not be found in the test and the tast fails. This patch move kubectlPath flag from RegisterClusterFlags to RegisterCommonFlags, thus default value for framework.TestContext.KubectlPath will be set,and user can also use --kubectl-path flag to set kubectl path. Signed-off-by: Howard Zhang <howard.zhang@arm.com>

zhlhahaha · 2019-10-17T03:44:05Z

/assign @fejta @tallclair

fejta

/lgtm
/approve

k8s-ci-robot · 2019-10-17T20:38:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, zhlhahaha

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:

~~test/e2e/framework/OWNERS~~ [fejta]

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

oomichi · 2019-10-18T20:32:34Z

/release-note-none

fejta-bot · 2019-10-19T03:40:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 4, 2019

k8s-ci-robot requested review from feiskyer and vishh September 4, 2019 03:26

k8s-ci-robot assigned tallclair Sep 4, 2019

k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 6, 2019

tallclair reviewed Sep 11, 2019

View reviewed changes

zhlhahaha force-pushed the kubectl_flag branch from 5369759 to 203a5a0 Compare September 12, 2019 09:02

zhlhahaha requested a review from tallclair September 16, 2019 11:32

tallclair reviewed Oct 16, 2019

View reviewed changes

zhlhahaha force-pushed the kubectl_flag branch from 203a5a0 to 1c9da19 Compare October 17, 2019 03:32

k8s-ci-robot added the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Oct 17, 2019

k8s-ci-robot assigned fejta Oct 17, 2019

fejta reviewed Oct 17, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2019

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 18, 2019

k8s-ci-robot merged commit 0f7873a into kubernetes:master Oct 19, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 19, 2019

zhlhahaha mentioned this pull request Mar 25, 2020

REQUEST: New membership for zhlhahaha kubernetes/org#1742

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kubectlPath flag to e2e_node.test #82308

Add kubectlPath flag to e2e_node.test #82308

zhlhahaha commented Sep 4, 2019 •

edited

k8s-ci-robot commented Sep 4, 2019

k8s-ci-robot commented Sep 4, 2019

lubinsz commented Sep 4, 2019

zhlhahaha commented Sep 4, 2019

RainbowMango commented Sep 6, 2019

tallclair Sep 11, 2019

zhlhahaha Sep 12, 2019

zhlhahaha commented Sep 16, 2019

zhlhahaha commented Sep 16, 2019

zhlhahaha commented Sep 19, 2019

zhlhahaha commented Oct 11, 2019

zhlhahaha commented Oct 12, 2019

tallclair Oct 16, 2019

zhlhahaha Oct 17, 2019

zhlhahaha commented Oct 17, 2019

fejta left a comment

k8s-ci-robot commented Oct 17, 2019

oomichi commented Oct 18, 2019

fejta-bot commented Oct 19, 2019

Add kubectlPath flag to e2e_node.test #82308

Add kubectlPath flag to e2e_node.test #82308

Conversation

zhlhahaha commented Sep 4, 2019 • edited

k8s-ci-robot commented Sep 4, 2019

k8s-ci-robot commented Sep 4, 2019

lubinsz commented Sep 4, 2019

zhlhahaha commented Sep 4, 2019

RainbowMango commented Sep 6, 2019

tallclair Sep 11, 2019

Choose a reason for hiding this comment

zhlhahaha Sep 12, 2019

Choose a reason for hiding this comment

zhlhahaha commented Sep 16, 2019

zhlhahaha commented Sep 16, 2019

zhlhahaha commented Sep 19, 2019

zhlhahaha commented Oct 11, 2019

zhlhahaha commented Oct 12, 2019

tallclair Oct 16, 2019

Choose a reason for hiding this comment

zhlhahaha Oct 17, 2019

Choose a reason for hiding this comment

zhlhahaha commented Oct 17, 2019

fejta left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 17, 2019

oomichi commented Oct 18, 2019

fejta-bot commented Oct 19, 2019

zhlhahaha commented Sep 4, 2019 •

edited