Support formatting and mounting GCE PD without 'safe_format_and_mount' #8530

ghost · 2015-05-19T21:09:47Z

The GCE PD plugin uses safe_format_and_mount found on standard GCE images:

https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/google-startup-scripts/usr/share/google/safe_format_and_mount

On custom images where this is not available pods fail to format and mount GCE PDs.

This patch uses linux utilities in a similar way to the safe_format_and_mount
script to format and mount the GCE PD device. That is first attempt a mount.
If mount fails and detects a file system "null" try to use file to investigate
the device. If 'file' fails to get any information about the device and simply
returns "data" then assume the device is not formatted and format it and attempt
to mount it again.

ghost · 2015-05-19T21:24:06Z

@ddysher What do you think ?
@childsb with this PR plus #7847 GCE PD works out of the box on Atomic

ddysher · 2015-05-21T02:50:20Z

pkg/volume/gce_pd/gce_util.go

-		glog.V(5).Infof("error running /usr/share/google/safe_format_and_mount\n%s", string(dataOut))
+		glog.V(5).Infof("error running %s\n%s", safeFormatAndMountPath, string(dataOut))
+		// if safe_format_and_mount is not available on this system use linux utilities directly.
+		if strings.Contains(err.Error(), safeFormatAndMountPath+": no such file or directory") {


This is brittle, at least, you need to lowercase err.Error(). BTW, I think shell use upper case for error message?, i.e. No such file or directory.

Also, why not check file existence first?

I have changed this to check for the existence of the file instead of trying to match the error.

ghost · 2015-05-21T20:10:31Z

@ddysher I have addressed your comments and responded to your questions. What do you think ?

ddysher · 2015-05-26T02:11:25Z

pkg/volume/gce_pd/gce_util.go

+	cmd := mounter.runner.Command("file", args...)
+	dataOut, err := cmd.CombinedOutput()
+
+	if err == nil && strings.TrimSpace(string(dataOut)) == disk+": data" {


https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/google-startup-scripts/usr/share/google/safe_format_and_mount#L90

The bash script uses filesystem, is it a better condition to check?

Yeah that makes more sense. Done.

ddysher · 2015-05-26T02:18:03Z

some minor comments. make sure PD e2d passes.

ghost · 2015-05-26T20:51:52Z

@ddysher I have updated the patch to address your comments. PD e2e tests are passing.

ddysher · 2015-05-27T22:04:49Z

LGTM
but need triage for code freeze

thockin · 2015-05-28T23:24:12Z

I want to have a look at this.

rjnagal · 2015-05-29T16:19:44Z

@thockin removing lgtm while you take a look and decide.

erictune · 2015-06-01T15:30:46Z

@thockin push this to post-1.0?

ghost · 2015-06-01T17:58:32Z

@thockin @erictune it would be great of this can patch can make it into 1.0 as it enables GCE PD on Atomic Host and other custom images uploaded by users to GCE out of the box

thockin · 2015-06-02T04:18:02Z

to the goal of "custom images uploaded by users to GCE" there are a lot of things that you really want to do to in order to properly be a GCE image.

https://cloud.google.com/compute/docs/tutorials/building-images

thockin · 2015-06-02T04:20:15Z

pkg/volume/gce_pd/gce_util.go

 	dataOut, err := cmd.CombinedOutput()
 	if err != nil {
-		glog.V(5).Infof("error running /usr/share/google/safe_format_and_mount\n%s", string(dataOut))
+		glog.V(5).Infof("error running %s\n%s", safeFormatAndMountPath, string(dataOut))
+		// if safe_format_and_mount is not available on this system use linux utilities directly.


Why support both ways? If this is equivalent, then we should drop the external dependency.

thockin · 2015-06-02T04:28:01Z

I am OK with this overall - it's pretty straightforward. Why not get rid of support for the old legacy script and just do it ourselves always?

ghost · 2015-06-04T22:46:41Z

@thockin I made the changes you asked for. Dropping the use of the script made writing the test easier so I expanded the test to better cover the code. I also removed the check for the specific error message as older versions of mount have a different message.
e2e PD tests are passing

thockin · 2015-06-05T03:59:21Z

The chaneg looks good, but I am nervous putting this in at this point. I'd rather this wait until after 1.0 - is that going to derail anything?

LGTM, hold until after 1.0

k8s-bot · 2015-06-10T19:46:50Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

ghost · 2015-06-11T13:50:34Z

pkg/volume/aws_ebs/aws_util.go

-// safe_format_and_mount is a utility script on AWS VMs that probes a persistent disk, and if
-// necessary formats it before mounting it.
-// This eliminates the necessity to format a PD before it is used with a Pod on AWS.
-// TODO: port this script into Go and use it for all Linux platforms


Did not notice this TODO last time :)

ghost · 2015-06-11T13:52:16Z

@thockin So it turns out the AWS EBS plugin was using the safe_format_and_mount script as well so I moved it to the mount package and now it shared between GCE PD and AWS EBS... WDYT ?

markturansky · 2015-07-10T17:30:40Z

@swagiaal we need to make sure these upstream PRs that were patched into OpenShift stay in sync downstream.

ghost · 2015-07-30T20:38:37Z

@thockin Does this still have your LGTM after expansion to AWS EBS ?

ghost · 2015-08-17T14:30:43Z

@thockin could you take another look at this ?

saad-ali · 2015-08-21T23:11:24Z

@swagiaal There have been a ton of changes since this was created, could you rebase?

k8s-bot · 2015-08-24T14:36:04Z

GCE e2e build/test failed for commit f27d5d564c4666ea7764c71e28fc555e6b261852.

k8s-bot · 2015-08-24T15:10:45Z

GCE e2e build/test passed for commit ededecf73a18032bf33b195bfe5ce0d82f43c1c9.

ghost · 2015-08-24T15:13:12Z

@saad-ali rebased... thanks for taking a look!

saad-ali · 2015-08-26T03:18:18Z

pkg/util/mount/mount.go

+	cmd := mounter.Runner.Command("file", args...)
+	dataOut, err := cmd.CombinedOutput()
+
+	// TODO (swagiaal): check if this disk has partitions and return false, and


Please open an issue to track this TODO and update this comment to include to issue #.

saad-ali · 2015-08-26T03:19:16Z

Thanks for the clean up here @swagiaal
PR LGTM mod one minor nit

The GCE PD plugin uses safe_format_and_mount found on standard GCE images: https://github.com/GoogleCloudPlatform/compute-image-packages/blob/master/google-startup-scripts/usr/share/google/safe_format_and_mount On custom images where this is not available pods fail to format and mount GCE PDs. This patch uses linux utilities in a similar way to the safe_format_and_mount script to format and mount the GCE PD and AWS EBC devices. That is first attempt a mount. If mount fails try to use file to investigate the device. If 'file' fails to get any information about the device and simply returns "data" then assume the device is not formatted and format it and attempt to mount it again. Signed-off-by: Sami Wagiaalla <swagiaal@redhat.com>

k8s-bot · 2015-08-26T16:19:46Z

GCE e2e build/test passed for commit ab0258f.

ghost · 2015-08-26T16:55:12Z

@saad-ali thanks for reviewing... comment addressed

saad-ali · 2015-08-26T17:54:05Z

Thanks @swagiaal
LGTM

Support formatting and mounting GCE PD without 'safe_format_and_mount'

googlebot added the cla: yes label May 19, 2015

yujuhong assigned ddysher May 19, 2015

ddysher reviewed May 21, 2015
View reviewed changes

ddysher reviewed May 26, 2015
View reviewed changes

ddysher added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2015

thockin assigned thockin and unassigned ddysher May 28, 2015

rjnagal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2015

erictune added this to the v1.0-post milestone Jun 1, 2015

thockin reviewed Jun 2, 2015
View reviewed changes

markturansky mentioned this pull request Jun 9, 2015

UPSTREAM: GCEPD mounting on Atomic openshift/origin#3012

Merged

ghost reviewed Jun 11, 2015
View reviewed changes

brosner mentioned this pull request Jun 11, 2015

fixed path for safe_format_and_mount in kubelet break coreos on gce #7042

Closed

ghost mentioned this pull request Jun 19, 2015

AWS persistent storage #10017

Closed

ghost mentioned this pull request Jul 10, 2015

AWS EBS Attaches but Pod Remains at Pending #11011

Closed

bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015

ghost mentioned this pull request Aug 20, 2015

GCE Disk based Volumes are broken on CoreOS #11042

Closed

saad-ali reviewed Aug 26, 2015
View reviewed changes

ghost mentioned this pull request Aug 26, 2015

Check for Partitions before formatting in SafeFormatAndMount #13212

Closed

saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2015

yujuhong added a commit that referenced this pull request Aug 26, 2015

Merge pull request #8530 from swagiaal/add-fomat-and-mount

a5fe33b

Support formatting and mounting GCE PD without 'safe_format_and_mount'

yujuhong merged commit a5fe33b into kubernetes:master Aug 26, 2015

iterion mentioned this pull request Oct 27, 2015

PersistentVolume mounting fails on AWS coreos/coreos-kubernetes#130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support formatting and mounting GCE PD without 'safe_format_and_mount' #8530

Support formatting and mounting GCE PD without 'safe_format_and_mount' #8530

ghost commented May 19, 2015

ghost commented May 19, 2015

ddysher May 21, 2015

ghost May 21, 2015

ghost commented May 21, 2015

ddysher May 26, 2015

ghost May 26, 2015

ddysher commented May 26, 2015

ghost commented May 26, 2015

ddysher commented May 27, 2015

thockin commented May 28, 2015

rjnagal commented May 29, 2015

erictune commented Jun 1, 2015

ghost commented Jun 1, 2015

thockin commented Jun 2, 2015

thockin Jun 2, 2015

ghost Jun 4, 2015

thockin commented Jun 2, 2015

ghost commented Jun 4, 2015

thockin commented Jun 5, 2015

k8s-bot commented Jun 10, 2015

ghost Jun 11, 2015

ghost commented Jun 11, 2015

markturansky commented Jul 10, 2015

ghost commented Jul 30, 2015

ghost commented Aug 17, 2015

saad-ali commented Aug 21, 2015

k8s-bot commented Aug 24, 2015

k8s-bot commented Aug 24, 2015

ghost commented Aug 24, 2015

saad-ali Aug 26, 2015

saad-ali commented Aug 26, 2015

k8s-bot commented Aug 26, 2015

ghost commented Aug 26, 2015

saad-ali commented Aug 26, 2015

Support formatting and mounting GCE PD without 'safe_format_and_mount' #8530

Support formatting and mounting GCE PD without 'safe_format_and_mount' #8530

Conversation

ghost commented May 19, 2015

ghost commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher commented May 26, 2015

ghost commented May 26, 2015

ddysher commented May 27, 2015

thockin commented May 28, 2015

rjnagal commented May 29, 2015

erictune commented Jun 1, 2015

ghost commented Jun 1, 2015

thockin commented Jun 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Jun 2, 2015

ghost commented Jun 4, 2015

thockin commented Jun 5, 2015

k8s-bot commented Jun 10, 2015

Choose a reason for hiding this comment

ghost commented Jun 11, 2015

markturansky commented Jul 10, 2015

ghost commented Jul 30, 2015

ghost commented Aug 17, 2015

saad-ali commented Aug 21, 2015

k8s-bot commented Aug 24, 2015

k8s-bot commented Aug 24, 2015

ghost commented Aug 24, 2015

Choose a reason for hiding this comment

saad-ali commented Aug 26, 2015

k8s-bot commented Aug 26, 2015

ghost commented Aug 26, 2015

saad-ali commented Aug 26, 2015