-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| var diskCommand = &cobra.Command{ | ||
| Use: "disk", | ||
| Short: "Modifies disks", | ||
| } | ||
|
|
||
| func init() { | ||
| rootCmd.AddCommand(diskCommand) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "cmp" | ||
| "context" | ||
| "errors" | ||
| "os" | ||
|
|
||
| "github.com/bmc-toolbox/common" | ||
| "github.com/metal-toolbox/ironlib" | ||
| "github.com/metal-toolbox/ironlib/actions" | ||
| "github.com/metal-toolbox/ironlib/utils" | ||
| "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func init() { | ||
| cmd := &cobra.Command{ | ||
| Use: "wipe /dev/disk", | ||
| Short: "Wipes all data from a disk", | ||
| Args: func(_ *cobra.Command, args []string) error { | ||
| if len(args) < 1 { | ||
| return errors.New("requires at least one arg") // nolint:goerr113 | ||
| } | ||
|
|
||
| _, err := os.Open(args[0]) | ||
| return err | ||
| }, | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| timeout, err := cmd.Flags().GetDuration("timeout") | ||
| if err != nil { | ||
| logger.With("error", err).Fatal("--timeout argument is invalid") | ||
| } | ||
|
|
||
| verbose, err := cmd.Flags().GetBool("verbose") | ||
| if err != nil { | ||
| logger.With("error", err).Fatal("--verbose argument is invalid") | ||
| } | ||
|
|
||
| driveName := args[0] | ||
|
|
||
| logger := logrus.New() | ||
| logger.Formatter = new(logrus.TextFormatter) | ||
| if verbose { | ||
| logger.SetLevel(logrus.TraceLevel) | ||
| } | ||
| l := logger.WithField("drive", driveName) | ||
|
|
||
| ctx := cmp.Or(cmd.Context(), context.Background()) | ||
| ctx, cancel := context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
|
|
||
| collector, err := ironlib.New(logger) | ||
| if err != nil { | ||
| l.WithError(err).Fatal("exiting") | ||
| } | ||
|
|
||
| inventory, err := collector.GetInventory(ctx, actions.WithDynamicCollection()) | ||
| if err != nil { | ||
| l.WithError(err).Fatal("exiting") | ||
| } | ||
|
|
||
| var drive *common.Drive | ||
| for _, d := range inventory.Drives { | ||
| if d.LogicalName == driveName { | ||
| drive = d | ||
| break | ||
| } | ||
| } | ||
| if drive == nil { | ||
| l.Fatal("unable to find disk") | ||
| } | ||
|
|
||
| // Pick the most appropriate wipe based on the disk type and/or features supported | ||
| var wiper actions.DriveWiper | ||
| // nolint:gocritic // will have more cases soon, remove nolint then | ||
| switch drive.Protocol { | ||
| case "nvme": | ||
| wiper = utils.NewNvmeCmd(verbose) | ||
| } | ||
|
|
||
| if wiper == nil { | ||
| l.Fatal("failed find appropriate wiper drive") | ||
| } | ||
|
|
||
| err = wiper.WipeDrive(ctx, logger, drive) | ||
| if err != nil { | ||
| l.Fatal("failed to wipe drive") | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| diskCommand.AddCommand(cmd) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| var partitionCommand = &cobra.Command{ | ||
| Use: "partition", | ||
| Short: "Modifies partitions", | ||
| } | ||
|
|
||
| func init() { | ||
| rootCmd.AddCommand(partitionCommand) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I imagine the intention here is to add more cases in the future?
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.
Also, shouldn't this 'figure out what tool to use based upon protocol' be moved into ironlib?
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.
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)
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.
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.
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.
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...
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.
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.
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.
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().
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.
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.
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.
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