Instance network interfaces don't need to be paginated (#1009) by ryaeng · Pull Request #1164 · oxidecomputer/omicron

ryaeng · 2022-06-06T21:39:56Z

Removed page params from entrypoint, app, and db calls.

…#1009) Removed page params from entrypoint, app, and db calls.

david-crespo · 2022-06-07T14:33:49Z

+        dsl::network_interface
            .filter(dsl::time_deleted.is_null())
-            .filter(dsl::instance_id.eq(authz_instance.id()))
+            .filter(dsl::vpc_id.eq(authz_instance.id()))


It seems you're checking if the vpc_id equals the instance ID — that can't be right.

@david-crespo What's a good way to test this?

I believe this would be caught by the existing integration tests. Specifically, this call here fetches the existing NICs attached to an instance. I can't see how that would return anything if the wrong UUID were used.

Can you show the output of your tests against this commit?

If you look at CI on this commit, the tests passed even when that bug was in place

c563bfe

david-crespo · 2022-06-07T14:38:13Z

Thank you for your contribution. I have been meaning to make a similar change at some point.

ryaeng · 2022-06-07T17:41:17Z

I'll have these fixed momentarily.

What's the best way to amend this PR with the changes?

bnaecker · 2022-06-07T17:42:44Z

@ryaeng Pushing a new commit is fine. It'll all be squashed at merge time.

ryaeng · 2022-06-07T17:45:51Z

@ryaeng Pushing a new commit is fine. It'll all be squashed at merge time.

Awesome! Thanks.

ryaeng · 2022-06-07T18:34:59Z

Done

david-crespo · 2022-06-09T17:09:42Z

This is looking pretty good. The reason I haven't merged is I'm mulling over whether the endpoint should still return ResultsPage<NetworkInterface>, which has the items key with the items and a next_page key will which always be null. If you look at firewall rules, which are similarly not paginated

omicron/nexus/src/external_api/http_entrypoints.rs

