NAS-117805 / 22.12 / Adding warnings for unused disks with exported pools #7078
Conversation
bugclerk
changed the title
Codecov Report
@@ Coverage Diff @@
## master #7078 +/- ##
==========================================
+ Coverage 41.24% 41.30% +0.06%
==========================================
Files 1049 1051 +2
Lines 46107 46183 +76
Branches 6406 6415 +9
==========================================
+ Hits 19017 19078 +61
- Misses 27090 27105 +15
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -72,4 +72,6 @@ export default { | |||
| err_title: T('Error Extending Vdev'), | |||
| err_msg: T('Could not extend Vdev.'), | |||
| }, | |||
| exported_pool_warning: T('This disk is part of the exported pool {pool}. Adding this disk to a new or existing pool will make {pool} unable to import. You will lose any and all data in {pool}. Please make sure you have backed up any sensitive data in {pool} before reusing/repurposing this disk.'), | |||
There was a problem hiding this comment.
Can't we have it in one file only? If you really have to use separate files, you can make one file reference another.
| @@ -136,6 +136,7 @@ export interface UnusedDisk extends Disk { | |||
| partitions: { | |||
| path: string; | |||
| }[]; | |||
| exported_zpool?: string; | |||
There was a problem hiding this comment.
Is field really optional?
| this.selected.splice(0, this.selected.length); | ||
| this.selected.push(...selected); | ||
| } | ||
|
|
||
| handleWarningAboutExportedPoolDisks(selectedDisks: ManagerDisk[]): void { |
There was a problem hiding this comment.
Just because it's okay to write longer names, doesn't mean that we should write them longer on purpose :)
handleWarningAboutExportedPoolDisks => warnAboutExportedPool
showWarningAboutExportedPoolForDisk => can be inlined and handleWarningAboutExportedPoolDisks can be changed early to return if no warning is necessary.
shouldWarnAboutExportedPoolForLastSelectedDisk => shouldWarnAboutExportedPool or hasExportedPoolWarning
| } | ||
|
|
||
| updateDisksWithExportedPoolWarningFlags(selectedDisks: ManagerDisk[]): void { | ||
| const selectedDisksWithExportedPoolsAttached = selectedDisks.filter((selectedDisk) => selectedDisk.exported_zpool); |
There was a problem hiding this comment.
updateDisksWithExportedPoolWarningFlags => updateExportedWarningFlags or rememberExportedWarnings or markExportedWarning.
selectedDisksWithExportedPoolsAttached => disksWithPools there is enough context.
disksNamesWithExportedPoolsAlreadyWarnedFor => exportedPoolWarningsShown or even exportedWarningsShown
| @@ -27,7 +27,7 @@ | |||
| <ng-template #poolContainer> | |||
| <div fxFlex="100%" fxLayout="column"> | |||
| <!-- TODO: https://ixsystems.atlassian.net/browse/NAS-117820 --> | |||
| this.matDialog.open(DiskWipeDialogComponent, { | ||
| data: disk.name, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think it may be better to put this login into DiskWipeDialog and show a line somewhere there to avoid adding too many responsibilities to this component.
| })); | ||
| } | ||
|
|
||
| getExportedPoolNameForDiskIfApplicable(disk: Disk): string { |
There was a problem hiding this comment.
getPoolColumn with disk.pool being used here as well
| unusedDisks$ = this.ws.call('disk.get_unused').pipe( | ||
| tap((unusedDisks) => { | ||
| this.unusedDisks = unusedDisks; |
There was a problem hiding this comment.
This is an unexpected side-effect - unusedDisks$ now has two responsibilities.
You should remove both of these and a new loadUnusedDisks method in ngOnInit.
| ); | ||
| } | ||
|
|
||
| warnAboutExportedPoolForUnusedDiskIfNeeded = (diskIdentifier: string): void => { |
There was a problem hiding this comment.
Same notes about extra functions, long names and template strings.
| isFormLoading = false; | ||
|
|
||
| form = this.fb.group({ | ||
| dev: ['', Validators.required], | ||
| expand: [false], | ||
| }); | ||
|
|
||
| unusedDisks: UnusedDisk[] = []; | ||
|
|
||
| dev = { |
|
The check to not show warning is there to avoid showing warning for a disk that was already selected. If the disk is unchecked and then checked again, showing the warning again. |
This PR aims to add warnings for wherever UI is dealing with unused disks that are currently also part of exported pools. Test the following places in the UI.
Name of the exported pool that's attached to the disk mentioned in the table like so
Also, when you click the checkbox to select this disk, you should see the warning like this
Select/unselect a bunch of disks to check that the warning is only shown for the correct disks
Add to Poolbutton on the unused disks card, the model opens up. On the model, disks that are attached to exported pools should be separate from other disks of similar size and properties with a warning underneath as in the screenshot.ExtendandReplacebuttons forZFS Infocards on the newDevicespage andReplaceandAttachoperations on theBoot Pool Statuspage, you see a modal where you can select a disk from a drop down. Disks with exported pools should have the exported pools name mentioned in the select.Also, if you select such a disk/vdev from the drop down, you should see a warning.
Diskspage, you should see the name of the exported pool in thePoolcolumn of such disks with the text'(Exported)'appended.if you select to
Wipesuch a disk, you should see a confirmation dialog.