Use dendrite's dpd-client rather than rolling our own #7907
Conversation
|
This also pulls in newer versions of lldp, maghemite, and dendrite which all make the same switch to dpd-client. I tagged @andrewjstone since he asked about this during demos. |
| /// | ||
| /// nat_ipv\[46\]_create are not idempotent (see oxidecomputer/dendrite#343), | ||
| /// but this wrapper function is. Call this from sagas instead. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Worth creating a type to avoid the clippy?
There was a problem hiding this comment.
I generally don't bother unless a function is called from more than one place or from another file/module, but I can if you'd like.
There was a problem hiding this comment.
Nah, I'm not a stickler.
| /// Ensure that a loopback address is created. | ||
| /// | ||
| /// loopback_ipv\[46\]_create are not idempotent (see | ||
| /// oxidecomputer/dendrite#343), but this wrapper function is. Call this |
There was a problem hiding this comment.
It looks like caller of this function, add_loopback_addresses_to_switch is called from a background task, which seems correct. What does the Call this from sagas instead indicate? Can you flesh that out a little more here please?
There was a problem hiding this comment.
Sorry - I meant to, and really should have, called out the code that was moved wholesale from clients/dpd-client/src/lib.rs when I deleted it. I don't actually know off the top of my head what that comment means, since I didn't write the code, and didn't dig into it while moving it.
@jmpesp might be able to explain it faster than I could figure it out?
There was a problem hiding this comment.
Aha thanks for pointing that out. In that case, I think this PR is good to merge. @jmpesp, can you update the comment at some point?
There was a problem hiding this comment.
Actually the underlying issue was addressed, this comment and function is outdated :) the dpd endpoints referenced in the issue are now all idempotent.
There was a problem hiding this comment.
Hmm, to be clear: ensure_loopback_created and ensure_loopback_deleted are no longer needed, but dpd_ensure_nat_entry might be.
| } | ||
| Err(e) => { | ||
| if e.status() == Some(http::StatusCode::NOT_FOUND) { | ||
| info!(log, "no nat entry found for: {target_ip:#?}"); |
There was a problem hiding this comment.
Should this be a warn! or error!?
There was a problem hiding this comment.
It doesn't look like it. This routine just wants to ensure that NAT entry exists, but doesn't have an opinion on whether it should already exist or not. This could indicate an error, or it could just be that the entry was just added by nexus and this is our first attempt to push it to the switch.
Nieuwejaar
force-pushed
the
osd
branch
4 times, most recently
from
4c756ac to
a70fcd8
Compare
No description provided.