Merge ~goneri/cloud-init:freebsd_mount_sync into cloud-init:master

Proposed by Gonéri Le Bouder on 2019-03-23
Status: Merged
Approved by: Ryan Harper on 2019-04-18
Approved revision: 7d413b2bee16eb8b486175dd4d09a759e12c37bf
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~goneri/cloud-init:freebsd_mount_sync
Merge into: cloud-init:master
Diff against target: 68 lines (+5/-19)
3 files modified
cloudinit/sources/DataSourceAzure.py (+1/-1)
cloudinit/sources/DataSourceConfigDrive.py (+2/-5)
cloudinit/util.py (+2/-13)
Reviewer Review Type Date Requested Status
Ryan Harper Approve on 2019-04-18
Server Team CI bot continuous-integration Approve on 2019-04-11
Dan Watkins 2019-03-23 Approve on 2019-04-11
Review via email: mp+364997@code.launchpad.net

Commit message

mount_cb: do not pass sync and rw options to mount

On FreeBSD, mount_cd9660 does not accept the sync option that is enabled
by default. In addition, the sync is only useful with the `rw` mode.
However the `rw` mode was never used.

This patch removes the `rw` and `sync` parameter of `mount_cb` to
simplify the code base and resolve the FreeBSD issue.

LP: #1645824

To post a comment you must log in.
Dan Watkins (daniel-thewatkins) wrote :

Hi Gonéri,

Thanks for the merge proposal! I've pasted in our standard template regarding the CLA below, which you'll need to sign before we can merge this.

(Until that's happened, I'm going to mark this as Needs Information.)

Thanks!

--

Thank you for contributing to cloud-init.

To contribute, you must sign the Canonical Contributor License Agreement
(CLA) [1].

If you have already signed it as an individual, your Launchpad user will
be listed in the contributor-agreement-canonical launchpad group [2].
Unfortunately there is no easy way to check if an organization or company
you are doing work for has signed. If you are unsure or have questions,
email <email address hidden> or ping powersj in #cloud-init channel
via freenode.

For information on how to sign, please see the HACKING document [3].

Thanks again, and please feel free to reach out with any questions.


[1] http://www.canonical.com/contributors
[2] https://launchpad.net/~contributor-agreement-canonical/+members
[3] http://cloudinit.readthedocs.io/en/latest/topics/hacking.html

review: Needs Information
Dan Watkins (daniel-thewatkins) wrote :

Hey Gonéri, I just noticed you said you'd signed the agreement in your comment on the bug; I'll chase up internally why the group hasn't been updated.

Dan Watkins (daniel-thewatkins) wrote :

Hey Gonéri, given the above, I'm going to review this MP.

This change appears to change the default for sync from enabled to disabled for all filesystems on all non-Linux platforms. Given that the bug only reports issues with a single filesystem on a single platform, that seems like a broad change. Could we look at scoping down this change to more specifically address the issue at hand?

