[nexus] Expose names with affinity group memberships#7658
Conversation
| .bind::<diesel::sql_types::Uuid, _>(authz_affinity_group.id()) | ||
| .sql(") "); | ||
|
|
||
| let (direction, limit) = match pagparams { |
There was a problem hiding this comment.
I'm doing pagination manually in this PR, and I'm refactoring it in #7851.
My goal was to have sufficient test coverage here that the refactor is basically "just clean up"
| pub fn determine_effective_state_inner( | ||
| instance_state: InstanceState, | ||
| migration_id: Option<Uuid>, | ||
| vmm_state: Option<VmmState>, | ||
| ) -> external::InstanceState { | ||
| use crate::db::model::InstanceState; |
There was a problem hiding this comment.
There's a small part of me that thinks that what we really want here is something like this:
struct InstanceStateComputer<'s> {
instance_state: &'s InstanceState,
migration_id: Option<&'s MigrationId>,
vmm_state: Option<&'s VmmState>,
}
impl InstanceStateComputer<'_> {
fn compute_state_from(
instance_state: InstanceState,
migration_id: MigrationId,
vmm_state: VmmState
) -> external::InstanceState {
// take references to params, create a Self, call compute_state
}
fn compute_state(&self) -> external::InstanceState {
// copy from determine_effective_state_inner
}
}
impl<'s> From<&'s InstanceAndVmmState> for InstanceStateComputer<'s> {
// take refs to the fields in the original state structure, using as_ref/map as needed
}I dunno. This may just be a very complicated way of saying "determine_effective_state_inner really should be a free function and not an associated function of InstanceAndVmmState", though it will require less surgery at the call sites (InstanceAndVmmState::determine_effective_state just calls From to get a state computer and then calls compute_state).
There was a problem hiding this comment.
I'm happy to pull this out, and not be an associated function, but it's not really convenient to call that From method as you have things written unless you want me to change the function signature of determine_effective_state.
determine_effective_state acts on a &Instance, so I can't make an InstanceAndActiveVmm object (which owns the instance) unless:
- I clone the instance
- I change the lifetime of the
InstanceAndActiveVmmobject
I tried to implement this in 4e95b23 - I'm calling InstanceStateComputer::from in the implementation of InstanceAndActiveVmm::effective_state, but that doesn't seem like a big win over it calling Self::determine_effective_state, like it did before
There was a problem hiding this comment.
I overlooked an important detail here, namely that determine_effective_state also doesn't have a receiver. I think if we were going to go the InstanceStateComputer route, I would also delete that routine and have its remaining direct callers (there are only two) convert to the state computer struct.
I think what I'd say is that:
- Having a receiver-less associated function on
InstanceAndVmmStatethat does the actual computation from just the instance/VMM state fields of interest seems at least as good as theInstanceStateComputerapproach. - But I wouldn't have more than one. If we don't do the separate struct, I would probably try to change the signature of
determine_effective_stateinto (what was) the signature ofdetermine_effective_state_inner, so that there's only one such associated function and not two.
I don't want to bikeshed too much, so I could go either way on having the struct vs. not, but I would like to get down to one callee in InstanceAndVmmState::effective_state's callee chain if it's not too much of a headache. (I think it's tractable if you have Instance and Vmm records from the DB, since all their fields are pub, but my Omicron clone is currently a shambles so I haven't tried it myself.)
Exposes names of affinity group members, as well as the state for instances.
Since names are being extracted here, this PR also implements pagination by member name.
Fixes #7625