Add volume reconstruct/cleanup logic in kubelet volume manager #12024

jsafrane · 2016-11-24T18:49:44Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1398417

There are two UPSTREAM fixes here, both are needed to fix the bug. Beware, the first one is HUGE :-(.

…me manager

…nformation if global mount path is gone

ncdc · 2016-11-24T19:56:42Z

@sttts @ingvagabund would you have time to review?

ingvagabund · 2016-11-24T21:15:15Z

This is basically backport from kubernetes, so it should be more like a formal review, right?

ingvagabund · 2016-11-24T21:17:11Z

Any chance to run more tests than the travis one? Hard to believe it is the only CI that is required to run.

stevekuznetsov · 2016-11-24T22:38:33Z

[test]

mfojtik · 2016-11-25T15:12:17Z

you forgot to run hack/update-generated-clientsets.sh

gnufied · 2016-11-25T15:14:46Z

Bunch of failures here appears to be because the test harness in jenkins doesn't work well for old builds anymore, such as:

docker version is upgraded to 1.12. Any code that use API/option present in only 1.10 fails (I am looking at router tests)
./test/extended/networking-minimal.sh does not exist on 1.3
conformonce test is flaking on Extended.deploymentconfigs with failing hook [Conformance] should get all logs from retried hooks #11630

stevekuznetsov · 2016-11-25T15:16:52Z

We will need to set up a different job to handle non-master PR tests for Origin. The setup exists for OSE today, but we have not replicated it here. Any chance we can open this PR against ose/enterprise-3.3 ?

gnufied · 2016-11-25T15:23:18Z

@mfojtik I ran that. It doesn't produce any changes to generated code (or any commitable changes). Also locally when I ran make verify on this PR - it did not throw errors I see here. Not sure

gnufied · 2016-11-25T16:34:33Z

@stevekuznetsov @mfojtik the PR for ose - https://github.com/openshift/ose/pull/480

jsafrane · 2016-11-28T11:03:41Z

@gnufied, thanks for taking care of this.

Indeed, make verify fails on PRs to release-1.3 branch, IMO there is nothing I can do about it. Exactly the same patch is in https://github.com/openshift/ose/pull/480 and the bot there does not complain.

rootfs · 2016-11-28T15:54:10Z

vendor/k8s.io/kubernetes/pkg/volume/vsphere_volume/vsphere_volume.go

@@ -135,6 +135,18 @@ func (plugin *vsphereVolumePlugin) getCloudProvider() (*vsphere.VSphere, error)
 	return vs, nil
 }

