Do not use fallocate in swap file creation on xfs. by otubo · Pull Request #70 · canonical/cloud-init

otubo · 2019-11-29T11:04:32Z

When creating a swap file on an xfs filesystem, fallocate cannot be used.
Doing so results in failure of swapon and a message like:
swapon: swapfile has holes

The solution here is to maintain a list (currently containing only XFS)
of filesystems where fallocate cannot be used. The, on those fileystems
use the slower but functional 'dd' method.

Based on initial pull request:
https://code.launchpad.net/~adobrawy/cloud-init/+git/cloud-init/+merge/354680

And further comments from Scott Moser:
http://paste.ubuntu.com/p/8pVhczVqG3/

Signed-off-by: Eduardo Otubo otubo@redhat.com

otubo · 2019-11-29T11:05:35Z

This is just the first draft RFC and check. No tests written yet.

OddBloke

Broadly speaking, I'm happy with the direction here. Some high-level questions/comments left inline.

(Note that I do have some more specific comments to make in a future review pass, but they may no longer apply after we've worked through my high-level concerns so I figured it would be a waste of everyone's time to leave them at this juncture. :-)

powersj · 2019-12-11T23:14:12Z

Marking WIP till tests are written

OddBloke

Thanks for the updates! I have a few more thoughts inline.

igalic

🙋

otubo · 2019-12-19T14:52:01Z

Related bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1772505

blackboxsw · 2019-12-19T23:33:35Z

Eduardo thanks for the contribution here github-side!

Just one more step so we can correlate old launchpad accounts that have signed the CLA to your github account so that we have accountability for which accounts actually signed the Contribution License Agreement.

Please run ./tools/migrate-lp-user-to-github to allow us to track your ownership of both launchpad and github accounts. Details are in our updated hacking guide https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-once

otubo · 2019-12-20T14:33:33Z

Eduardo thanks for the contribution here github-side!

Just one more step so we can correlate old launchpad accounts that have signed the CLA to your github account so that we have accountability for which accounts actually signed the Contribution License Agreement.

Please run ./tools/migrate-lp-user-to-github to allow us to track your ownership of both launchpad and github accounts. Details are in our updated hacking guide https://cloudinit.readthedocs.io/en/latest/topics/hacking.html#do-these-things-once

Thanks Chad!
I started the process here: #135

otubo · 2019-12-20T14:36:01Z

Also, I have to say that I already have a commit on cloud-init on the github repository before I run this tool: f1094b1
Not sure if there's anything to be done on this commit for some reason :-)

igalic

👍

When creating a swap file on an xfs filesystem, fallocate cannot be used. Doing so results in failure of swapon and a message like: swapon: swapfile has holes The solution here is to maintain a list (currently containing only XFS) of filesystems where fallocate cannot be used. The, on those fileystems use the slower but functional 'dd' method. Based on initial pull request: https://code.launchpad.net/~adobrawy/cloud-init/+git/cloud-init/+merge/354680 And further comments from Scott Moser: http://paste.ubuntu.com/p/8pVhczVqG3/ Signed-off-by: Eduardo Otubo <otubo@redhat.com>

In order to avoid duplicate code the function create_swapfile() was reduced and a new function create_swap() was introduced. create_swapfile identifies which type of filesystem is currently in use (xfs or btrfs) and calls create_swap(), that chooses between dd and fallocate - this, in case of failure will try dd as fallback. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

Signed-off-by: Eduardo Otubo <otubo@redhat.com>

* replacing function get_fstype() by util.del_file() * removing get_fstype() by it's return call, avoinding a new function call * replacing os.chmod() by util.chmod() and removing from try block Signed-off-by: Eduardo Otubo <otubo@redhat.com>

OddBloke

One remaining comment, we're very close!

OddBloke

LGTM now, thank you!

blackboxsw

Thanks otubo for the fixes!

norrisjeremy · 2020-04-14T21:16:48Z

This commit broke swap file creation, with the following error:

cc_mounts.py[WARNING]: failed to setup swap: %d format: a number is required, not str

norrisjeremy · 2020-04-14T21:33:36Z

+                  fname, fstype, method)
+
+        if method == "fallocate":
+            cmd = ['fallocate', '-l', '%dM' % size, fname]


Size is a string, not a number, so '%s' format specifier should be used.

norrisjeremy · 2020-04-14T21:34:32Z

+            cmd = ['fallocate', '-l', '%dM' % size, fname]
+        elif method == "dd":
+            cmd = ['dd', 'if=/dev/zero', 'of=%s' % fname, 'bs=1M',
+                   'count=%d' % size]


