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

Remove internal/external disk type from PhysicalDisk view #3496

Merged
merged 6 commits into from
Jul 6, 2023

Conversation

zephraph
Copy link
Contributor

@zephraph zephraph commented Jul 5, 2023

This PR follows up on oxidecomputer/console#1628. I started #3482 to clarify the field but it was decided there it's best just to take it out.

@zephraph zephraph marked this pull request as ready for review July 5, 2023 20:49
@zephraph zephraph requested review from smklein and david-crespo July 6, 2023 15:48
Comment on lines 341 to 344
PhysicalDiskKind::M2,
PhysicalDiskKind::U2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to a U2 b/c the M2 is filtered out in the results

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I find this a little confusing, an internal function called sled_list_physical_disks doesn't indicate that it would filter out M.2s.

If we're keeping that name: we should continue inserting this M.2 in the test, and keep returning it.
Otherwise, we should rename sled_list_physical_disks to sled_list_external_disks or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine w/ changing the name if that helps. We can't leave the M.2s in because sled_list_physical_disk pipes up to the app layer sled_list_physical_disk which maps to /v1/system/hardware/sleds/:sled/disks.

Copy link

Choose a reason for hiding this comment

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

Why wouldn't we just list both the U.2s and M.2s? The original reason we had confusion was that I think @ericaasen saw the wrong types here. This endpoint feels like it should list everything. We talked in #3482 about terminology challenges, but I don't see why wouldn't list all disks here.

@david-crespo david-crespo changed the title Remove internal/external disk types Remove internal/external disk type from PhysicalDisk view Jul 6, 2023
Comment on lines 129 to 130
let disks = physical_disks_list(&external_client, &disks_url).await;
assert_eq!(disks.len(), 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the part that verifies the filtering works

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Looks good. Only part that worries me is the potential for confusion based on the expectation that we would be listing everything in the DB rather than filtering for U2s. Short of changing the name of the struct, maybe a comment on the list endpoint would help? Or maybe changing the name of the Nexus function? I'm kind of flailing, but I feel like some name somewhere should give a hint that that filter is present in the query.

/// List physical disks
#[endpoint {
method = GET,
path = "/v1/system/hardware/disks",
tags = ["system/hardware"],
}]
async fn physical_disk_list(

@zephraph
Copy link
Contributor Author

zephraph commented Jul 6, 2023

Chatted offline with @rmustacc about the filtering. I'd slightly misunderstood the original issue so I've removed all the filtering. U.2s and M.2s will be listed but the type field is still out.

@david-crespo
Copy link
Contributor

Solves my problem!

@zephraph zephraph merged commit c3e856e into main Jul 6, 2023
@zephraph zephraph deleted the filter-physical-disks branch July 6, 2023 21:23
@smklein
Copy link
Collaborator

smklein commented Jul 7, 2023

Chatted offline with @rmustacc about the filtering. I'd slightly misunderstood the original issue so I've removed all the filtering. U.2s and M.2s will be listed but the type field is still out.

I'm unclear on this -- how is it less confusing to expose both U.2s and M.2s, but make them totally indistinguishable from one another?

@smklein
Copy link
Collaborator

smklein commented Jul 7, 2023

Follow-up from some out-of-band discussions:

  • Exposing the info "external" vs "internal" is too opinionated, and not a sufficient replacement of true topology information
  • However, exposing the form factor of the device -- u.2 vs m.2 -- is a "fact". We'd like to expose more topology info when we can, but providing this info will be helpful when operating on disks in the short-term, when we have a single gimlet layout.

Therefore: We should probably still report whether or not disks are u.2s or m.2s, and we can provide more topology information later too.

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.

4 participants