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

Accept "adv" key as well as "thp" key #3700

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented Oct 14, 2024

Closes #3699

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-3700
  • And now you can install the packages.

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

@mulkieran mulkieran self-assigned this Oct 14, 2024
@mulkieran mulkieran force-pushed the issue_stratisd_3699 branch 5 times, most recently from a2b02fb to 2ea1cc6 Compare October 15, 2024 16:22
@mulkieran mulkieran marked this pull request as ready for review October 15, 2024 16:31
@mulkieran
Copy link
Member Author

@jbaublitz This is a port of #3701 . It has to distribute some bits of that PR into v2 as well, and that's probably the only thing worth checking for correctness.

@mulkieran
Copy link
Member Author

rebased

The Clevis config specification allows both a "thp" key, which the code
already checks for and an "adv" key, which the code has been unaware of.

Change name of Clevis config verification method to match expanded
specification. Change header documentation to match.

Change error message to match other changes.

Signed-off-by: mulhern <amulhern@redhat.com>
The "adv" value is either a string containing the filename or more JSON.

Signed-off-by: mulhern <amulhern@redhat.com>
Do not strip config of thumbprint entry, as the comparison that required
that operation has been removed.

It would be nice to also check equality of URLs, if the pin is "tang".
But that is a lot of work for a small benefit. If a user for some reason
attempts to create a pool with the same name, the same devices, and the
same pin, as the pool was created with previously, then
stratisd will return a value indicating that the pool was already
created even if something else about the Clevis config is different.
This seems acceptable.

Signed-off-by: mulhern <amulhern@redhat.com>
The return value was used to decide whether to remove a key/value pair,
but that no longer needs to be removed.

The argument passed to it was only used for a comparison which is no
longer being made and an error message display that can use the
value it was cloned from instead.

If the config was malformed, then it would error early, i.e., before
attempting to bind the first blockdev. Now it will just error when it
tries to bind the first blockdev. This is simpler to reason about.

Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
@mulkieran
Copy link
Member Author

rebased. We expect all tests to succeed.

@mulkieran mulkieran merged commit 07d780d into stratis-storage:master Oct 17, 2024
49 checks passed
@mulkieran mulkieran deleted the issue_stratisd_3699 branch October 17, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

stratisd CreatePool method does not know about Clevis tang "adv" properties, only "thp".
2 participants