(Looking at https://www.freebsd.org/cgi/man.cgi?mount(8), I don't think this change is correct even just for FreeBSD; evidently some filesystems do support sync on FreeBSD.)

Gonéri Le Bouder (goneri) wrote :

I'm actually not sure why we need the 'sync' option since the volume is supposed to be in RO mode. This is the reason why the uses of sync sounds more like a Linux specific work around. Since there is no real isolation, FreeBSD ends up with the parameter too and we have to ignore it. This is the reason why I took this approach: I enable it only on Linux, since it "may" be relevant there, I disable it everywhere else.

Dan Watkins (daniel-thewatkins) wrote :

On Mon, Mar 25, 2019 at 02:48:32PM -0000, Gonéri Le Bouder wrote:
> I'm actually not sure why we need the 'sync' option since the volume
> is supposed to be in RO mode.

mount_cb has a rw argument, so "the volume is supposed to be in RO mode"
isn't an accurate statement within that function.

> This is the reason why the uses of sync sounds more like a Linux
> specific work around. Since there is no real isolation, FreeBSD ends
> up with the parameter too and we have to ignore it. This is the reason
> why I took this approach: I enable it only on Linux, since it "may" be
> relevant there, I disable it everywhere else.

In the FreeBSD mount man page I pointed to ([0]), sync is a supported
mount option, so I don't believe that this should be Linux-specific.

[0] https://www.freebsd.org/cgi/man.cgi?mount(8)

Gonéri Le Bouder (goneri) wrote :

I may misunderstand something. The goal here is to open a device, read some data and unmount the device. The device being a little disk or a CDROM. I don't think cloud-init do any modification, does it? So, it should just use the 'ro' parameter and ignores the 'sync' flag.

Dan Watkins (daniel-thewatkins) wrote :

On Mon, Mar 25, 2019 at 05:09:02PM -0000, Gonéri Le Bouder wrote:
> I may misunderstand something. The goal here is to open a device, read
> some data and unmount the device. The device being a little disk or a
> CDROM. I don't think cloud-init do any modification, does it? So, it
> should just use the 'ro' parameter and ignores the 'sync' flag.

That's how mount_cb is used where you're modifying the call in
DataSourceConfigDrive, yes. However, the mount_cb function has a rw
parameter, which means you need to support that parameter correctly in
your changes to that function; your current change to mount_cb removes
sync from _all_ mount calls made on non-Linux platforms.

Furthermore, removing the sync flag in places where it is currently
acceptable to pass it is a change in behaviour, which could end up
causing a regression on platforms we don't (or can't) test. My feeling
is that "we probably don't need it" is not compelling enough
justification for such a change, without clear benefits.

(I would suggest that you target your changes at specifically the case
that you are seeing fail, rather than modifying default behaviour for
the silent majority of working cases.)

Gonéri Le Bouder (goneri) wrote :

mount_cb's rw flag is always False. This is actually what leads my to thing the `sync` flag is pointless. I'm convinced we can drop the two parameters.

In addition, the current code base relies on blkid which is Linux specific. To get cloud-init to work on FreeBSD, we need to mock blkid. In addition, there is no Linux 'distro' beside FreeBSD. So I doubt such change will impact anyone.

From my view it's a good opportunity to clean up some old irrelevant function parameters and simplify the whole code base.

Dan Watkins (daniel-thewatkins) wrote :

On Tue, Mar 26, 2019 at 02:29:51PM -0000, Gonéri Le Bouder wrote:
> mount_cb's rw flag is always False. This is actually what leads my to
> thing the `sync` flag is pointless. I'm convinced we can drop the two
> parameters.

I wouldn't be opposed to dropping the rw argument to mount_cb if it
isn't used anywhere, but that isn't what you had proposed thus far. :)

> In addition, the current code base relies on blkid which is Linux
> specific. To get cloud-init to work on FreeBSD, we need to mock blkid.

cloud-init does already work on FreeBSD for some data sources. I
certainly agree that this particular data source needs some work. :)

> In addition, there is no Linux 'distro' beside FreeBSD.

I don't really know what this means; FreeBSD isn't a Linux 'distro', and
there are certainly operating systems other than GNU/Linux and FreeBSD.
(Also, platform.system() reads from uname data, so it could feasibly be
any string.)

> So I doubt such change will impact anyone.

I also doubt that such a change would impact anyone, but "it will
probably be fine" is (annoyingly, I agree) not the standard we have to
hold ourselves to for a piece of software that is fundamentally
important to boot in so many environments, and that is very difficult to
debug if it fails (because you often won't be able to access an instance
on which cloud-init has failed to run). We have to be very
conservative.

> From my view it's a good opportunity to clean up some old irrelevant
> function parameters and simplify the whole code base.

As I said above, I don't disagree. :)

Gonéri Le Bouder (goneri) wrote :

Dan Watkins <email address hidden> writes:

> On Tue, Mar 26, 2019 at 02:29:51PM -0000, Gonéri Le Bouder wrote:
>> mount_cb's rw flag is always False. This is actually what leads my to
>> thing the `sync` flag is pointless. I'm convinced we can drop the two
>> parameters.
>
> I wouldn't be opposed to dropping the rw argument to mount_cb if it
> isn't used anywhere, but that isn't what you had proposed thus far. :)

