fix(api): Introduced VPC status property#320
Conversation
|
@yoks Maybe you can take a look. You also started working on getting the APIs standardized. |
| ( | ||
| "DPA_VNI", | ||
| format!("{}", vpc.dpa_vni.unwrap_or_default()).into(), | ||
| format!("{}", vpc.status.and_then(|s| s.vni).unwrap_or_default()).into(), |
There was a problem hiding this comment.
Is this copy/paste bug or we don't need this column in output anymore?
There was a problem hiding this comment.
It's not copy-pasta, but maybe the DPA field in the proto should be marked deprecated. DPA VNI is going to come from the VPC VNI now.
The question is whether we want to leave it in the proto as a separate field for any reason. @chet
If we do, then this line should pull from dpa_vni. If not, then this line could maybe just go away entirely.
There was a problem hiding this comment.
Yeah it can probably just go away entirely. I think there's something to be said re: communicating that the same VNI is used for N/S and E/W, but that doesn't really have to do with how we print this column, lol.
I could say it's temporarily useful just for people to realize, but since the DPA VNI is a new thing that people aren't leveraging yet, they'd be none the wiser anyway.
There was a problem hiding this comment.
Perfect, so no need for any deprecation process or anything? Just blow it all away completely?
It seemed like that was the case but wanted the review to make sure.
| /// be used by DPA as pools.dpa-vni | ||
| /// DPA (aka Cluster Interconnect Network) related configuration | ||
| /// In addition to enabling DPA and specifying the mqtt endpoint, | ||
| /// the vni to be used by DPA will come from pools.vpc-vni. |
There was a problem hiding this comment.
Should this actually say "the VNI to be used by DPA will be the same VNI as the VPC?" This just makes it read like we're allocating a separate VNI for E/W, and not re-using the same VNI?
| uint32 available_31_segments = 2; | ||
| uint64 total_linknet_segments = 3; | ||
| uint64 available_linknet_segments = 4; | ||
| uint64 available_linknet_segments = 4; |
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems like a basic test we'd want to keep in principle, even if it looks totally different? Or is it like totally infallible so there's way to even write it up?
There was a problem hiding this comment.
- I refactored to drop the set_vni function entirely, which is really what this was testing.
- The create_vpc test already, now, covers the "duplicate vni" case.
(last bullet point in the PR description, but might not be clear enough)
|
|
||
| -- Add a unique "constraint" similar to the one on | ||
| -- the original VNI column. | ||
| CREATE UNIQUE INDEX ON vpcs(((status->>'vni')::integer)); |
There was a problem hiding this comment.
The original index had a WHERE deleted IS NULL clause on it (so that there can be multiple with the same vni so long as they're deleted)... I think we may need to carry that forward here if possible.
5 participants
Description
This lets an API consumer know the source of the active VNI (auto/non-auto) and also gives us a place to handle additional dynamic/asynchronously populated fields in the future.
Type of Change
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes