Add a Drive type and Wiper interface #160

mmlb · 2024-06-03T19:31:16Z

What does this PR do

Removes DiskWiper from StorageControllerAction

It will be back better in a different PR. I intend to add a Wipe or similar method to StorageController that will clear raid devices and optionally wipe each individual disk.

Some light refactoring of model

Filenames did not make sense considering the contents so I refactored the files so they do.

Creates actions/wipe package with a small interface'

Created a separate package since actions has grown pretty crazy. Packages in go are cheap and its more idiomatic to have many small packages vs few larger ones. actions/wipe just defines the Wiper interface.

Add model/drive package with Drive type and WipersGetter interface

This is the most important commit. I swapped out use of common.Drive for model.Drive. model.Drive embeds common.Drive so usage should be pretty seamless. I've also added a WipersGetter interface here since it needs it so should define what it needs.

The main idea is for a Drive to be able to wipe itself, or at least tell you what you need to wipe it. Right now the API is going to be something like the following pseudo code:

for each disk
	for each utility in disk.Wipers
		if utility.Wipe(disk) succeeds proceed to next disk
	if no utility succeeded error out

Maybe it'll evolve to just disk.Wipe (or grow that option too).

Stores LogicalName in drives

Pretty handy to have the LogicalName to be able to wipe using a utility so store it when we have it. For those times we don't (mvcli) we'll probably have to lean on StorageController.Wipe "recursively" wiping the disks too.

Store nvme discovered capabilities in Drive

Similar to LogicalName where we need some data from discovery later for wiping so store it in the pre-existing field. We're going to need it for nvme so we can return any/all of the sanitize/format wipers depending on what the device supports.

Finally, make nvme a WipersGetter

NVME devices are the first that know how to wipe themselves.

DoctorVin

Seems pretty reasonable to me, but I'm not an SME here so I'd prefer to let @joelrebel or @splaspood comment.

DoctorVin · 2024-06-03T19:40:08Z

fixtures/supermicro/x11dph-t.go

Why did the device names change here?

The LogicalNames weren't being saved into the common.Drive/common.Common so weren't actually checked. Now that they are they are just different, not sure when the disparity happened though and I don't have a way to track it down I think.

I would prefer to not check the logical names, because they are going to move around as the kernel version changes

The fixtures are compared using assert.Equal, to avoid the check I'd have to a deep check myself just to skip the LogicalName field. I don't think ironlib should drop the LogicalName, instead tools using ironlib should be able to decide if they need to drop it.

DoctorVin · 2024-06-03T19:56:22Z

utils/lsblk_test.go

