-
Notifications
You must be signed in to change notification settings - Fork 1.2k
storage: Full support for Stratis V2 pools #21927
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
base: main
Are you sure you want to change the base?
Conversation
eae4457
to
580ce86
Compare
580ce86
to
b720bf8
Compare
7e2fef6
to
bb10442
Compare
c75b8dd
to
e6e6cc8
Compare
60bc81e
to
26f667a
Compare
26f667a
to
717b286
Compare
717b286
to
d5bf943
Compare
There is a lot of uncovered code, but most of it is for the "r6" API, which we don't have on Fedora anymore, where we collect the coverage data. |
Haven't looked at the video yet as I can't listen atm. @mvollmer IIUC this is both a refactor to API usage and also a feature added on top of that. I'd ideally want them split into two PRs or made clear in the description here Do you have any GH or Jira issues this relates to or any user stories to show the issue of missing keyserver and allowing the changing of passphrases? If no issues exist then lay out some simpler user stories so I can follow what is the "purpose" of the PR so I can review with that in mind. Helps the designer in me to approach it better, and the tech lead in me to review the code with that purpose to see if there are any better ways :D Not shutting anything down just trying to get more details before I venture into a bigger review |
Sure, we can do this in steps. Here are some considerations: The important thing (IMO) is the new feature. This is a new feature in Stratis 3.8 and Cockpit should follow along and offer a UI for it. See https://stratis-storage.github.io/stratis-release-notes-3-8-0/ To implement the feature, we need the new API revision "r8". If we don't implement it, we can stay on "r6". Stratis supports old API revisions indefinitely, and we only move to a newer one when required. We have skipped "r7" completely, for example, because we didn't need any of its changes. Until we implement the multiple passphrase feature, managing encrypted Stratis pools in Cockpit that have been created by Stratis 3.8. is going to be weird, however. For example, if you add two passphrases to your pool on the command line, Cockpit will only show one and lets you remove it, but if you do, nothing changes in the UI because the pool still has a passphrase. Being weird is okay, so I don't think there is a lot of urgency here, but we should catch up in a reasonable time. So I'd say splitting this into separate PRs brings no advantage, but I can split it into two commits.
The user stories will have to come from the Stratis people, Cockpit is just implementing a UI for it. (This is unfortunately how it normally goes, Cockpit doesn't drive a lot of new features, we mostly react to what is happening in the OS.) There is this in our private Design project: #21836 And this in Jira: https://issues.redhat.com/browse/COCKPIT-1272
Although I said that it is Stratis that is driving the feature, I might have some idea of what is going on. :-) When Stratis first came on the scene, I gave a couple remarks to the Stratis people when implementing the first UI for it in Cockpit. I can remember saying:
and:
None of that was in any way important, but I feel I am getting my wishes fulfilled now! :-) So the user story is probably simply "As a system administrator, I want to use multiple passphrases with my encrypted Stratis pools for the same reason that I am using multiple passphrases for my other encrypted filesystems." We can involve the Stratis people if you feel it is worth their time. |
d5bf943
to
d978732
Compare
But keep using only "r6" for now.
This exposes the machinery used to create the action area of cards to be used elsewhere. It also gains some new features: - All actions can be shoved into the menu. - Butons can have spinners while their action runs. The latter is used when starting Stratis pools, which can take a bit when they are encrypted.
d978732
to
0d087e9
Compare
I failed. :-/ I have split out a number of auxiliary changes into their own commits and changed the description to put the focus on the feature instead of the implementation challenge, but I didn't see any nice way to start using using the r8 API without changing the UI at the same time. Just doesn't make sense. :-) |
They can have multiple passphrases and keyservers per pool. This needs the "r8" API revision, and this commit changes Cockpit to use r8 in preference to r6 in general. We should only use a single Stratis API revision in a given session and this also makes it trivial to drop support for r6 when the time comes.
0d087e9
to
62809b7
Compare
Ah, there is this paragraph in the Stratris 3.8 release notes: This change enables a number of use cases that the previous scheme did not allow. For example, a pool might be configured so that it can be unlocked with one key belonging to a storage administrator, for occasional necessary maintenance, and with a different key by the designated user of the pool. |
if (for_menu(a)) | ||
items.push(make_menu_item(a)); | ||
else | ||
buttons.push( | ||
<StorageButton key={a.title} onClick={() => a.action(false)} | ||
kind={a.danger ? "danger" : null} excuse={a.excuse}> | ||
kind={a.danger ? "danger" : null} excuse={a.excuse} |
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 added line is not executed by any test.
if (client.stratis_interface_revision < 8) { | ||
return client.stratis_manager.CreatePool(name, | ||
devs, | ||
key_desc ? [true, key_desc] : [false, ""], | ||
clevis_info ? [true, clevis_info] : [false, ["", ""]]); |
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.
These 5 added lines are not executed by any test.
key_desc ? [[[false, 0], key_desc]] : [], | ||
clevis_info ? [[[false, 0], clevis_info[0], clevis_info[1]]] : [], |
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.
These 2 added lines are not executed by any test.
if (client.stratis_interface_revision < 8) { | ||
const val = pool.KeyDescription; | ||
if (val[0] && val[1][0]) | ||
result.push({ slot: null, keydesc: val[1][1] }); |
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.
These 4 added lines are not executed by any test.
if (pool.MetadataVersion == 1) { | ||
if (val[0] && val[1][0]) | ||
result.push({ slot: null, keydesc: val[1][1] }); | ||
} else if (pool.MetadataVersion == 2) { |
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 added line is not executed by any test.
const keydesc = get_v1_keydesc(uuid); | ||
return with_stored_passphrase(client, keydesc, passphrase, start); | ||
} else { | ||
return start(); |
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 added line is not executed by any test.
} | ||
} else { | ||
// XXX - does "any" always work, also for unencrypted pools? | ||
return await stratis_r8_manager_start_pool(uuid, unlock_method ? "any" : "-", passphrase); |
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 added line is not executed by any test.
if (client.stratis_interface_revision < 8) { | ||
const key_desc = get_v1_keydesc(uuid); | ||
const stored_keydescs = await get_stored_keydescs(); | ||
if (stored_keydescs.includes(key_desc)) | ||
return await client.stratis_manager.StartPool(uuid, "uuid", [true, "keyring"]).then(std_reply); |
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.
These 5 added lines are not executed by any test.
} else if (key_desc && !clevis_info) { | ||
return unlock_with_passphrase(); | ||
return manager_start_pool(uuid, null, null); | ||
} else if (have_passphrase && have_clevis) { |
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 added line is not executed by any test.
return manager_start_pool(uuid, "clevis", null).catch(prompt_for_passphrase); | ||
} else if (!have_passphrase && have_clevis) { | ||
return manager_start_pool(uuid, "clevis", null); | ||
} else if (have_passphrase && !have_clevis) { |
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 added line is not executed by any test.
Just filling the description is good enough for me
Yeah I meant more like: "As a user of Stratis, I want to be able to add passphrases and keyservers within Cockpit." This is a simple example but can be made into more broader like "As a user of btrfs, I want to be able to create, name, and delete btrfs snapshots" or more specific depending on the ask. Just wanted something from a "stratis user's POV" or "non-user of stratis POV" who want to do something with stratis.
Yep exactly something like this! In short I want to know different user POVs to not miss out on anything that can mess up the system too easily. Always assume people are stupid and will click through things without thinking, so needs to be adequate information and confirmation before doing dangerous actions for example. Can also be written more declarative or something. Who: System administrators I know Stratis is driving us to do this (IIUC) and they setup their features. But we're doing web design too so we need to make it good enough on first release kinda thing :D Anyway, I've watched the video now and I'll take a bit of a closer look soon! |
Demo: https://www.youtube.com/watch?v=VUSnt6fLTwk
Screenshot of new "Encryption tokens" card: