Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make various fixes to flex tests and fix some crashes #65536

Merged
merged 1 commit into from Jul 2, 2018

Conversation

@gnufied
Copy link
Member

gnufied commented Jun 27, 2018

  • Fixes two controller-manager crashes when a flex plugin gets removed from flex directory.
  • Also enables e2e tests to run in local clusters and other environments.
  • Removes disruptive from flex e2e tests because flex can be installed in a running cluster and does not require kubelet or controller-manager restart anymore.

/sig storage

cc @verult @jsafrane

Fix controller-manager crashes when flex plugin is removed from flex plugin directory
@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jun 27, 2018

/assign @verult @jsafrane

@gnufied gnufied force-pushed the gnufied:fix-flex-crashing-controller-manager branch 2 times, most recently from f4199ef to 5caa979 Jun 27, 2018

@@ -211,7 +242,7 @@ var _ = utils.SIGDescribe("Flexvolumes [Disruptive]", func() {
driverInstallAs := driver + "-" + suffix

This comment has been minimized.

@verult

verult Jun 27, 2018

Contributor

We don't need the test "should install plugin without kubelet restart" anymore, it's exactly the same as this test

This comment has been minimized.

@gnufied

gnufied Jun 28, 2018

Author Member

fixed

@@ -229,9 +260,9 @@ var _ = utils.SIGDescribe("Flexvolumes [Disruptive]", func() {
driverInstallAs := driver + "-" + suffix

By(fmt.Sprintf("installing flexvolume %s on node %s as %s", path.Join(driverDir, driver), node.Name, driverInstallAs))
installFlex(cs, &node, "k8s", driverInstallAs, path.Join(driverDir, driver), true /* restart */)
installFlex(cs, &node, "k8s", driverInstallAs, path.Join(driverDir, driver), false /* restart */)

This comment has been minimized.

@verult

verult Jun 27, 2018

Contributor

Could you also remove the restart param altogether and remove the restart == true branch in installFlex()?

@@ -605,7 +605,8 @@ func (pm *VolumePluginMgr) refreshProbedPlugins() {
}
pm.probedPlugins[event.Plugin.GetPluginName()] = event.Plugin
} else if event.Op == ProbeRemove {
delete(pm.probedPlugins, event.Plugin.GetPluginName())
// Plugin is not available on ProbeRemove event, only PluginName
delete(pm.probedPlugins, event.PluginName)

This comment has been minimized.

@verult

verult Jun 27, 2018

Contributor

Thanks for the catch!

@gnufied gnufied force-pushed the gnufied:fix-flex-crashing-controller-manager branch from 5caa979 to 4f2c37f Jun 28, 2018

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jun 28, 2018

cc @saad-ali for approving other general changes done to test/ .

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jun 28, 2018

@verult fixed all your comments can you PTAL?

@wongma7

This comment has been minimized.

Copy link
Contributor

wongma7 commented Jun 28, 2018

lgtm, we no longer need restarts with the prober proven working and i am looking forward to being able to run these locally!!

@verult
Copy link
Contributor

verult left a comment

Just one small comments now

host = net.JoinHostPort(hostName, sshPort)
}

if host == "" {

This comment has been minimized.

@verult

verult Jun 28, 2018

Contributor

Do we need this check?

This comment has been minimized.

@gnufied

gnufied Jun 28, 2018

Author Member

I think yes for printing proper error when we can't determine external or internal ip for some reason. Before this change GetNodeExternalIP used to fail on it.

This comment has been minimized.

@verult

verult Jun 28, 2018

Contributor

We don't need to check if host == "" though, since having an error is equivalent to host == ""

This comment has been minimized.

@gnufied

gnufied Jun 28, 2018

Author Member

You mean having err set or having a failure in general? Are you saying - we should rename this to be if err != nil ? I think that will work too.

Removing this check altogether means - sshAndLog will fail, but it may not be super obvious why it failed. Having explicit printing of error here means - we will not be calling sshAndLog if host is not found. Is there downside I am not seeing?

This comment has been minimized.

@verult

verult Jun 29, 2018

Contributor

Sorry I wasn't clear. I meant that

if host == "" {
    	framework.ExpectNoError(err)
}

is equivalent to only

framework.ExpectNoError(err)

because there's an error only if host == "" in the implementations of GetNode*ternalIP()

This comment has been minimized.

@gnufied

gnufied Jun 29, 2018

Author Member

fixed

@gnufied gnufied force-pushed the gnufied:fix-flex-crashing-controller-manager branch from 4f2c37f to 4e7c2f6 Jun 29, 2018

@gnufied

This comment has been minimized.

Copy link
Member Author

gnufied commented Jun 29, 2018

/test pull-kubernetes-e2e-gce

@verult

This comment has been minimized.

Copy link
Contributor

verult commented Jun 30, 2018

/lgtm

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 30, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 2, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, gnufied, verult

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:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 2, 2018

Automatic merge from submit-queue (batch tested with PRs 65299, 65524, 65154, 65329, 65536). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 92b8111 into kubernetes:master Jul 2, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.