Merge ~smoser/cloud-init:bug/before-fsck into cloud-init:master

[Scott Moser]

systemd: make systemd-fsck run after cloud-init.service

cloud-init.service may write filesystems (fs_setup) or re-partition
(disk_setup) disks.

If systemd-fsck is running on a device while that is occuring
then the partitioning or mkfs might fail due to the device being busy.
Alternatively, the fsck might fail and cause subsequent mount to fail.

LP: #1691489

[Scott Moser (smoser)]

This *seems* to work for me, but I'd like to have some review by someone else who is knowledgeable in systemd.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6442476bda38f617605a433d2fe3cfa1af88b17b
https://jenkins.ubuntu.com/server/job/cloud-init-ci/344/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/344
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/344
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/344
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/344

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Balint,
Could you please comment on this?

[Balint Reczey (rbalint)]

This potentially slows down startup a bit, but running fsck and cloudinit.service in parallel is not safe thus the change looks good to me and I have no better idea.

The change is also consistent with cloud-init's documentation:
http://cloudinit.readthedocs.io/en/0.7.9/topics/boot.html#network

[Balint Reczey (rbalint)]

review: Approve

[Balint Reczey (rbalint)]

review: Approve

[Balint Reczey (rbalint)]

I think stopping and later restarting systemd-fsckd.service only during reformatting would be a better way of handling this case because that solution would not slow down systems which are not affected.
What do you think?

review: Needs Information

[Scott Moser (smoser)]

Balint,
How would that work?

I think you're essentially saying to run:
 systemctl stop systemd-fsck@<something>.service

But to do that, I have to figure out which are the appropriate values for 'something'. That could be 'dev-disk-by-uuid=XXXX', 'dev-disk-by-label=XXXX' for any filesystem on the block device.

Additionally, if I did:
 a. stop
 b. <try to reformat/adjust>
 c. restart
there is a potential that my 'stop' happens before has started. does a 'stop' of a service prevent it from starting if it would start later on its own?

[Balint Reczey (rbalint)]

> Balint,
> How would that work?

Please consider my previous comment superseded by this MP:
https://code.launchpad.net/~rbalint/cloud-init/+git/cloud-init/+merge/326252

I put a comment in the bug but forgot also noting it here.

>
> I think you're essentially saying to run:
> systemctl stop systemd-fsck@<something>.service
>
> But to do that, I have to figure out which are the appropriate values for
> 'something'. That could be 'dev-disk-by-uuid=XXXX', 'dev-disk-by-label=XXXX'
> for any filesystem on the block device.
>
> Additionally, if I did:
> a. stop
> b. <try to reformat/adjust>
> c. restart
> there is a potential that my 'stop' happens before has started. does a 'stop'
> of a service prevent it from starting if it would start later on its own?

[David Britton (dpb)]

Hi Balint --

Please fix up the structural problems with these MPs.

1) set this MP "rejected"
2) Also reference this MP in the description of
https://code.launchpad.net/~rbalint/cloud-init/+git/cloud-init/+merge/326252
3) Link the bug to the 326252 merge

Thanks!

On Tue, Jul 11, 2017 at 8:20 AM, Balint Reczey <email address hidden>
wrote:

>
>
>
> > Balint,
> > How would that work?
>
> Please consider my previous comment superseded by this MP:
> https://code.launchpad.net/~rbalint/cloud-init/+git/cloud-
> init/+merge/326252
>
> I put a comment in the bug but forgot also noting it here.
>
> >
> > I think you're essentially saying to run:
> > systemctl stop systemd-fsck@<something>.service
> >
> > But to do that, I have to figure out which are the appropriate values for
> > 'something'. That could be 'dev-disk-by-uuid=XXXX',
> 'dev-disk-by-label=XXXX'
> > for any filesystem on the block device.
> >
> > Additionally, if I did:
> > a. stop
> > b. <try to reformat/adjust>
> > c. restart
> > there is a potential that my 'stop' happens before has started. does a
> 'stop'
> > of a service prevent it from starting if it would start later on its own?
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/324191
> You are subscribed to branch cloud-init:master.
>
> Launchpad-Message-Rationale: Subscriber
> Launchpad-Message-For: davidpbritton
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~cloud-init-dev/cloud-init/+git/cloud-init:master
> Launchpad-Project: cloud-init
>

