Detect kernel version before swap file creation by otubo · Pull Request #428 · canonical/cloud-init

otubo · 2020-06-11T11:44:28Z

According to man page `man 8 swapon', "Preallocated swap files are
supported on XFS since Linux 4.18". This patch checks for kernel version
before attepting to create swapfile, using dd for XFS only on kernel
versions <= 4.18 or btrfs.

This is the updated version taking into consideration all the comments on the PR#218

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

According to man page `man 8 swapon', "Preallocated swap files are supported on XFS since Linux 4.18". This patch checks for kernel version before attepting to create swapfile, using dd for XFS only on kernel versions <= 4.18 or btrfs. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

Replaced the regex by the os function calls and put it on a separate function inside util.py module. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

otubo · 2020-06-11T11:52:48Z

I left the commit log in this new PR so we could check the history. But it could be squashed upon final merge.

igalic

👀

…g the runtime of cloud-init.

OddBloke

Thanks for the follow-up! This approach looks good to me. I have one inline comment, and I'd like to see some testing for the new util function and new cc_mounts behaviour.

Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>

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

OddBloke

Thanks for adding the tests! I have some comments. :-)

Wrapping test_kernel_version() into class TestKernelVersion() and adding pytest parametrize decoration. In order to have it working, the cache decoration would have to be removed, though. Otherwise the function cache would always return the same value and would fail the tests. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

Splitting test_swap_creation_method into 3 different cases for different filesystems. Leaving the methods way of asserting the test with only cc_mounts.handle() function. Like test_swap_integrity() does. Signed-off-by: Eduardo Otubo <otubo@redhat.com> [0] pytest-dev/pytest#3484

OddBloke

Thanks for the update! Just one inline question about the tests.

Creating a new class of tests TestSwapFileCreation() for this purpose. The class still uses test_helpers.FilesystemMockingTestCase to mock a file system so swap file can be created. Also added a check on cloudinit/config/cc_mounts.py to make sure the swap file really exists, which is not the case on unittests. The swap file is not being created, the test just makes sure if the correct calls are being done. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

otubo · 2020-07-09T14:23:07Z

Looking at the diff, it looks incredibly messy to review. Looks like I replaced TestFstabHandling with TestSwapFileCreation and then again readded TestFstabHandling down below. If this is too terrible to review, I can close this PR and open a new one with squashed commits for easier reading :)

otubo · 2020-07-09T14:35:28Z

argh, I could swear I had this flake stuff fixed before my commit

github-actions · 2020-07-30T00:18:00Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging powersj, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

OddBloke

This is pretty much there, I think, thanks for your work! I've requested one additional test inline, but that should be largely copy/paste of what you already have for the other tests, so hopefully not too tricky!

mitechie · 2020-08-18T13:32:14Z

Thanks for the update. Github is noting you have some conflicts before landing. Can you peek at those, please?

…blank line added by merge

blackboxsw

+1 thanks for the updates @otubo

Review comments and unit tests are addressed.

otubo added 4 commits June 11, 2020 11:31

Changing into a tuple and fixing pycodestyle

f2eae6a

fixing pycodestyle: Missing whitespace

73f97e9

Replace regex by os function calls

c746b03

Replaced the regex by the os function calls and put it on a separate function inside util.py module. Signed-off-by: Eduardo Otubo <otubo@redhat.com>

adding new line to avoid flake8 complain

e5fc9d4

igalic reviewed Jun 11, 2020

View reviewed changes

Comment thread cloudinit/util.py

Comment thread cloudinit/util.py

adding @lru_cache() as the kernel version is unlikely to change durin…

183c5ae

…g the runtime of cloud-init.

OddBloke self-assigned this Jun 16, 2020

OddBloke suggested changes Jun 17, 2020

View reviewed changes

Comment thread cloudinit/config/cc_mounts.py Outdated

otubo and others added 3 commits June 23, 2020 14:11

Update cloudinit/config/cc_mounts.py

bf109a0

Co-authored-by: Daniel Watkins <daniel@daniel-watkins.co.uk>

Adding tests for util.kernel_version() new function

ec1f08b

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

Adding test to check swap file creation method

d2cd4b4

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

otubo requested a review from OddBloke June 23, 2020 15:17

otubo added 2 commits June 24, 2020 13:59

Wrong mock wasn't actually testing kernel_version

d01111b

Mocking os.uname() should set release and not return_value

4f6b9fc

OddBloke suggested changes Jun 24, 2020

View reviewed changes

Comment thread tests/unittests/test_util.py Outdated

Comment thread tests/unittests/test_util.py Outdated

Comment thread tests/unittests/test_handler/test_handler_mounts.py Outdated

Comment thread tests/unittests/test_handler/test_handler_mounts.py Outdated

otubo added 2 commits July 6, 2020 18:13

OddBloke previously requested changes Jul 6, 2020

View reviewed changes

Comment thread tests/unittests/test_handler/test_handler_mounts.py Outdated

removing blank line again

8082a78

otubo requested a review from OddBloke July 10, 2020 12:37

Merge branch 'master' into detect-kernel-version

474c47b

OddBloke reviewed Jul 15, 2020

View reviewed changes

Comment thread tests/unittests/test_handler/test_handler_mounts.py Outdated

github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Jul 30, 2020

Removing fstab writing from tests

a521f6c

otubo requested a review from OddBloke July 31, 2020 07:45

OddBloke removed the stale-pr Pull request is stale; will be auto-closed soon label Aug 6, 2020

OddBloke reviewed Aug 6, 2020

View reviewed changes

Comment thread tests/unittests/test_handler/test_handler_mounts.py

Adding an additional test for the case of fallocate on xfs

2a8239c

otubo requested a review from OddBloke August 17, 2020 12:36

otubo added 2 commits August 18, 2020 16:50

Merge branch 'master' into detect-kernel-version

97801c7

fixing merge, removing pytest import (done by my patch) and removing …

9b0ac2c

…blank line added by merge

blackboxsw approved these changes Aug 18, 2020

View reviewed changes

blackboxsw merged commit b749548 into canonical:master Aug 18, 2020

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 20.3 #3774

Closed

Conversation

otubo commented Jun 11, 2020

Uh oh!

otubo commented Jun 11, 2020

Uh oh!

igalic left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

otubo commented Jul 9, 2020

Uh oh!

otubo commented Jul 9, 2020

Uh oh!

Uh oh!

github-actions Bot commented Jul 30, 2020

Uh oh!

OddBloke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mitechie commented Aug 18, 2020

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants