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

Add initial disk wipe support to vogelkop #84

Merged
merged 4 commits into from
Jun 12, 2024
Merged

Conversation

mmlb
Copy link
Member

@mmlb mmlb commented Jun 11, 2024

This PR is mainly to add initial disk wipe support to vogelkop. Only nvme based devices are supported/attempted at the moment, more will follow.

I've also tweaked the partition & format commands so they are noun-verb and with subcommands to match what raid is doing. It seems silly to have a partition subcommand right now but I think it makes sense. Someone might want to modify or grow partitions, maybe even wipe them. I don't see myself having the need but if anyone else does the setup is ready for them. The old non-subcommand verb-noun are still around, albeit hidden, so no existing scripts should break (for now).

mmlb added 3 commits June 11, 2024 17:04
Just like raid. The old names are deprecated but still runnable.
Doesn't do anything yet, I just wanted to get the scaffolding up.
Need DriveWiper support.
// Pick the most appropriate wipe based on the disk type and/or features supported
var wiper actions.DriveWiper
switch drive.Protocol {
case "nvme":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I imagine the intention here is to add more cases in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, shouldn't this 'figure out what tool to use based upon protocol' be moved into ironlib?

Copy link
Member Author

Choose a reason for hiding this comment

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

I imagine the intention here is to add more cases in the future?

Yep after this is out I’ll be adding more things in like in the ironlib example (and also hdparm based SED once I add it to ironlib)

Copy link
Member Author

@mmlb mmlb Jun 12, 2024

Choose a reason for hiding this comment

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

Also, shouldn't this 'figure out what tool to use based upon protocol' be moved into ironlib?

Yep but I’d want the Drives to support that themselves instead of having a function that needs to grow more and more complex as features are needed. For example maybe we want to add a flag to vogelkop that says "never use fill_zeros" or maybe we only want to allow that for HDDs. Maybe we want secure erase methods only. Adding flags to the hypothetical ironlib function for this means lots of params or lots of individual functions. Either way I don't think this belongs in ironlib. I think other tools should take ironlib's raw features and smooth them out for their specific use case instead of throwing everything into ironlib.

I do want ironlib to grow some more features/smarts but leave it as functionality only and leave policy decisions to its users/callers. I want the Drives to be able to keep a ref to the tool(s) able to wipe it and offer those up to the ironlib users so that it can make decisions regarding which tool/method it wants to use. I started in on that in metal-toolbox/ironlib#160 but that can be seen as a breaking change (even though ironlib is still v0... really it should have been v1 by now) so will have to wait until work starts on v2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like scattering this logic everywhere that needs it is wrong... Maybe there should be another lib in the middle that contains the logic you'll otherwise implement in vogelkop. I imagine reusing this logic elsewhere... but maybe ultimately nothing else will ever use it...

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what I think vogelkop should be. Right now its also very cli driven but there's no reason it can't be a library for disk stuff that also comes with a cli. Similar to libcurl & curl.

Copy link

Choose a reason for hiding this comment

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

I think what you're both talking about here is the distinction between implementation and behaviour. Implementation, for instance, might vary from one disk to the next: what we do to a disk is dependent on the disk type, and given the disk type is an interface we would want a call that reflects the behaviour which is diskX.destroy_all_data_with_prejudice() or diskX.lightly_mangle_data() or diskX.do_what_you_can_in_5_minutes().

Copy link

Choose a reason for hiding this comment

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

The point about hiding the implementation for a disk type is that I should be able to test that the implementation for a disk type produces a result without having to know how it does that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what you're both talking about here is the distinction between implementation and behaviour. Implementation, for instance, might vary from one disk to the next: what we do to a disk is dependent on the disk type, and given the disk type is an interface we would want a call that reflects the behaviour which is diskX.destroy_all_data_with_prejudice() or diskX.lightly_mangle_data() or diskX.do_what_you_can_in_5_minutes().

Not quite because the disk type is not an interface and has none, its just a POD struct. So we either provided a function to take the drive and return back some behavior or we make the drive implement an interface so that we can leave that to the disk or we let caller deal with it. I vote to let the caller deal with it instead of providing the function and later implement interfacification in the drive :D

This gives vogelkop the ability to actually wipe a drive now. Will be
adding more device types/features soon.
Copy link

@iawells iawells left a comment

Choose a reason for hiding this comment

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

I want to know where we prove that vogelkop is doing this on real hardware and not by running a bunch of horrible 'I'll just deploy a machine and we'll figure out how to make it call vogelkop in precisely the right way' sort of fashion. We have ironlib, which clears disks, but it has no real-world tests - so this is the first opportunity to prove this is actually doing what it claims to do when we throw it at our glorious assortment of disk types.

(You might also argue that ironlib should contain those tests or have a test suite - but my point is it has to go somewhere for this to be prod-ready, or you're really not proving this is better than the current model of 'call the functions in shellscript', and it's certainly a lot more complex to follow since you have to walk three repos to work out what it does.)

// Pick the most appropriate wipe based on the disk type and/or features supported
var wiper actions.DriveWiper
switch drive.Protocol {
case "nvme":
Copy link

Choose a reason for hiding this comment

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

I think what you're both talking about here is the distinction between implementation and behaviour. Implementation, for instance, might vary from one disk to the next: what we do to a disk is dependent on the disk type, and given the disk type is an interface we would want a call that reflects the behaviour which is diskX.destroy_all_data_with_prejudice() or diskX.lightly_mangle_data() or diskX.do_what_you_can_in_5_minutes().

// Pick the most appropriate wipe based on the disk type and/or features supported
var wiper actions.DriveWiper
switch drive.Protocol {
case "nvme":
Copy link

Choose a reason for hiding this comment

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

The point about hiding the implementation for a disk type is that I should be able to test that the implementation for a disk type produces a result without having to know how it does that.

@mmlb
Copy link
Member Author

mmlb commented Jun 12, 2024

I want to know where we prove that vogelkop is doing this on real hardware and not by running a bunch of horrible 'I'll just deploy a machine and we'll figure out how to make it call vogelkop in precisely the right way' sort of fashion. We have ironlib, which clears disks, but it has no real-world tests - so this is the first opportunity to prove this is actually doing what it claims to do when we throw it at our glorious assortment of disk types.

(You might also argue that ironlib should contain those tests or have a test suite - but my point is it has to go somewhere for this to be prod-ready, or you're really not proving this is better than the current model of 'call the functions in shellscript', and it's certainly a lot more complex to follow since you have to walk three repos to work out what it does.)

I'll take this on as a separate action but do not want to gate this PR on it.

@mmlb
Copy link
Member Author

mmlb commented Jun 12, 2024

Merging and will figure out some functional tests for here and/or ironlib next.

@mmlb mmlb merged commit 4f13d6f into metal-toolbox:main Jun 12, 2024
2 checks passed
@mmlb mmlb deleted the disk-wipe branch June 12, 2024 21:10
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.

3 participants