Skip to content
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

Allow setting size for non-existing LUKS devices #848

Conversation

vojtechtrefny
Copy link
Member

The LUKS device "inherits" its size from the parent so we are
just setting the parent's size here. This allows automatic size
adjustments for format minimum size for growable devices with
sizes smaller than format minimum size (e.g. using --grow in
kickstart for PVs).

Resolves: rhbz#1836269


I'm not sure about this one. There are two issues here. First, the size setter currently doesn't work for LUKSDevice at all. I think it's python problem with mixing the new style properties (using the @property decorator) and the old style properties and the setter from StorageDevice is currently not being inherited which causes the installation crash with the can't set attribute error. Second issue is about setting size for the LUKSDevice during _set_format -- the device factory first creates a 1 MiB partition, formats to PV format and updates its size to 8 MiB (we are talking about --size=1 --grow in the kickstart) and after this we add the encryption by creating LUKSDevice which is now 6 MiB (size of the partition minus LUKS metadata) and try to format it to PV updating its size to 8 MiB which now fails.
I've added a new test case to cover this new LUKS size getter/setter functionality (setter is covered with test_size_setter inherited from StorageDeviceSizeTest) but I'm not sure if this is the best approach and this might affect other things not covered by our test suite.

The LUKS device "inherits" its size from the parent so we are
just setting the parent's size here. This allows automatic size
adjustments for format minimum size for growable devices with
sizes smaller than format minimum size (e.g. using --grow in
kickstart for PVs).

Resolves: rhbz#1836269
@vojtechtrefny
Copy link
Member Author

Jenkins, test this please.

@dwlehman
Copy link
Contributor

I never added a setter to LUKSDevice because I figured we could adjust the slave/backing device size directly. Your patch looks reasonable to me, and I don't think there is much risk given that it has never been possible to set size directly for LUKSDevice.

It looks like the traceback from the referenced bug is a regression caused by caec289, which is amazing given its commit date of Feb 2019.

Copy link
Contributor

@dwlehman dwlehman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@vojtechtrefny
Copy link
Member Author

t looks like the traceback from the referenced bug is a regression caused by caec289, which is amazing given its commit date of Feb 2019.

I think something also changed in Python and the setter was inherited from StorageDevice before.

@vojtechtrefny vojtechtrefny merged commit d1d27f8 into storaged-project:3.2-devel May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants