-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect kernel version before swap file creation #428
Conversation
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>
I left the commit log in this new PR so we could check the history. But it could be squashed upon final merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
…g the runtime of cloud-init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: Eduardo Otubo <otubo@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Looking at the diff, it looks incredibly messy to review. Looks like I replaced |
argh, I could swear I had this flake stuff fixed before my commit |
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Thanks for the update. Github is noting you have some conflicts before landing. Can you peek at those, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 thanks for the updates @otubo
Review comments and unit tests are addressed.
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