--
David Britton <email address hidden>

[Scott Moser (smoser)]

David,

Balint's work flow here is completely valid.
He was reviewing https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324191 and wanted to suggest a different path for fixes so he proposed a merge into that branch (~smoser/cloud-init:bug/before-fsck).

I often do the same when I think its easier to just show code for what i mean than explaining in words the changes I'd like.

The one thing I would suggest is that when that is done you should comment in the original merge proposal. something to the affect of:
  Hey, i thought of a different way to do this and worked up a merge proposal at http://.....

[David Britton (dpb)]

Thanks Scott -- Agreed, I mostly just want to see the links back and forth.

On Tue, Jul 11, 2017 at 9:05 AM, Scott Moser <email address hidden> wrote:

> David,
>
> Balint's work flow here is completely valid.
> He was reviewing https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/324191 and wanted to suggest a different path for fixes so he
> proposed a merge into that branch (~smoser/cloud-init:bug/before-fsck).
>
> I often do the same when I think its easier to just show code for what i
> mean than explaining in words the changes I'd like.
>
> The one thing I would suggest is that when that is done you should comment
> in the original merge proposal. something to the affect of:
> Hey, i thought of a different way to do this and worked up a merge
> proposal at http://.....
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-
> init/+merge/324191
> You are subscribed to branch cloud-init:master.
>
> Launchpad-Message-Rationale: Subscriber
> Launchpad-Message-For: davidpbritton
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~cloud-init-dev/cloud-init/+git/cloud-init:master
> Launchpad-Project: cloud-init
>

--
David Britton <email address hidden>

[Balint Reczey (rbalint)]

We discussed the proposal with Scott and since invalid fstab entries could appear in other cases, not just when re-partitioning devices on Azure I dropped my MP and I'm ok with this change.
If the delayed fsck turns out to slow down boot significantly we could try narrowing the application of the delay.

[Scott Moser (smoser)]

Balint,
thanks for your review!

[Scott Moser (smoser)]

Balint,
I had a thoguht that might improve this a bit.
cloud-init could (i think) make sure that fscks are delayed for each of the filesystems it "owns" (the ones it would write 'comment=cloud-config' on in /etc/fstab).

[Balint Reczey (rbalint)]

Scott,
I think this would not be fully reliable in a case when the disk where the "owned" partitions reside also contains not "owned" partitions but it gets re-partitioned.

diff --git a/setup.py b/setup.py
index 1197ece..b1bde43 100755
--- a/setup.py
+++ b/setup.py
@@ -125,6 +125,7 @@ INITSYS_FILES = {
                 for f in (glob('systemd/*.tmpl') +
                           glob('systemd/*.service') +
                           glob('systemd/*.target')) if is_f(f)],
+    'systemd.fsck-dropin': ['systemd/systemd-fsck@.service.d/cloud-init.conf'],
     'systemd.generators': [f for f in glob('systemd/*-generator') if is_f(f)],
     'upstart': [f for f in glob('upstart/*') if is_f(f)],
 }
@@ -134,6 +135,9 @@ INITSYS_ROOTS = {
     'sysvinit_deb': 'etc/init.d',
     'sysvinit_openrc': 'etc/init.d',
     'systemd': pkg_config_read('systemd', 'systemdsystemunitdir'),
+    'systemd.fsck-dropin': (
+        os.path.sep.join([pkg_config_read('systemd', 'systemdsystemunitdir'),
+                          'systemd-fsck@.service.d'])),
     'systemd.generators': pkg_config_read('systemd',
                                           'systemdsystemgeneratordir'),
     'upstart': 'etc/init/',
diff --git a/systemd/systemd-fsck@.service.d/cloud-init.conf b/systemd/systemd-fsck@.service.d/cloud-init.conf
new file mode 100644
index 0000000..0bfa465
--- /dev/null
+++ b/systemd/systemd-fsck@.service.d/cloud-init.conf
@@ -0,0 +1,2 @@
+[Unit]
+After=cloud-init.service