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

Rename Disk stuff -> Drive and teach diskwipe example more tricks #163

Merged

Conversation

mmlb
Copy link
Member

@mmlb mmlb commented Jun 10, 2024

What does this PR do

The main reason for this PR is to make the diskwipe example a better example of both using ironlib to detect hardware presence and capabilities and using the information to influence actions to be taken. After collecting all the hardware the new example code detects nvme vs sata and detects the features supported by the disk all to decide which is the best tool to wipe the drive.

Along the way many refactors stood out as necessary. The main one was blkdiscard did not implement the DiskWiper interface. I also noticed we were using Device when Drive already existed and was more appropriate and that the args and log lines could do with some homogeneity so that they don't appear to be originating from different repositories.

Further breakdown of the changes are shown below:

Make blkdiscard look more like other DiskWipers & utilities

I missed this when doing review on the original PR, so doing it now.

Change wipe examples to all take the same cli args

Best to make the examples all actually usable and there's no point to have different arguments so lets make them all similar.

Rename Disk, device -> Drive, drive

I don't want to have 2 names for the same thing and even worse reuse a name that means something completely different in a different context. bmc-toolbox/common already has Drive and Device so lets use Drive and drive for disks and device for Devices.

Make all WipeDrive methods logs look similar

Similar to the cli arg homogeneity argument a few commits ago, lets do the same thing for log messages in WipeDrive implementations. This makes it so every line has the drive name, has the wipe method under the same key and has similar start/error messages.

Change WipeDrive to take a common.Drive instead of string

We use a statically typed language so why devolve down to strings when we also have a better type lying around, so lets use it.

Get rid of Nvme.wipe

Now that Nvme.WipeDrive takes a common.Drive that has the capabilities saved, WipeDrive was just a thin wrapper around wipe. Hoist the code for wipe into WipeDrive then.

Better tests in fill_zero_test

Use stretchr/testify in fill_zero_test for easier to read tests. Actually test different sized files in fill_zero_test.Test_WipeDrive, previously there was a bug and all files were 4096 bytes long. The test were also updated to actually verify that FillZero actually fills the file with 0s, before it was just ensuring the file was not shrunk or grown.

Make Blkdiscard.WipeDrive & FillZeroCmd.WipeDrive verify the wipe

Nvme wipe has multiple sub-methods for wiping and it verifies the method worked correctly or tries the next one. FillZeroCmd nor Blkdiscard did not do this. I was on the fence if we should change Nvme or FillZero and Blkdiscard and chose to stay with the way Nvme does it because ultimately, I want callers to not have to worry about this. If a WipeDrive method returns success then the wipe is good.

Make diskwipe a better example

I thought it would be nice to have a diskwipe example that could do a better job of using ironlib's hardware collection capabilities vs just statically using a tool, it serves as a better example of using ironlib vs just being a go wrapper for various unix cli utilities.

So I got rid of the separate blkdiscard example and wipe code from nvme-wipe (so it just does nvme namespace clearing/creating so I renamed it to nvme-ns-wipe) and taught diskwipe to know when to use each tool.

The HW vendor this change applies to (if applicable)

N.A.

The HW model number, product name this change applies to (if applicable)

N.A.

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

N.A.

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

N.A.

How can this change be tested by a PR reviewer?

These are mostly refactors of code but can mess around with the examples/diskwipe and examples/nvme-ns-wipe binaries.

Description for changelog/release notes

Reword changelog/release notes for DiskWiper/WipeDisk -> DriveWiper/WipeDrive. Reword changelog/release notes regarding the diskwipe example and how it uses the best tool for the job.

@mmlb mmlb force-pushed the rename-DiskWiper-and-other-wipe-refactors branch from b80f648 to 4b0de0e Compare June 10, 2024 15:58
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 believe the net number of new comments in this commit is 0, or very close to it, after 15 commits (also: not sure why there are 15 commits - 'git rebase -i' is your friend). Assuming you have an intention for this code, could you please add some comments explaining what that intention is?