I will be happy to refresh my patch to take that into account.

>> In addition, the current code base relies on blkid which is Linux
>> specific. To get cloud-init to work on FreeBSD, we need to mock blkid.
>
> cloud-init does already work on FreeBSD for some data sources. I
> certainly agree that this particular data source needs some work. :)

For the record, I've also started that:
https://github.com/goneri/cloud-init/commit/5129be3bdc947c629c971c08c811db1997454edf

It's a network renderer for FreeBSD based on what already exists. It's
not ready yet for review. I will push a PR later.

>> In addition, there is no Linux 'distro' beside FreeBSD.
>
> I don't really know what this means; FreeBSD isn't a Linux 'distro', and
> there are certainly operating systems other than GNU/Linux and FreeBSD.
> (Also, platform.system() reads from uname data, so it could feasibly be
> any string.)

Yes, I put the quotes on purpose. I was refering to the
./cloudinit/distros/freebsd.py file which is a directory called
'distro'.

>> So I doubt such change will impact anyone.
>
> I also doubt that such a change would impact anyone, but "it will
> probably be fine" is (annoyingly, I agree) not the standard we have to
> hold ourselves to for a piece of software that is fundamentally
> important to boot in so many environments, and that is very difficult to
> debug if it fails (because you often won't be able to access an instance
> on which cloud-init has failed to run). We have to be very
> conservative.
>
>> From my view it's a good opportunity to clean up some old irrelevant
>> function parameters and simplify the whole code base.
>
> As I said above, I don't disagree. :)

Thanks for the clarification. Can I push a patch for review without the
'rw' and 'sync'? Or should I instead refact the current patch and keep
the sync=True by default.

Cheers,
--
    Gonéri Le Bouder

Dan Watkins (daniel-thewatkins) wrote :

On Tue, Mar 26, 2019 at 03:27:24PM -0000, Gonéri Le Bouder wrote:
> > On Tue, Mar 26, 2019 at 02:29:51PM -0000, Gonéri Le Bouder wrote:
> >> mount_cb's rw flag is always False. This is actually what leads my to
> >> thing the `sync` flag is pointless. I'm convinced we can drop the two
> >> parameters.
> >
> > I wouldn't be opposed to dropping the rw argument to mount_cb if it
> > isn't used anywhere, but that isn't what you had proposed thus far. :)
>
> I will be happy to refresh my patch to take that into account.

Thanks!

> >> In addition, the current code base relies on blkid which is Linux
> >> specific. To get cloud-init to work on FreeBSD, we need to mock blkid.
> >
> > cloud-init does already work on FreeBSD for some data sources. I
> > certainly agree that this particular data source needs some work. :)
>
> For the record, I've also started that:
> https://github.com/goneri/cloud-init/commit/5129be3bdc947c629c971c08c811db1997454edf
>
> It's a network renderer for FreeBSD based on what already exists. It's
> not ready yet for review. I will push a PR later.

Excellent, I look forward to it!

> >> In addition, there is no Linux 'distro' beside FreeBSD.
> >
> > I don't really know what this means; FreeBSD isn't a Linux 'distro', and
> > there are certainly operating systems other than GNU/Linux and FreeBSD.
> > (Also, platform.system() reads from uname data, so it could feasibly be
> > any string.)
>
> Yes, I put the quotes on purpose. I was refering to the
> ./cloudinit/distros/freebsd.py file which is a directory called
> 'distro'.

Aha, OK, I see what you mean. For clarity, mount_cb doesn't use
cloud-init's definition of 'distro' at all; it's calling
platform.system() which is in the Python stdlib[0].

[0] https://docs.python.org/3/library/platform.html#platform.system

> >> From my view it's a good opportunity to clean up some old irrelevant
> >> function parameters and simplify the whole code base.
> >
> > As I said above, I don't disagree. :)
>
> Thanks for the clarification. Can I push a patch for review without the
> 'rw' and 'sync'? Or should I instead refact the current patch and keep
> the sync=True by default.

I think one patch dropping both rw and sync would be acceptable.