-var fixtureLsblkDrives = []*common.Drive{
-	{
+var fixtureLsblkDrives = []*drive.Drive{
+	{Drive: common.Drive{


Seeing this in action, I find the stuttering confusing. Is there a way we can define your data structure so that we don't have Drive 3x?

Tell me about it! I don't think there's much clean up available here, as far as I'm aware of. I very much want to keep it embedding a common.Drive but the compiler doesn't like it if I remove any of the types here and complains about "missing type in composite literal" :/

DoctorVin · 2024-06-03T19:57:49Z

utils/mvcli.go

-		drive := &common.Drive{
+	drives := make([]*drive.Drive, len(devices))
+	for i, d := range devices {
+		drives[i] = drive.New(&common.Drive{


Not all of this is your doing, but the proximity of drive and common in various cases is really awful here.

yeah there's lots of low hanging fruits for cleanups

which I'd like to clean up little by little here and there as PRs for other stuff comes in and then I'm sure at some point it'll make sense to a specific PR for some clean ups.

joelrebel

Left some feedback inline, besides that, I'd like us to be more cautious before merging this change,

The Component Convertors in Alloy and Flasher expect a common.Drive type is this going to fail now?
Can you verify the inventory example continues to works as expected?
How does this work with other disk utilities that don't provide a method to wipe the drive?
What happens if a caller gets a ironlib.Drive object after running smartctl or lshw, is the Wipe() method available?
lshw returns a common.Drive type, should that be wrapped as well?
Add back the wipe example once this is ready

Is there another way to approach, because it changes the contract we have with the callers.
Would a method similar to this work for this case instead

ironlib/actions/update.go

Line 151 in 41022f4

    
           // UpdateDrive identifies the drive eligible for update from the inventory and runs the firmware update utility based on the drive vendor

?

joelrebel · 2024-06-04T12:12:54Z

model/drive/drive.go

@@ -0,0 +1,29 @@
+package drive


I'd prefer this stays at the model package level, creating a package for each component gets messy and will introduce import cycle issues as we go ahead.

I created this package because I've found that many of the packages are already too wide and would have caused import cycles (I think it had to do with utilitie, I don't quite remember what it was but I definitely had one or 2 in the first iterations of this) and bad naming.

This may have been an over correction though :D let me give it a try as part of model.

ok so I dropped models/drive and just moved drive.go up, looks alright.

joelrebel · 2024-06-04T12:17:40Z

utils/smartctl.go

 		if err != nil {
 			return nil, err
 		}

 		item := &common.Drive{
 			Common: common.Common{
-				Vendor:      common.VendorFromString(smartctlAll.ModelName),
+				LogicalName: d.Name,


This is may cause our stored data to flip-flop based on what the kernel identifies the logical name as (the order, the naming strategy), is this necessary?

This is necessary to be able to wipe disks w/o having to store the logicalname externally (a map maybe). Its also necessary because this is deep in ironlib and we lose the logicalname once we return to the caller. It seems to me that the lower layers should have all the data and higher level should be in charge or removing things it doesn't want. It definitely makes sense to not send LogicalName to an external store but that should be take care of in the sending side not data collection side right?

joelrebel · 2024-06-04T12:19:00Z

utils/smartctl.go

 		if err != nil {
 			return nil, err
 		}

 		item := &common.Drive{
 			Common: common.Common{
-				Vendor:      common.VendorFromString(smartctlAll.ModelName),
+				LogicalName: d.Name,
+				Vendor:      cmp.Or(common.VendorFromString(smartctlAll.ModelName), common.VendorFromString(smartctlAll.ModelFamily)),


nit, split this into two lines for readability or set the variable earlier

joelrebel · 2024-06-04T12:21:06Z

actions/interface.go

@@ -85,7 +85,7 @@ type InventoryCollector interface {
 // DriveCollector defines an interface to return disk drive inventory
 type DriveCollector interface {
 	UtilAttributeGetter
-	Drives(ctx context.Context) ([]*common.Drive, error)
+	Drives(ctx context.Context) ([]*drive.Drive, error)


Since this is a potentially breaking change - can you make sure the inventory collection example continues to report drives as expected a server?

Ah good call, I hadn't tried those will do though.

just built the example for both main and this pr and ran them back to back and there's no difference

[10:27:54]-[~/p/g/m/i/e/inventory]-[manny@c3mv18nix] strings inventory-main | grep vcs.revision build vcs.revision=41022f4154dcdc16cbe9eab19be1cd1c720ce37e build vcs.revision=41022f4154dcdc16cbe9eab19be1cd1c720ce37e [10:27:58]-[~/p/g/m/i/e/inventory]-[manny@c3mv18nix] strings inventory-pr160 | grep vcs.revision build vcs.revision=4071cecd836eae31a20e87d628cfc8010b531370 build vcs.revision=4071cecd836eae31a20e87d628cfc8010b531370 [10:28:25]-[~/p/g/m/i/e/inventory]-[manny@c3mv18nix] sudo ./inventory-main 2>/dev/null | jq -S . >inventory-main.json; sudo ./inventory-pr160 2>/dev/null | jq -S . >inventory-pr160.json [10:28:30]-[~/p/g/m/i/e/inventory]-[manny@c3mv18nix] cmp inventory-main.json inventory-pr160.json && echo no diff no diff

inventory-pr160.json
inventory-main.json

joelrebel · 2024-06-04T12:24:06Z

fixtures/supermicro/x11dph-t.go

I would prefer to not check the logical names, because they are going to move around as the kernel version changes

model/update.go

joelrebel · 2024-06-04T12:27:26Z

actions/wipe/wipe.go

@@ -0,0 +1,24 @@
+package wipe


should we instead name these packages by the component they target so its a bit more obvious, so this one would be called drive

I'm a bit wary of creating nested packages, in these cases we have to be sure we're not going to import anything from the parent package,
since the parent will most likely import from this one.

I don't really like that this ends up in actions but I do think it makes sense it is an action being taken. I intend to have Wipe action for storage controllers too so we can use it to delete raid configs and nvme namespaces.

joelrebel · 2024-06-04T12:31:15Z

utils/nvme.go

@@ -300,7 +298,11 @@ func (n *Nvme) WipeDisk(ctx context.Context, logger *logrus.Logger, device strin
 	return n.wipe(ctx, logger, device, caps)
 }

-func (n *Nvme) wipe(ctx context.Context, logger *logrus.Logger, device string, caps []*common.Capability) error {
+func (n *Nvme) Wipers(d *drive.Drive) []wipe.Wiper {


Can you include a comment here on what this method does exactly

added a comment, PTAL

mmlb · 2024-06-04T14:08:34Z

Left some feedback inline, besides that, I'd like us to be more cautious before merging this change,
* The Component Convertors in Alloy and Flasher expect a `common.Drive` type is this going to fail now?

Maybe but I don't mind updating/fixing them. If so it shouldn't be too bad since drive.Drive embeds common.Drive and is thus small edits to convert.

* Can you verify the inventory example continues to works as expected?

* How does this work with other disk utilities that don't provide a method to wipe the drive?

I'm taking small steps to figure that out as I build out wiping support in vogelkop. I plan to add support for SEI, SED, blkdiscard and ultimately fall back to NewFillZeroCmd. Also I meant to check/catch nil wipersGetter (and see that I need to rename the struct member) in Wipers so that it returns nil too instead nil dereference. Callers should be iterating over the slice. They can detect if any method succeeded and if non did (or none were returned) they would be able to detect that in handle appropriately.

* What happens if a caller gets a `ironlib.Drive` object after running `smartctl` or `lshw`,  is the `Wipe()` method available?

* lshw returns a `common.Drive` type, should that be wrapped as well?

Yep I realized eod yesterday that I'm going to have to also create an ironlib.Device since common.Device refers to concrete implementation and not interface (not advocating for a Drive interface...). At first I groaned thinking this is bad but have come around. Wrapping external dependencies makes a lot of sense in many examples and I think it definitely applies here.

* Add back the wipe example once this is ready
Is there another way to approach, because it changes the contract we have with the callers. Would a method similar to this work for this case instead

ironlib/actions/update.go

Line 151 in 41022f4

// UpdateDrive identifies the drive eligible for update from the inventory and runs the firmware update utility based on the drive vendor

?

There's only one implementation of that so it doesn't seem too bad right but once more and more DriveUpdaters are added to the code base its going to get onerous. We're going to have to implement the DriveUpdating in $utility and also update GetDriveUpdater so it knows about the new utility. Contrast that with how I've done Wipers, I'm embedding that this Drive knows how to wipe itself and so don't need to keep actions in sync with utilities. If we flip it around and add an UpdateFirmware method to Drive the msecli utility would set itself to handle it like I do for nvme and wipe. My idea is to let the objects be self contained vs having to build up scaffolding around it that needs to be kept in sync.

mmlb · 2024-06-05T13:23:03Z

I'm working on an ironlib.Device too so the lshw can use it and then have everything in this repo using model.Drive. So far so good, code changes are very tiny the biggest pain is in the snapshot-type tests. Also assert.Equal compares private fields which isn't great but I have a work around that seems to work.

mmlb · 2024-06-06T19:14:47Z

Broke out the non-breaking refactors/clean ups into #161

The WipeDrive in StorageControllerAction was mostly serving as an example but we already have multiple in examples/, so we can drop this.

Needs to be its own package to avoid import cycles. Should be its own package even if there would be no import cycles since its definitely its own concept and actions is already way too large.

--- Define and use our own Drive type Going to invert how drive wiping works by having the drive know how to wipe itself, so we need our own type to be able to give it methods. --- Make Nvme a WipersGetter and pass to drive.New Now we have a way to get the wipe utitlies for an nvme drive easily.

mmlb requested review from joelrebel, splaspood, turegano-equinix and DoctorVin as code owners June 3, 2024 19:31

mmlb force-pushed the drive branch from 01b138f to 53ca297 Compare June 3, 2024 19:34

DoctorVin reviewed Jun 3, 2024

View reviewed changes

mmlb force-pushed the drive branch from 0e0d1f9 to cd70a9b Compare June 3, 2024 20:16

joelrebel requested changes Jun 4, 2024

View reviewed changes

mmlb force-pushed the drive branch from 4071cec to 798c24d Compare June 4, 2024 19:50

mmlb requested a review from joelrebel June 4, 2024 19:51

mmlb force-pushed the drive branch from 4d0463d to 44d8d0b Compare June 4, 2024 19:52

mmlb force-pushed the drive branch from 205e1b1 to 6490d27 Compare June 5, 2024 14:27

mmlb marked this pull request as draft June 6, 2024 14:33

mmlb added the v2 Breaking change appropriate for ironlib v2 label Jun 6, 2024

mmlb force-pushed the drive branch from 44f1a60 to da21321 Compare June 7, 2024 14:14

mmlb added 4 commits June 11, 2024 10:20

Drop wiping code in StorageControllerAction

5ff8e91

The WipeDrive in StorageControllerAction was mostly serving as an example but we already have multiple in examples/, so we can drop this.

Create actions/wipe package

b34fc38

Needs to be its own package to avoid import cycles. Should be its own package even if there would be no import cycles since its definitely its own concept and actions is already way too large.

model.Device

ff770eb

mmlb force-pushed the drive branch from da21321 to ff770eb Compare June 11, 2024 14:21

mmlb mentioned this pull request Jun 12, 2024

Add initial disk wipe support to vogelkop metal-toolbox/vogelkop#84

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Drive type and Wiper interface #160

Add a Drive type and Wiper interface #160

mmlb commented Jun 3, 2024 •

edited

Loading

DoctorVin left a comment

DoctorVin Jun 3, 2024

mmlb Jun 3, 2024

joelrebel Jun 4, 2024 •

edited

Loading

mmlb Jun 4, 2024

DoctorVin Jun 3, 2024

mmlb Jun 3, 2024

DoctorVin Jun 3, 2024

mmlb Jun 3, 2024

mmlb Jun 3, 2024

joelrebel left a comment •

edited

Loading

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

mmlb Jun 4, 2024

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

mmlb Jun 4, 2024

joelrebel Jun 4, 2024 •

edited

Loading

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

joelrebel Jun 4, 2024

mmlb Jun 4, 2024

mmlb Jun 4, 2024

mmlb commented Jun 4, 2024

mmlb commented Jun 5, 2024

mmlb commented Jun 6, 2024

Add a Drive type and Wiper interface #160

Are you sure you want to change the base?

Add a Drive type and Wiper interface #160

Conversation

mmlb commented Jun 3, 2024 • edited Loading

What does this PR do

Removes DiskWiper from StorageControllerAction

Some light refactoring of model

Creates actions/wipe package with a small interface'

Add model/drive package with Drive type and WipersGetter interface

Stores LogicalName in drives

Store nvme discovered capabilities in Drive

Finally, make nvme a WipersGetter

DoctorVin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelrebel Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelrebel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelrebel Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb commented Jun 4, 2024

mmlb commented Jun 5, 2024

mmlb commented Jun 6, 2024

mmlb commented Jun 3, 2024 •

edited

Loading

joelrebel Jun 4, 2024 •

edited

Loading

joelrebel left a comment •

edited

Loading

joelrebel Jun 4, 2024 •

edited

Loading