I can't test code when I don't know what that code is intended to accomplish, and I can't trust your proposed tests (not now, and certainly not later when I need to understand them again) if they're not testing against intentions.

How would you unit test this behaviour on a server with a device in?

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.

(To add: your PR commentary is good. But PR commentary is not where I am going to find what I need to know when I look at this code in the future. Code comments, code docs, design docs - good places for anything that needs to persist.)

Copy link
Contributor

@DoctorVin DoctorVin left a comment

Choose a reason for hiding this comment

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

Doesn't seem too controversial.

utils/blkdiscard_test.go Outdated Show resolved Hide resolved
utils/fill_zero_test.go Outdated Show resolved Hide resolved
utils/nvme.go Outdated Show resolved Hide resolved
@mmlb
Copy link
Member Author

mmlb commented Jun 10, 2024

@iawells thanks for looking, responses inline:

I believe the net number of new comments in this commit is 0, or very close to it, after 15 commits (also: not sure why there are 15 commits - 'git rebase -i' is your friend).

Git rebase is one of my best friends indeed. I've got 15 commits in this PR so I can avoid large commits doing multiple changes. Each commit diff and message should be able to stand on its own merit, if not let me know and I'll take a look.

Assuming you have an intention for this code, could you please add some comments explaining what that intention is?

Will add to the PR body. The main intention was to get better example in the code base to showcase usage of ironlib, everything else are simple refactors, api fixes and test fixes.

I can't test code when I don't know what that code is intended to accomplish, and I can't trust your proposed tests (not now, and certainly not later when I need to understand them again) if they're not testing against intentions.

Answered in the PR template no?

How can this change be tested by a PR reviewer?

These are mostly refactors of code but can mess around with the examples/diskwipe and examples/nvme-ns-wipe binaries.

How would you unit test this behaviour on a server with a device in?

The unit tests have not been written to require a machine with the required hardware, if they did they would also require root and would be dangerous to run (potential for deletion of real data).

mmlb added 7 commits June 10, 2024 16:53
Should have been a DiskWiper from the beginning and its going to be
useful soon.
Just like other utilities.
Drive is already a well known/used name for disk in this repo and
bmc-toolbox so lets go with that instead of mixing Drive and Disk. Its
unfortunate that we have Disk in RAID stuff but thats also an issue in
bmc-toolbox/common. Changing would need to wait for major releases.

Same deal with device -> drive, device looks like it would be a
common.Device which is something totally different.
So users reading the logs can have an easier time parsing the lines if
they look alike.
We have a better type available to us so lets do better and use it.
Obviously we can still have non-sense passed in but the caller will have
to work harder at it.
Since WipeDrive has the same signature as wipe and now does nothing more
than wrap it there is no need for the separation anymore.
@mmlb mmlb force-pushed the rename-DiskWiper-and-other-wipe-refactors branch from b80f648 to ad840fe Compare June 10, 2024 20:53
mmlb added 7 commits June 10, 2024 17:16
Helper funcs make for easier to read tests.
Previous test wasn't really testing the file being filled with zeros,
just that it doesn't change size. Now we write random data to the file,
verify the contents to *not* be just zeros, proceed with writing zeros
and the finally verify the new contents is just zeros. Along the way
make sure the file also hasn't changed size.
This is in line with what Nvme.WipeDrive already does. Nvme has to do it
that way because it needs to verify the method tried actually worked or
fallback to another one. No point in returning no-error to the caller if
the disk wasn't actually wiped.

Had to bump up the file sizes in Test_FillZeroWipeDrive because
ApplyWatermarks needs more than 4KiB worth of space to write all the
watermarks (512 * 10 == 5120).
By using collector.GetInventory to be able to inspect the drives for
appropriate wiper.

