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

[nexus] Expose names with affinity group memberships #7658

Merged
merged 207 commits into from
Mar 24, 2025
Merged

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Feb 27, 2025

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

@smklein smklein requested review from hawkw and gjcolombo March 3, 2025 20:35
@smklein smklein changed the base branch from aa-group-of-group to vmm-reduce-contention March 18, 2025 00:22
@smklein smklein changed the title (10/5) [Nexus] Expose names with affinity group memberships [nexus] Expose names with affinity group memberships Mar 18, 2025
Base automatically changed from vmm-reduce-contention to main March 20, 2025 16:40
.bind::<diesel::sql_types::Uuid, _>(authz_affinity_group.id())
.sql(") ");

let (direction, limit) = match pagparams {
Copy link
Collaborator Author

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"

Comment on lines 128 to 133
pub fn determine_effective_state_inner(
instance_state: InstanceState,
migration_id: Option<Uuid>,
vmm_state: Option<VmmState>,
) -> external::InstanceState {
use crate::db::model::InstanceState;
Copy link
Contributor

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).

Copy link
Collaborator Author

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:

  1. I clone the instance
  2. 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

Copy link
Contributor

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 the InstanceStateComputer 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 of determine_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.)

Copy link
Collaborator Author

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

@smklein smklein merged commit 36430ee into main Mar 24, 2025
17 checks passed
@smklein smklein deleted the affinity-expose-name branch March 24, 2025 23:14
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.

Affinity group members could expose name information when being listed
4 participants