Implement CanMount() for gfsMounter for linux #36686

rkouj · 2016-11-12T01:36:32Z

What this PR does / why we need it:
To implement CanMount() check for glusterfs. If mount binaries are not present on the underlying node, the mount will not proceed and return an error message stating so.

Related to issue : #36098

Related to similar change for NFS :
#36280

Release note:
Check binaries for GlusterFS on the underlying node before doing mount

This change is

Sample output from testing in GCE/GCI:

rkouj@rkouj0:~/go/src/k8s.io/kubernetes$ kubectl describe pods
Name: glusterfs
Namespace: default
Node: e2e-test-rkouj-minion-group-kjq3/10.240.0.3
Start Time: Fri, 11 Nov 2016 17:22:04 -0800
Labels:
Status: Pending
IP:
Controllers:
Containers:
glusterfs:
Container ID:
Image: gcr.io/google_containers/busybox
Image ID:
Port:
QoS Tier:
cpu: Burstable
memory: BestEffort
Requests:
cpu: 100m
State: Waiting
Reason: ContainerCreating
Ready: False
Restart Count: 0
Environment Variables:
Conditions:
Type Status
Initialized True
Ready False
PodScheduled True
Volumes:
glusterfs:
Type: Glusterfs (a Glusterfs mount on the host that shares a pod's lifetime)
EndpointsName: glusterfs-cluster
Path: kube_vol
ReadOnly: true
default-token-2zcao:
Type: Secret (a volume populated by a Secret)
SecretName: default-token-2zcao
Events:
FirstSeen LastSeen Count From SubobjectPath Type Reason Message

8s 8s 1 {default-scheduler } Normal Scheduled Successfully assigned glusterfs to e2e-test-rkouj-minion-group-kjq3
7s 4s 4 {kubelet e2e-test-rkouj-minion-group-kjq3} Warning FailedMount Unable to mount volume kubernetes.io/glusterfs/6bb04587-a876-11e6-a712-42010af00002-glusterfs (spec.Name: glusterfs) on pod glusterfs (UID: 6bb04587-a876-11e6-a712-42010af00002). Verify that your node machine has the required components before attempting to mount this volume type. Required binary /sbin/mount.glusterfs is missing

rkouj · 2016-11-12T02:25:27Z

@saad-ali

rootfs · 2016-11-12T17:09:49Z

does it work for ns mount?

rkouj · 2016-11-12T21:20:07Z

Edit : I misread it as nfs mount. The following comment is for nfs mount

Yes we have enabled the check for only GCI and only when the containerized mount flag is turned off.
I have linked the PR in description with sample output.

cc: @saad-ali

humblec · 2016-11-13T05:23:32Z

pkg/volume/glusterfs/glusterfs.go

+	exe := exec.New()
+	switch runtime.GOOS {
+	case "linux":
+		if _, err := exe.Command("/bin/ls", "/sbin/mount.glusterfs").CombinedOutput(); err != nil {


Please define the mount client binary path as constant. It would also be good if you can add a comment about what distros are verified under linux .

saad-ali · 2016-11-14T01:05:28Z

does it work for ns mount?

@rootfs Can you clarify what you mean by this?

rootfs · 2016-11-14T14:03:23Z

@rkouj @saad-ali In ns_mount.go, the mount binary is under hostRootFsPath, rather than /sbin.

rkouj · 2016-11-14T20:14:44Z

@rootfs : For now the change is enabled only in GCI where we don't run kubelet in container.

Once we start doing that or if we need to make the change take effect elsewhere, we need to add additional code to support it.

rootfs · 2016-11-14T20:43:25Z

@rkouj Unfortunately, there are use cases to run kubelet in containers.

I think a clean way to do this is to have a mounter interface to detect mount binaries. @saad-ali

rkouj · 2016-11-14T20:58:36Z

As discussed on slack with @rootfs
the CanMount() is part of the mounter inteface which means we can detect where the mounter runs.

rootfs · 2016-11-14T21:18:56Z

@rkouj checkout util mounter and ns mounter

rkouj · 2016-11-14T22:01:04Z

@rootfs : The CanMount() check that we are doing is specific to each volume plugin and the intent of this change was to make CanMount() isolated to each plugin.

Later on we can refactor this when we want this change to turned on on other platforms and make it more generic.

saad-ali · 2016-11-14T22:05:20Z

LGTM

k8s-ci-robot · 2016-11-14T22:29:29Z

@rkouj: you can't LGTM your own PR.

In response to [this comment](https://github.com//pull/36686#issuecomment-260484357):

/lgtm

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

rkouj · 2016-11-14T22:31:03Z

Post applying the lgtm label, the Submit Queue is not getting updated to pick up the change.

saad-ali · 2016-11-14T22:32:47Z

/lgtm

k8s-github-robot · 2016-11-14T23:04:21Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-14T23:40:45Z

Automatic merge from submit-queue

eparis · 2016-11-17T15:58:47Z

Can someone @saad-ali @rootfs help me understand. Does this break containerized kubelet and gluster? If so, it seems like a regression that should not have been merged...

rootfs · 2016-11-17T16:12:07Z

The code checks /sbin/mount.glusterfs for linux (btw, gluster client also runs on Mac), while containerized kubelet actually executes /rootfs/sbin/mount.glusterfs, there is a mismatch: if containerized kubelet doesn't have /sbin/mount.glusterfs, while host has, then we get a false negative

eparis · 2016-11-17T17:31:28Z

@saad-ali I'm removing the cherrypick-candidate label. This is a regression breaking working setups. Personally I think we should revert until it and look for a non-regressing path, but I'd love to hear your take.

rkouj · 2016-11-17T19:05:37Z

@eparis I'm not sure if this helps but this check is enabled only when we don't do a containerized mount.

Right now the flag is enabled only in GCI (where we don't do containerized kubelet yet I believe). https://github.com/kubernetes/kubernetes/pull/36280/files#diff-6e3b476b9225d1213dc6ad13e453fc16R477

If we are doing a containerized mount, this check will not run. https://github.com/kubernetes/kubernetes/pull/36280/files#diff-bf28da68f62a8df6e99e447c4351122dR719

Once we start exploring more avenues, we will need to refactor/add more code to handle different cases.

rootfs · 2016-11-17T19:24:42Z

@rkouj As I understand it, experimental-check-node-capabilities-before-mount is a user visible flag. Although it is currently only set on gci image, there is nothing preventing other kubernetes distribution to set it, and there is no words around or doc update to alert the potential issue for containerized kubelet.

rkouj · 2016-11-17T19:36:24Z

Right. What do you suggest ? I can create an issue and link it if it helps. Or add a comment around the flag in options.go .

rootfs · 2016-11-17T19:38:53Z

@eparis what's the best practice?

saad-ali · 2016-11-22T02:25:50Z

Sorry for the delay in response--overwhelmed with CCs so I just saw this thread.

Can someone @saad-ali @rootfs help me understand. Does this break containerized kubelet and gluster? If so, it seems like a regression that should not have been merged...

@eparis No, it does not (or at least should not). The CanMount() check is only enabled if you are deploying a GCI OS image on GCE (see https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/gci/configure-helper.sh#L486). It is disabled by default in all other cases. This PR was a follow up to #36280 which has the bulk of the logic. On GCE/GKE the new default OS image, GCI, did not have the correct binaries for NFS and GlusterFS and those mounts were failing for users with cryptic mount errors. This was a way for us to at least give users better error message (while we work on adding support back). While the check is designed to be extensible per plugin, it is not, as @rootfs mentioned, extensible based on underlying platform. I like the suggestion of extending the mounter interface so that the check is custom per platform, but that also gets messy since the check has to also be custom per plugin. We will iterate on the current design in the future and refactor (we plan to invest heavily in this area of better user messaging). But rest assured that this should not affect any platforms other than GCE on GCI.

@saad-ali I'm removing the cherrypick-candidate label. This is a regression breaking working setups. Personally I think we should revert until it and look for a non-regressing path, but I'd love to hear your take.

It should not have broken anything. If it did, our bad, please file a bug and we'll take a look ASAP. Any break here would be a release blocker.

@rkouj As I understand it, experimental-check-node-capabilities-before-mount is a user visible flag. Although it is currently only set on gci image, there is nothing preventing other kubernetes distribution to set it, and there is no words around or doc update to alert the potential issue for containerized kubelet.

The flag is explicitly marked experimental because it is not guaranteed to work on all platforms. It is also explicitly disabled by default for this reason.

We need this in GKE for 1.4, I'm going to re-add the cherrypick candidate label. If you have any other concerns, I'll be happy to discuss them. Feel free to ping me here, or directly on Slack, and I'm happy to discuss this during the Storage SIG meeting as well.

…upstream-release-1.4 Automated cherry pick of #36686

k8s-cherrypick-bot · 2016-12-07T22:54:36Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

googlebot added the cla: yes label Nov 12, 2016

k8s-github-robot assigned rootfs Nov 12, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Nov 12, 2016

humblec reviewed Nov 13, 2016

View reviewed changes

Implement CanMount() for gfsMounter for linux

b85ac95

rkouj force-pushed the check-gluster-fs-binaries branch from 06733df to b85ac95 Compare November 14, 2016 20:18

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 14, 2016

saad-ali assigned saad-ali and rootfs and unassigned rootfs Nov 14, 2016

saad-ali added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 14, 2016

saad-ali added this to the v1.4 milestone Nov 14, 2016

saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2016

k8s-github-robot merged commit 9be1191 into kubernetes:master Nov 14, 2016

saad-ali added the cherrypick-candidate label Nov 15, 2016

eparis removed the cherrypick-candidate label Nov 17, 2016

saad-ali added the cherrypick-candidate label Nov 22, 2016

saad-ali mentioned this pull request Nov 22, 2016

Better messaging for missing volume binaries on host #36280

Merged

jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 1, 2016

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

rkouj mentioned this pull request Dec 7, 2016

Automated cherry pick of #36686 #38326

Merged

jessfraz added a commit that referenced this pull request Dec 7, 2016

Merge pull request #38326 from rkouj/automated-cherry-pick-of-#36686-…

07ef1f4

…upstream-release-1.4 Automated cherry pick of #36686

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CanMount() for gfsMounter for linux #36686

Implement CanMount() for gfsMounter for linux #36686

rkouj commented Nov 12, 2016 •

edited

Loading

rkouj commented Nov 12, 2016

rootfs commented Nov 12, 2016

rkouj commented Nov 12, 2016 •

edited

Loading

humblec Nov 13, 2016

rkouj Nov 14, 2016

saad-ali commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

saad-ali commented Nov 14, 2016

k8s-ci-robot commented Nov 14, 2016

rkouj commented Nov 14, 2016

saad-ali commented Nov 14, 2016

k8s-github-robot commented Nov 14, 2016

k8s-github-robot commented Nov 14, 2016

eparis commented Nov 17, 2016

rootfs commented Nov 17, 2016

eparis commented Nov 17, 2016

rkouj commented Nov 17, 2016 •

edited

Loading

rootfs commented Nov 17, 2016

rkouj commented Nov 17, 2016

rootfs commented Nov 17, 2016

saad-ali commented Nov 22, 2016

k8s-cherrypick-bot commented Dec 7, 2016

Implement CanMount() for gfsMounter for linux #36686

Implement CanMount() for gfsMounter for linux #36686

Conversation

rkouj commented Nov 12, 2016 • edited Loading

rkouj commented Nov 12, 2016

rootfs commented Nov 12, 2016

rkouj commented Nov 12, 2016 • edited Loading

humblec Nov 13, 2016

Choose a reason for hiding this comment

rkouj Nov 14, 2016

Choose a reason for hiding this comment

saad-ali commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

rootfs commented Nov 14, 2016

rkouj commented Nov 14, 2016

saad-ali commented Nov 14, 2016

k8s-ci-robot commented Nov 14, 2016

rkouj commented Nov 14, 2016

saad-ali commented Nov 14, 2016

k8s-github-robot commented Nov 14, 2016

k8s-github-robot commented Nov 14, 2016

eparis commented Nov 17, 2016

rootfs commented Nov 17, 2016

eparis commented Nov 17, 2016

rkouj commented Nov 17, 2016 • edited Loading

rootfs commented Nov 17, 2016

rkouj commented Nov 17, 2016

rootfs commented Nov 17, 2016

saad-ali commented Nov 22, 2016

k8s-cherrypick-bot commented Dec 7, 2016

rkouj commented Nov 12, 2016 •

edited

Loading

rkouj commented Nov 12, 2016 •

edited

Loading

rkouj commented Nov 17, 2016 •

edited

Loading