If we end up using FillZeroCmd then the timeout is increased if it's
still the default.
Better off in diskwipe than solo.
@mmlb mmlb force-pushed the rename-DiskWiper-and-other-wipe-refactors branch from 1b9927d to 317897a Compare June 10, 2024 21:16
@mmlb mmlb requested review from DoctorVin and iawells June 10, 2024 21:18
utils/fill_zero_test.go Show resolved Hide resolved
@mmlb mmlb merged commit 3514176 into metal-toolbox:main Jun 11, 2024
5 checks passed
@mmlb mmlb deleted the rename-DiskWiper-and-other-wipe-refactors branch June 11, 2024 13:35
mmlb referenced this pull request in metal-toolbox/vogelkop Jun 14, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/metal-toolbox/ironlib](https://togithub.com/metal-toolbox/ironlib)
| `v0.2.18-0.20240611133518-3514176030a4` -> `v0.2.18` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fmetal-toolbox%2fironlib/v0.2.18?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fmetal-toolbox%2fironlib/v0.2.18?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fmetal-toolbox%2fironlib/v0.2.18-0.20240611133518-3514176030a4/v0.2.18?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fmetal-toolbox%2fironlib/v0.2.18-0.20240611133518-3514176030a4/v0.2.18?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>metal-toolbox/ironlib
(github.com/metal-toolbox/ironlib)</summary>

###
[`v0.2.18`](https://togithub.com/metal-toolbox/ironlib/releases/tag/v0.2.18)

[Compare
Source](https://togithub.com/metal-toolbox/ironlib/compare/v0.2.17...v0.2.18)

#### What's Changed

- Vc/instrument firmware by
[@&#8203;DoctorVin](https://togithub.com/DoctorVin) in
[https://github.com/metal-toolbox/ironlib/pull/123](https://togithub.com/metal-toolbox/ironlib/pull/123)
- Dockerfile: add support to build non-dist image by
[@&#8203;joelrebel](https://togithub.com/joelrebel) in
[https://github.com/metal-toolbox/ironlib/pull/124](https://togithub.com/metal-toolbox/ironlib/pull/124)
- Update actions/checkout action to v4 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/112](https://togithub.com/metal-toolbox/ironlib/pull/112)
- House/spring cleaning by [@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/128](https://togithub.com/metal-toolbox/ironlib/pull/128)
- ironlib is able to fill a disk with all zeros by
[@&#8203;turegano-equinix](https://togithub.com/turegano-equinix) in
[https://github.com/metal-toolbox/ironlib/pull/131](https://togithub.com/metal-toolbox/ironlib/pull/131)
- ironlib is able to detect ineffective wipes by
[@&#8203;turegano-equinix](https://togithub.com/turegano-equinix) in
[https://github.com/metal-toolbox/ironlib/pull/135](https://togithub.com/metal-toolbox/ironlib/pull/135)
- House Cleaning Part 2 - Electric Boogaloo! by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/133](https://togithub.com/metal-toolbox/ironlib/pull/133)
- Better nvme capability detection by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/129](https://togithub.com/metal-toolbox/ironlib/pull/129)
- Quiet down tests by [@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/137](https://togithub.com/metal-toolbox/ironlib/pull/137)
- More refactors, fixes, etc by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/141](https://togithub.com/metal-toolbox/ironlib/pull/141)
- chore(deps): update docker/login-action action to v3 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/118](https://togithub.com/metal-toolbox/ironlib/pull/118)
- chore(deps): update docker/build-push-action action to v5 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/113](https://togithub.com/metal-toolbox/ironlib/pull/113)
- fix(deps): update module github.com/r3labs/diff/v2 to v3 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/108](https://togithub.com/metal-toolbox/ironlib/pull/108)
- chore(deps): update docker/metadata-action action to v5 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/144](https://togithub.com/metal-toolbox/ironlib/pull/144)
- chore(deps): update actions/setup-go action to v5 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/143](https://togithub.com/metal-toolbox/ironlib/pull/143)
- chore(deps): update module google.golang.org/protobuf to v1.33.0
\[security] by [@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/126](https://togithub.com/metal-toolbox/ironlib/pull/126)
- fix(deps): update module golang.org/x/net to v0.23.0 \[security] by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/116](https://togithub.com/metal-toolbox/ironlib/pull/116)
- chore(deps): update docker/setup-buildx-action action to v3 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/146](https://togithub.com/metal-toolbox/ironlib/pull/146)
- fix(deps): update module github.com/sirupsen/logrus to v1.9.3 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/100](https://togithub.com/metal-toolbox/ironlib/pull/100)
- chore(deps): update anchore/sbom-action action to v0.15.11 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/99](https://togithub.com/metal-toolbox/ironlib/pull/99)
- fix(deps): update module github.com/tidwall/gjson to v1.17.1 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/109](https://togithub.com/metal-toolbox/ironlib/pull/109)
- fix(deps): update module golang.org/x/net to v0.24.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/145](https://togithub.com/metal-toolbox/ironlib/pull/145)
- chore(deps): update golangci/golangci-lint-action action to v5 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/147](https://togithub.com/metal-toolbox/ironlib/pull/147)
- fix(deps): update module github.com/stretchr/testify to v1.9.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/101](https://togithub.com/metal-toolbox/ironlib/pull/101)
- fix(deps): update module github.com/beevik/etree to v1.3.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/97](https://togithub.com/metal-toolbox/ironlib/pull/97)
- fix(deps): update module golang.org/x/net to v0.25.0 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/148](https://togithub.com/metal-toolbox/ironlib/pull/148)
- chore(deps): update github/codeql-action action to v3 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/149](https://togithub.com/metal-toolbox/ironlib/pull/149)
- Update CODEOWNERS by
[@&#8203;DoctorVin](https://togithub.com/DoctorVin) in
[https://github.com/metal-toolbox/ironlib/pull/152](https://togithub.com/metal-toolbox/ironlib/pull/152)
- Update modules that renovate is having trouble with by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/150](https://togithub.com/metal-toolbox/ironlib/pull/150)
- chore(deps): update golangci/golangci-lint-action action to v6 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/metal-toolbox/ironlib/pull/153](https://togithub.com/metal-toolbox/ironlib/pull/153)
- Add DiskWipe support to nvme using sanitize by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/136](https://togithub.com/metal-toolbox/ironlib/pull/136)
- Add format support to nvme WipeDisk by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/142](https://togithub.com/metal-toolbox/ironlib/pull/142)
- Add ns delete/create support to nvme WipeDisk by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/154](https://togithub.com/metal-toolbox/ironlib/pull/154)
- More clean ups by [@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/158](https://togithub.com/metal-toolbox/ironlib/pull/158)
- Add basic support for blkdiscard by
[@&#8203;ScottGarman](https://togithub.com/ScottGarman) in
[https://github.com/metal-toolbox/ironlib/pull/159](https://togithub.com/metal-toolbox/ironlib/pull/159)
- Some more clean ups by [@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/161](https://togithub.com/metal-toolbox/ironlib/pull/161)
- Rename Disk stuff -> Drive and teach diskwipe example more tricks by
[@&#8203;mmlb](https://togithub.com/mmlb) in
[https://github.com/metal-toolbox/ironlib/pull/163](https://togithub.com/metal-toolbox/ironlib/pull/163)

#### New Contributors

- [@&#8203;turegano-equinix](https://togithub.com/turegano-equinix) made
their first contribution in
[https://github.com/metal-toolbox/ironlib/pull/131](https://togithub.com/metal-toolbox/ironlib/pull/131)
- [@&#8203;ScottGarman](https://togithub.com/ScottGarman) made their
first contribution in
[https://github.com/metal-toolbox/ironlib/pull/159](https://togithub.com/metal-toolbox/ironlib/pull/159)

**Full Changelog**:
metal-toolbox/ironlib@v0.2.17...v0.2.18

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/metal-toolbox/vogelkop).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
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