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.

Sign up for GitHub

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

Jump to bottom

Bug fix for liminal device code #3381

Closed
wants to merge 1 commit into from
Closed

Bug fix for liminal device code #3381

wants to merge 1 commit into from

Conversation

jbaublitz
Copy link
Member

No description provided.

@jbaublitz jbaublitz requested a review from mulkieran July 11, 2023 16:10
@jbaublitz jbaublitz self-assigned this Jul 11, 2023
mulkieran
mulkieran reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

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

This looks correct to me, but I'm worried that it might have a bad consequence, due to the issue discussed here: #3353 . The consequence would be that there is a pool that is setup but has no mounted filesystems. But the LUKS metadata gets corrupted. The pool cannot be started and all the devices are torn down and now the pool is basically impossible to set up in the normal way and may be entirely unrecoverable. That may very well be better than the alternatives, but I'ld like to introduce the possibility for your consideration.

@jbaublitz
Copy link
Member Author

I think there are arguments on both ends. If we do tear down the pool, we could permanently lose data. If we don't, we could leave the devices in an unlocked state unintentionally which could leave sensitive data available to an attacker. I think data loss may be more of a concern here and I'm happy to simply default to putting the partially constructed pool into the partially constructed pool slot to be torn down through stratis pool stop later given that we now have the ability to do that through stratisd. What are your thoughts?

@mulkieran
Copy link
Member

I think there are arguments on both ends. If we do tear down the pool, we could permanently lose data. If we don't, we could leave the devices in an unlocked state unintentionally which could leave sensitive data available to an attacker. I think data loss may be more of a concern here and I'm happy to simply default to putting the partially constructed pool into the partially constructed pool slot to be torn down through stratis pool stop later given that we now have the ability to do that through stratisd. What are your thoughts?

That seems like the best choice that we have available.

@jbaublitz
Copy link
Member Author

This is ready for review again.

@jbaublitz
Copy link
Member Author

I think this is actually a real bug. I need to fix the tests.

@jbaublitz jbaublitz force-pushed the bug-fix-liminal branch 3 times, most recently from 110de51 to 3e0f140 Compare July 13, 2023 18:21
@jbaublitz jbaublitz requested a review from mulkieran July 14, 2023 04:00
@jbaublitz
Copy link
Member Author

Blocked by #3385

@mulkieran mulkieran mentioned this pull request Jul 20, 2023
Closed
@jbaublitz
Bug fix for liminal device code
2c7c31b 
The path where starting pools automatically if the metadata indicates
that they should be started did not correctly handle placing the
partially stopped pool in the designated part of liminal devices.
@jbaublitz
Copy link
Member Author

Merged in #3389

@jbaublitz jbaublitz closed this Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mulkieran mulkieran Awaiting requested review from mulkieran

Assignees

@jbaublitz jbaublitz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbaublitz @mulkieran