-
Notifications
You must be signed in to change notification settings - Fork 42
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
InstanceAndActiveVmm
object
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
InstanceAndVmmState
that does the actual computation from just the instance/VMM state fields of interest seems at least as good as theInstanceStateComputer
approach. - 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_state
into (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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I gave this a swing in 72d0e9c
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