+func (plugin *vsphereVolumePlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.Spec, error) {


should we pick up the latest fix
kubernetes/kubernetes#37167

Lets keep that to a separate PR.

kubernetes/kubernetes#37167

that's for photon volume plugin, it's not present in OpenShift 1.3

childsb · 2016-11-28T16:03:35Z

Both changes are merged upstream (there's some funk in the links & description):

kubernetes/kubernetes#36840

kubernetes/kubernetes#27970

lgtm for merge as soon as tests complete.

stevekuznetsov · 2016-11-28T16:08:49Z

@smarterclayton is this something we want to be updating the Origin 1.3 branch with? @jsafrane as I said before, our test infra is not set up to be able to run this test correctly. You won't see it pass here. This is why the OSE PR is created.

rootfs · 2016-11-28T16:13:21Z

vendor/k8s.io/kubernetes/pkg/util/mount/mount.go

@@ -37,6 +41,9 @@ type Interface interface {
 	// IsLikelyNotMountPoint determines if a directory is a mountpoint.
 	// It should return ErrNotExist when the directory does not exist.
 	IsLikelyNotMountPoint(file string) (bool, error)
+	// GetDeviceNameFromMount finds the device name by checking the mount path
+	// to get the global mount path which matches its plugin directory
+	GetDeviceNameFromMount(mountPath, pluginDir string) (string, error)


also needs to pick up kubernetes/kubernetes#30724

The PR won't hurt , but do we support OpenShift on non-Linux platforms?

Origin is required to compile on all Linux platforms that have a build target (32, 64, arm64, and s390 eventually).

Breaking the openshift dev experience on mac will earn a swift revert :)

Basically must compile on all platforms, must not knowingly break kubelet on all Linux platforms.

fixed, picked kubernetes/kubernetes#30724
thanks for spotting that.

smarterclayton · 2016-11-28T16:36:45Z

Was this back ported to 1.3? We've had another request to get at fixes in kube 1.3.x - it may be easier to simply merge those fixes via an update.

gnufied · 2016-11-28T17:03:07Z

kubernetes/kubernetes#27970 was backported to kubernetes-1.3 release branch. kubernetes/kubernetes#36840 is too new and hasn't been backported.

gnufied · 2016-11-28T17:34:47Z

vendor/k8s.io/kubernetes/pkg/kubelet/kubelet.go

 			glog.V(3).Infof("Orphaned pod %q found, but volumes are not cleaned up; err: %v", uid, err)
 			continue
 		}
+		// Check whether volume is still mounted on disk. If so, do not delete directory
+		if volumeNames, err := kl.getPodVolumeNameListFromDisk(uid); err != nil || len(volumeNames) != 0 {


@eparis @jsafrane so I reviewed this PR in some detail and this is the bit - which helps with volumes not getting deleted if it is still mounted on disk.

The other big change here is in reconciler.go:

Refactor reconcile() function in its own function. There isn't code a code change in whole of reconcilation itself but diff looks pretty big because of refactoring.

After reconcillation - reconciler also periodically calls reconstruct - which populates state of world from pod by iterating through each pod volume plugin directory.

Overall, I think we should be okay.

childsb · 2016-11-30T15:01:53Z

this is critical bug. both changes were merged upstream already and this is just a pick of those changes. i reviewed and this lgtm.

smarterclayton · 2016-11-30T16:29:39Z

Removing merge, this does not compile on macs.

smarterclayton · 2016-11-30T16:30:00Z

You need to fix

○ make
hack/build-go.sh
++ Building go targets for darwin/amd64: cmd/openshift cmd/oc
# github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/mount
/Users/clayton/projects/origin/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/util/mount/mount.go:88: cannot use Mounter literal (type *Mounter) as type Interface in return argument:
	*Mounter does not implement Interface (missing GetDeviceNameFromMount method)

in this PR before merge.

jsafrane · 2016-11-30T16:57:37Z

@smarterclayton, it should be compilable now, I do not have mac though.

jsafrane · 2016-11-30T17:02:32Z

[test]

openshift-bot · 2016-11-30T17:06:14Z

Evaluated for origin test up to feae665

childsb · 2016-11-30T17:41:19Z

I verified that this builds on OSX.

childsb · 2016-11-30T17:42:09Z

[merge]

openshift-bot · 2016-11-30T18:21:40Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11823/) (Base Commit: d200e8a)

openshift-bot · 2016-12-01T06:05:19Z

Evaluated for origin merge up to feae665

openshift-bot · 2016-12-01T07:29:22Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11847/) (Base Commit: d200e8a)

jsafrane added 2 commits November 24, 2016 19:45

UPSTREAM: 27970: Add volume reconstruct/cleanup logic in kubelet volu…

20ff233

…me manager

UPSTREAM: 36840: Kubelet volume manager fails to reconstruct volume i…

d52c488

…nformation if global mount path is gone

ingvagabund added the component/kubernetes label Nov 24, 2016

jsafrane force-pushed the aws-detach-fixes branch from a4a8af2 to d52c488 Compare November 28, 2016 15:45

rootfs reviewed Nov 28, 2016

View reviewed changes

gnufied reviewed Nov 28, 2016

View reviewed changes

UPSTREAM: 30724: Add GetDeviceNameFromMount in mount_unsupported.go

feae665

eparis merged commit 0955c1d into openshift:release-1.3 Dec 1, 2016

jsafrane mentioned this pull request Feb 21, 2017

persistent storage volumes data is deleted when node becomes unresponsive #13003

Closed

Add volume reconstruct/cleanup logic in kubelet volume manager #12024

Add volume reconstruct/cleanup logic in kubelet volume manager #12024

Conversation

jsafrane commented Nov 24, 2016

ncdc commented Nov 24, 2016

ingvagabund commented Nov 24, 2016

ingvagabund commented Nov 24, 2016

stevekuznetsov commented Nov 24, 2016

mfojtik commented Nov 25, 2016 • edited by stevekuznetsov Loading

gnufied commented Nov 25, 2016

stevekuznetsov commented Nov 25, 2016 • edited Loading

gnufied commented Nov 25, 2016

gnufied commented Nov 25, 2016

jsafrane commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

childsb commented Nov 28, 2016

stevekuznetsov commented Nov 28, 2016

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

smarterclayton commented Nov 28, 2016

gnufied commented Nov 28, 2016

gnufied Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

childsb commented Nov 30, 2016

smarterclayton commented Nov 30, 2016

smarterclayton commented Nov 30, 2016

jsafrane commented Nov 30, 2016

jsafrane commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

childsb commented Nov 30, 2016

childsb commented Nov 30, 2016

openshift-bot commented Nov 30, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

mfojtik commented Nov 25, 2016 •

edited by stevekuznetsov

Loading

stevekuznetsov commented Nov 25, 2016 •

edited

Loading

gnufied Nov 28, 2016 •

edited

Loading