Lines 2458 to 2461 in 215491b

    
           async fn vpc_firewall_rules_get( 
        
               rqctx: Arc<RequestContext<Arc<ServerContext>>>, 
        
               path_params: Path<VpcPathParam>, 
        
           ) -> Result<HttpResponseOk<VpcFirewallRules>, HttpError> {

we return a dedicated struct that just has the rules on it:

omicron/common/src/api/external/mod.rs

Lines 1409 to 1413 in 215491b

    
           /// Collection of a [`Vpc`]'s firewall rules 
        
           #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] 
        
           pub struct VpcFirewallRules { 
        
               pub rules: Vec<VpcFirewallRule>, 
        
           }

That is probably what we should do here too, though I would prefer it live in nexus/src/external_api/views.rs rather than omicron_common spot.

just-be-dev · 2022-06-09T21:18:04Z

ResultsPage should be used only for paginated endpoints. Not doing so introduces ambiguity into the API that'd be a potential point of confusion for users.

ryaeng · 2022-06-09T21:59:19Z

I'll make this happen tomorrow, fellas.

- Modify instance_network_interfaces_get to return NetworkInterfaces

ryaeng · 2022-06-13T15:31:38Z

Got this done. Spent a couple of hours working on it before I realized I was comparing it to subnets and not firewall rules 🤦‍♂️.

What is the rule for creating tests? Trying to determine whether this change needs a test or not. Seems rather trivial so I didn't include one but I can fix that if necessary.

ryaeng · 2022-06-13T15:32:12Z

Seems there's a conflict. Lemme pull that in real quick and then I'll perform another push.

…instance-network-interfaces

david-crespo · 2022-06-13T16:32:41Z

Ok, this is good! Thank you!

Re: tests, I would have said no additional test is required beyond what we already have for basic functionality, but the vpc_id == instance_id mistake was not caught by any existing test. So if you want to add one that would have caught that, that would be great.

Add test to list interfaces attached to an instance

- Create test for instance_network_interfaces_get - Replace object_get with helper function across tests - Fix typos

ryaeng · 2022-06-23T14:25:33Z

@david-crespo Came back around to this one. Should be up to par. Thanks.

david-crespo · 2022-08-23T20:00:13Z

Thanks for your work on this @ryaeng, but in #1495 we decided that even an endpoint with a capped-length result set should still return the paginated interface for consistency (and flexibility, in case we paginate later). So I'm going to close this and leave the endpoint as-is.

Crucible changes: Per client, queue-based backpressure (#1186) A builder for the Downstairs Downstairs struct. (#1152) Update Rust to v1.76.0 (#1153) Deactivate the read only parent after a scrub (#1180) Start byte-based backpressure earlier (#1179) Tweak CI scripts to fix warnings (#1178) Make `gw_ds_complete` less public (#1175) Verify extent under repair is valid after copying files (#1159) Remove individual panic setup, use global panic settings (#1174) [smf] Use new zone network config service (#1096) Move a few methods into downstairs (#1160) Remove extra clone in upstairs read (#1163) Make `crucible-downstairs` not depend on upstairs (#1165) Update Rust crate rusqlite to 0.31 (#1171) Update Rust crate reedline to 0.29.0 (#1170) Update Rust crate clap to 4.5 (#1169) Update Rust crate indicatif to 0.17.8 (#1168) Update progenitor to bc0bb4b (#1164) Do not 500 on snapshot delete for deleted region (#1162) Drop jobs from Offline downstairs. (#1157) `Mutex<Work>` → `Work` (#1156) Added a contributing.md (#1158) Remove ExtentFlushClose::source_downstairs (#1154) Remove unnecessary mutexes from Downstairs (#1132) Propolis changes: PHD: improve Windows reliability (#651) Update progenitor and omicron deps Clean up VMM resource on server shutdown Remove Inventory mechanism Update vergen dependency Properly handle pre/post illumos#16183 fixups PHD: add `pfexec` to xtask phd-runner invocation (#647) PHD: add Windows Server 2016 adapter & improve WS2016/2019 reliability (#646) PHD: use `clap` for more `cargo xtask phd` args (#645) PHD: several `cargo xtask phd` CLI fixes (#642) PHD: Use ZFS clones for file-backed disks (#640) PHD: improve ctrl-c handling (#634)

Crucible changes: Per client, queue-based backpressure (#1186) A builder for the Downstairs Downstairs struct. (#1152) Update Rust to v1.76.0 (#1153) Deactivate the read only parent after a scrub (#1180) Start byte-based backpressure earlier (#1179) Tweak CI scripts to fix warnings (#1178) Make `gw_ds_complete` less public (#1175) Verify extent under repair is valid after copying files (#1159) Remove individual panic setup, use global panic settings (#1174) [smf] Use new zone network config service (#1096) Move a few methods into downstairs (#1160) Remove extra clone in upstairs read (#1163) Make `crucible-downstairs` not depend on upstairs (#1165) Update Rust crate rusqlite to 0.31 (#1171) Update Rust crate reedline to 0.29.0 (#1170) Update Rust crate clap to 4.5 (#1169) Update Rust crate indicatif to 0.17.8 (#1168) Update progenitor to bc0bb4b (#1164) Do not 500 on snapshot delete for deleted region (#1162) Drop jobs from Offline downstairs. (#1157) `Mutex<Work>` → `Work` (#1156) Added a contributing.md (#1158) Remove ExtentFlushClose::source_downstairs (#1154) Remove unnecessary mutexes from Downstairs (#1132) Propolis changes: PHD: improve Windows reliability (#651) Update progenitor and omicron deps Clean up VMM resource on server shutdown Remove Inventory mechanism Update vergen dependency Properly handle pre/post illumos#16183 fixups PHD: add `pfexec` to xtask phd-runner invocation (#647) PHD: add Windows Server 2016 adapter & improve WS2016/2019 reliability (#646) PHD: use `clap` for more `cargo xtask phd` args (#645) PHD: several `cargo xtask phd` CLI fixes (#642) PHD: Use ZFS clones for file-backed disks (#640) PHD: improve ctrl-c handling (#634) Co-authored-by: Alan Hanson <alan@oxide.computer>

Instance network interfaces don't need to be paginated (oxidecomputer…

c563bfe

…#1009) Removed page params from entrypoint, app, and db calls.

smklein requested a review from david-crespo June 7, 2022 13:43

david-crespo reviewed Jun 7, 2022

View reviewed changes

bnaecker reviewed Jun 7, 2022

View reviewed changes

Comment thread nexus/src/db/datastore.rs Outdated

Sort interfaces by slot in ascending order.

42ef4b8

- Create NetworkInterfaces view

99c01c5

- Modify instance_network_interfaces_get to return NetworkInterfaces

Merge remote-tracking branch 'upstream/main' into remove-page-params-…

363feb9

…instance-network-interfaces

ryaeng added 3 commits June 22, 2022 10:43

Solve merge conflict

a7d8d13

Add test to list interfaces attached to an instance

Merge branch 'main' into remove-page-params-instance-network-interfaces

15b12b0

- Create helper function to get network interfaces

cb7afca

- Create test for instance_network_interfaces_get - Replace object_get with helper function across tests - Fix typos

david-crespo mentioned this pull request Jul 25, 2022

External IPs endpoint returns struct with items Vec #1495

Merged

david-crespo closed this Aug 23, 2022

karencfv mentioned this pull request Feb 20, 2024

Create composite crucible and pantry packages #4927

Merged

leftwo mentioned this pull request Mar 4, 2024

Update Propolis and Crucible to latest #5187

Merged

Conversation

ryaeng commented Jun 6, 2022

Uh oh!

david-crespo Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

ryaeng Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

bnaecker Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Jun 7, 2022

Uh oh!

Uh oh!

ryaeng commented Jun 7, 2022

Uh oh!

bnaecker commented Jun 7, 2022

Uh oh!

ryaeng commented Jun 7, 2022

Uh oh!

ryaeng commented Jun 7, 2022

Uh oh!

david-crespo commented Jun 9, 2022

Uh oh!

just-be-dev commented Jun 9, 2022

Uh oh!

ryaeng commented Jun 9, 2022

Uh oh!

ryaeng commented Jun 13, 2022

Uh oh!

ryaeng commented Jun 13, 2022

Uh oh!

david-crespo commented Jun 13, 2022

Uh oh!

ryaeng commented Jun 23, 2022

Uh oh!

david-crespo commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bnaecker Jun 15, 2022 •

edited

Loading