Gonéri Le Bouder (goneri) wrote :

Thanks Dan,

I rebased my patch to drop the two parameters.

Gonéri Le Bouder (goneri) wrote :

Dan, could you please take a look a the updated patch?

FAILED: Continuous integration, rev:aeb278c94e66b03eef15bf1fd79f6f465d099824
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~goneri/cloud-init/+git/cloud-init/+merge/364997/+edit-commit-message

https://jenkins.ubuntu.com/server/job/cloud-init-ci/677/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/677/rebuild

review: Needs Fixing (continuous-integration)
Dan Watkins (daniel-thewatkins) wrote :

This looks good to me, but as it's a change to a function that's used in a lot of places, I would like a second review.

review: Approve

PASSED: Continuous integration, rev:aeb278c94e66b03eef15bf1fd79f6f465d099824
https://jenkins.ubuntu.com/server/job/cloud-init-ci/678/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/678/rebuild

review: Approve (continuous-integration)
Ryan Harper (raharper) :
Ryan Harper (raharper) wrote :

Im +1 on this, update the comment one way or other and that should be enough.

Gonéri Le Bouder (goneri) wrote :

Hum. What comment should I change?

Ryan Harper (raharper) wrote :

Thanks for working on getting this mergable.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
2index 76b1661..6416525 100755
3--- a/cloudinit/sources/DataSourceAzure.py
4+++ b/cloudinit/sources/DataSourceAzure.py
5@@ -407,7 +407,7 @@ class DataSourceAzure(sources.DataSource):
6 elif cdev.startswith("/dev/"):
7 if util.is_FreeBSD():
8 ret = util.mount_cb(cdev, load_azure_ds_dir,
9- mtype="udf", sync=False)
10+ mtype="udf")
11 else:
12 ret = util.mount_cb(cdev, load_azure_ds_dir)
13 else:
14diff --git a/cloudinit/sources/DataSourceConfigDrive.py b/cloudinit/sources/DataSourceConfigDrive.py
15index 564e3eb..571d30d 100644
16--- a/cloudinit/sources/DataSourceConfigDrive.py
17+++ b/cloudinit/sources/DataSourceConfigDrive.py
18@@ -72,15 +72,12 @@ class DataSourceConfigDrive(openstack.SourceMixin, sources.DataSource):
19 dslist = self.sys_cfg.get('datasource_list')
20 for dev in find_candidate_devs(dslist=dslist):
21 try:
22- # Set mtype if freebsd and turn off sync
23- if dev.startswith("/dev/cd"):
24+ if util.is_FreeBSD() and dev.startswith("/dev/cd"):
25 mtype = "cd9660"
26- sync = False
27 else:
28 mtype = None
29- sync = True
30 results = util.mount_cb(dev, read_config_drive,
31- mtype=mtype, sync=sync)
32+ mtype=mtype)
33 found = dev
34 except openstack.NonReadable:
35 pass
36diff --git a/cloudinit/util.py b/cloudinit/util.py
37index 385f231..ea4199c 100644
38--- a/cloudinit/util.py
39+++ b/cloudinit/util.py
40@@ -1679,7 +1679,7 @@ def mounts():
41 return mounted
42
43
44-def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True,
45+def mount_cb(device, callback, data=None, mtype=None,
46 update_env_for_mount=None):
47 """
48 Mount the device, call method 'callback' passing the directory
49@@ -1726,18 +1726,7 @@ def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True,
50 for mtype in mtypes:
51 mountpoint = None
52 try:
53- mountcmd = ['mount']
54- mountopts = []
55- if rw:
56- mountopts.append('rw')
57- else:
58- mountopts.append('ro')
59- if sync:
60- # This seems like the safe approach to do
61- # (ie where this is on by default)
62- mountopts.append("sync")
63- if mountopts:
64- mountcmd.extend(["-o", ",".join(mountopts)])
65+ mountcmd = ['mount', '-o', 'ro']
66 if mtype:
67 mountcmd.extend(['-t', mtype])
68 mountcmd.append(device)

Subscribers

People subscribed via source and target branches