Skip to content

Conversation

@david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Jan 7, 2022

Noticed this while working with VPC subnets, then found a few others.

Left out NetworkInterface because it's used by sled agent as a model rather than a view, so changing it causes tests to fail in mysterious ways that I think are 400s when Nexus calls Sled Agent.

/// Describes the instance hardware.
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
pub struct InstanceHardware {
pub runtime: internal::nexus::InstanceRuntimeState,
pub nics: Vec<external::NetworkInterface>,
}

@david-crespo david-crespo changed the title Add #[serde(flatten)] to some views that were missing it Add #[serde(flatten)] to identity on some views that were missing it Jan 7, 2022
@david-crespo david-crespo marked this pull request as ready for review January 10, 2022 02:44
@david-crespo david-crespo requested review from smklein and zephraph and removed request for smklein January 10, 2022 19:34
@zephraph
Copy link
Contributor

Ah, nice. I didn't realize that was a thing (or understand what it really did before this).

@david-crespo david-crespo merged commit e8c31ef into main Jan 10, 2022
@david-crespo david-crespo deleted the flatten-identity branch January 10, 2022 19:40
@smklein
Copy link
Collaborator

smklein commented Jan 10, 2022

I'm curious what "fail in mysterious ways" means, but this LGTM too.

@david-crespo
Copy link
Contributor Author

david-crespo commented Jan 10, 2022

Oh yeah, some detail would be nice. When I make the change on NetworkInterface and then run this test

async fn test_subnet_allocation(cptestctx: &ControlPlaneTestContext) {

I get a not very helpful 400 when attempting to create an instance on this line:

create_instance(client, &url_instances, "i1").await;

Checking the log turns up this:

{
   "msg":"request completed",
   "v":0,
   "name":"test_subnet_allocation",
   "level":30,
   "time":"2022-01-10T13:45:36.622900-06:00",
   "hostname":"Davids-M1-MBP.local",
   "pid":6698,
   "uri":"/instances/4092b2ce-c0b5-42ea-b23f-1e95e4890c93",
   "method":"PUT",
   "req_id":"685b0721-8544-4563-9702-965b574ea416",
   "remote_addr":"127.0.0.1:50988",
   "local_addr":"127.0.0.1:50977",
   "component":"dropshot",
   "sled_id":"b6d65341-167c-41df-9b5c-41cded99c229",
   "component":"omicron_sled_agent::sim::Server",
   "error_message_external":"unable to parse body: missing field `id` at line 1 column 460",
   "error_message_internal":"unable to parse body: missing field `id` at line 1 column 460",
   "response_code":"400"
}

If this were only a question of the generated clients, this would be strange, because the generated clients should be updated in accordance with the change to NetworkInterface. That's why I think it has to do with one of the following, which are direct uses of the NetworkInterface struct as an internal data structure, bypassing the generated clients.

struct InstanceInner {
id: Uuid,
log: Logger,
// Properties visible to Propolis
properties: propolis_client::api::InstanceProperties,
// NIC-related properties
nic_id_allocator: IdAllocator,
requested_nics: Vec<NetworkInterface>,

/// Describes the instance hardware.
#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
pub struct InstanceHardware {
pub runtime: internal::nexus::InstanceRuntimeState,
pub nics: Vec<external::NetworkInterface>,
}


Edit: Oh yeah, it's probably the InstanceHardware one:

#[endpoint {
method = PUT,
path = "/instances/{instance_id}",
}]
async fn instance_put(
rqctx: Arc<RequestContext<Arc<SledAgent>>>,
path_params: Path<InstancePathParam>,
body: TypedBody<InstanceEnsureBody>,
) -> Result<HttpResponseOk<InstanceRuntimeState>, HttpError> {

/// Sent to a sled agent to establish the runtime state of an Instance
#[derive(Serialize, Deserialize, JsonSchema)]
pub struct InstanceEnsureBody {
/// Last runtime state of the Instance known to Nexus (used if the agent
/// has never seen this Instance before).
pub initial: InstanceHardware,
/// requested runtime state of the Instance
pub target: InstanceRuntimeStateRequested,
}

leftwo pushed a commit that referenced this pull request Dec 9, 2023
Crucible
Start queue backpressure earlier (#1047)

Propolis
Fix no-deps option for clippy xtask
nvme: don't fail on abort cmd (#581)
Update openssl and rustix deps
Add xtask for pre-push checks
Do not require casting for API version cmp
better softnpu management command reliability (#570)
Log when pause futures complete (#575)
leftwo added a commit that referenced this pull request Dec 10, 2023
Crucible
Start queue backpressure earlier (#1047)

Propolis
Fix no-deps option for clippy xtask
nvme: don't fail on abort cmd (#581)
Update openssl and rustix deps
Add xtask for pre-push checks
Do not require casting for API version cmp
better softnpu management command reliability (#570) Log when pause
futures complete (#575)

Co-authored-by: Alan Hanson <[email protected]>
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