While I don't use xfs, I suspect this too should be using a '%s' format specifier since size is a string.

norrisjeremy · 2020-04-14T21:43:00Z

+def create_swapfile(fname, size):
+    """Size is in MiB."""
+
+    errmsg = "Failed to create swapfile '%s' of size %dMB via %s: %s"


This too should likely be a '%s' format modifier instead of '%d' since size is a string.

powersj · 2020-04-14T21:43:52Z

@otubo thoughts on the above comments?

@norrisjeremy if you have not already can you open a bug: https://bugs.launchpad.net/cloud-init/+filebug

norrisjeremy · 2020-04-14T21:53:15Z

@powersj Please see https://bugs.launchpad.net/cloud-init/+bug/1872836

norrisjeremy · 2020-04-14T21:54:18Z

FYI, I discovered this when attempting to spin up the latest 20.04 beta cloud image and noticed that swapfile creation didn't work in my cloud-config.yml. Hopefully this can be fixed before next weeks release?

powersj · 2020-04-15T21:00:38Z

Fix in #316

OddBloke reviewed Nov 29, 2019

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py Outdated

Comment thread cloudinit/config/cc_mounts.py Outdated

Comment thread cloudinit/config/cc_mounts.py Outdated

powersj added the wip Work in progress, do not land label Dec 11, 2019

otubo requested a review from OddBloke December 12, 2019 12:03

OddBloke suggested changes Dec 17, 2019

View reviewed changes

igalic reviewed Dec 17, 2019

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py

otubo force-pushed the fallocate_swap branch from 9a84a8f to 84736fb Compare December 19, 2019 14:22

otubo requested review from OddBloke and igalic December 19, 2019 14:47

blackboxsw added the CLA not signed The submitter of the PR has not (yet) signed the CLA label Dec 19, 2019

blackboxsw added CLA signed The submitter of the PR has signed the CLA and removed CLA not signed The submitter of the PR has not (yet) signed the CLA labels Jan 9, 2020

igalic approved these changes Jan 13, 2020

View reviewed changes

smoser reviewed Jan 13, 2020

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py Outdated

blackboxsw requested changes Jan 13, 2020

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py Outdated

Comment thread cloudinit/config/cc_mounts.py Outdated

otubo added 5 commits January 15, 2020 13:13

First draft for integrity swap tests

ab128f1

Signed-off-by: Eduardo Otubo <otubo@redhat.com>

Fixing multiple things upon review

2b4c443

Final fixes before merge

adad3bd

* replacing function get_fstype() by util.del_file() * removing get_fstype() by it's return call, avoinding a new function call * replacing os.chmod() by util.chmod() and removing from try block Signed-off-by: Eduardo Otubo <otubo@redhat.com>

otubo force-pushed the fallocate_swap branch from 2e99b45 to adad3bd Compare January 15, 2020 12:34

OddBloke suggested changes Jan 20, 2020

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py Outdated

Removing outdated docstring

263a651

OddBloke approved these changes Jan 23, 2020

View reviewed changes

Merge branch 'master' into fallocate_swap

5391a1a

OddBloke removed the wip Work in progress, do not land label Jan 23, 2020

blackboxsw approved these changes Jan 23, 2020

View reviewed changes

blackboxsw merged commit 6603706 into canonical:master Jan 23, 2020

norrisjeremy reviewed Apr 14, 2020

View reviewed changes

blackboxsw mentioned this pull request May 29, 2020

Ubuntu/xenial #395

Merged

TheRealFalcon mentioned this pull request Jun 18, 2020

Manual verification for lp-1781781 cloud-init/ubuntu-sru#121

Merged

Conversation

otubo commented Nov 29, 2019

Uh oh!

otubo commented Nov 29, 2019

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

powersj commented Dec 11, 2019

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

otubo commented Dec 19, 2019

Uh oh!

blackboxsw commented Dec 19, 2019

Uh oh!

otubo commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

otubo commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

norrisjeremy commented Apr 14, 2020

Uh oh!

norrisjeremy Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

norrisjeremy Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

norrisjeremy Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

powersj commented Apr 14, 2020

Uh oh!

norrisjeremy commented Apr 14, 2020

Uh oh!

norrisjeremy commented Apr 14, 2020

Uh oh!

powersj commented Apr 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

otubo commented Dec 20, 2019 •

edited

Loading

otubo commented Dec 20, 2019 •

edited

Loading