extract loopback ip manager from maghemite#347
extract loopback ip manager from maghemite#347internet-diglett wants to merge 7 commits intomainfrom
Conversation
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
Thanks @internet-diglett. Comments/questions follow.
| let mut ip_descr = format!("{v}{}", ip.address); | ||
| ip_descr.retain(|c| c.is_alphanumeric()); | ||
| let addr_obj = format!("{}/{}", ifname, ip_descr); | ||
| let cmd = [ |
There was a problem hiding this comment.
It would be nice to use https://github.com/oxidecomputer/netadm-sys/ here eventually instead of shelling out. If you want something cross plaform https://github.com/oxidecomputer/network-interface/tree/illumos might be an option.
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| // "Address already exists/assigned" is ok — another process beat us to it | ||
| if !stderr.to_lowercase().contains("already") { |
There was a problem hiding this comment.
This seems quite fragile.
There was a problem hiding this comment.
I think most of this implementation is what levon took out of maghemite, i.e. it's what I wrote originally.
When I wrote it, I just needed something quickly to enable multiple BGP speakers on the same local system to use TCP and I just got a quick and dirty impl working.
Do you view this as a blocker?
| break; | ||
| } | ||
|
|
||
| // Last in-process user: decrement the cross-process refcount |
There was a problem hiding this comment.
What is the multiprocess model here? What does it mean semantically for multiple processes to hold a reference to a loopback address? Since only one process can listen on a given address at a time for a particular port, it's not clear to me what multiple processes holding a reference to a loopback address means.
There was a problem hiding this comment.
My initial implementation was meant to accommodate both cargo test (separate thread per test) and cargo nextest (separate process per test).
I've updated all the bgp tests relying on this to use unique IPs per test to avoid overlap where possible, but I wanted the framework to support IP re-use across tests regardless of whether the tests are executed in the same or different processes.
There was a problem hiding this comment.
I could see this being applicable to the omicron-dev stuff levon is doing by having a set of processes all have a reference to a Loopback IP since they pretend to be the same virtual node
taspelund
left a comment
taspelund
left a comment
There was a problem hiding this comment.
LGTM. Seems like a straightforward port of the maghemite implementation.
Obviously there's room for improvement, but I'm not sure if that needs to be addressed prior to merge.
3 participants
No description provided.