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

Add infrastructure for supporting multiple token slots per unlock method #3720

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add infrastructure for supporting multiple token slots per unlock method #3720

wants to merge 2 commits into from

Conversation

jbaublitz
Copy link
Member

Related to #3598

@jbaublitz jbaublitz self-assigned this Nov 20, 2024
@jbaublitz jbaublitz marked this pull request as draft November 20, 2024 14:58
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 2 times, most recently from cdbc29b to 0748815 Compare December 21, 2024 11:40
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 2 times, most recently from fe201f5 to 48fe461 Compare January 2, 2025 17:09
@jbaublitz
Copy link
Member Author

@mulkieran As we discussed, I'll let you handle the tests that involve stratis-cli and the cert tests.

@jbaublitz
Copy link
Member Author

@mulkieran I'm going to say that udev tests passing is good enough to call stratis-storage/libcryptsetup-rs#379 ready for merge. We should likely put out a new release for the locking updates and this method when you get a chance.

@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from 48fe461 to b896b52 Compare January 3, 2025 15:33
@jbaublitz jbaublitz marked this pull request as ready for review January 3, 2025 15:35
@jbaublitz jbaublitz marked this pull request as draft January 3, 2025 15:36
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 3 times, most recently from a912fa5 to b6dd766 Compare January 7, 2025 15:15
@packit-as-a-service Packit-as-a-Service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3720
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@jbaublitz jbaublitz marked this pull request as ready for review January 7, 2025 16:29
mulkieran
mulkieran requested changes Jan 7, 2025
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.

Two small request...

src/dbus_api/api/manager_3_8/api.rs Outdated Show resolved Hide resolved
src/dbus_api/api/manager_3_8/api.rs Outdated Show resolved Hide resolved
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 4 times, most recently from ea065d7 to 7cb14de Compare January 7, 2025 20:04
@jbaublitz
Copy link
Member Author

The usage of i was pretty widespread but I think I've tracked down and changed all of the places I used it.

@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from 7cb14de to 918ccf1 Compare January 7, 2025 20:07
mulkieran
mulkieran requested changes Jan 8, 2025
View reviewed changes
Copy link
Member

@mulkieran mulkieran left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Found one disagreement between published D-Bus API and dbus method. I may be able to find one more...nope, just that one showing up in tests.

src/dbus_api/pool/pool_3_8/methods.rs Show resolved Hide resolved
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 2 times, most recently from 02ac615 to da6701d Compare January 8, 2025 16:21
mulkieran
mulkieran requested changes Jan 8, 2025
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.

Problem with GetManagedObjects result...

src/dbus_api/pool/mod.rs Outdated Show resolved Hide resolved
mulkieran
mulkieran requested changes Jan 9, 2025
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.

One more D-Bus bug found...still working.

src/dbus_api/api/manager_3_8/api.rs Show resolved Hide resolved
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from da6701d to 19e4fdb Compare January 9, 2025 17:13
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from 19e4fdb to 18be963 Compare January 9, 2025 17:30
@jbaublitz
Copy link
Member Author

@mulkieran This should be all set now!

mulkieran
mulkieran requested changes Jan 9, 2025
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.

One more D-Bus problem and a request regarding stratis-min parser.

src/bin/stratis-min/stratis-min.rs Show resolved Hide resolved
src/dbus_api/pool/mod.rs Outdated Show resolved Hide resolved
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch 5 times, most recently from 8c15a88 to 36459de Compare January 13, 2025 16:28
@jbaublitz
Copy link
Member Author

@mulkieran I just fixed a bug where creating a pool with Clevis only failed. Please update any testing to use the most recent version with the bug fix.

mulkieran
mulkieran requested changes Jan 13, 2025
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.

Just the one thing we discussed in standup so far...

src/engine/sim_engine/pool.rs Outdated Show resolved Hide resolved
@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from 36459de to dcdcd16 Compare January 13, 2025 19:12
mulkieran
mulkieran requested changes Jan 13, 2025
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.

Still working...

src/engine/sim_engine/pool.rs Outdated Show resolved Hide resolved
mulkieran
mulkieran requested changes Jan 13, 2025
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 is the last one I found in testing...

src/engine/sim_engine/pool.rs Outdated Show resolved Hide resolved
mulkieran
mulkieran requested changes Jan 13, 2025
View reviewed changes
Copy link
Member

@mulkieran mulkieran left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm noting one more discrepancy of the sim with strat engine. The strat engine behavior seems correct to me:

  • In the strat engine, it is forbidden to unbind the last remaining binding. In the sim engine, you can remove the last remaining binding, which is a little awkward.

(If, when unbinding, you specify a non-existant token slot, stratid will return Ok(false) and stratis-cli will report that no change occurred. This is true for sim and strat engine and that seems correct to me.)

This is with keyring, not tested with Clevis yet.

@jbaublitz jbaublitz force-pushed the issue-stratisd-3598 branch from dcdcd16 to f3637df Compare January 14, 2025 03:47
@jbaublitz
Copy link
Member Author

@mulkieran This should be ready to go again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mulkieran mulkieran mulkieran requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@jbaublitz jbaublitz

Labels
None yet
Projects
2025January
Status: In Review
Milestone
No milestone
Development

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