networking: refactor is_physical from cloudinit.net#457
Conversation
|
|
||
| pass | ||
| def is_physical(self, devname: DeviceName) -> bool: | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
i've had someone from #FreeBSD contribute this piece of sh code:
xeon-freebsd ➜ ~ { ifconfig -l | xargs -n1; ifconfig -C | xargs -n1 ifconfig -g; } | sort | uniq -u
cxl0
cxl1
igb0
igb1
igb2
igb3
igb4
igb5
to list only the physical devices.
we'll have to check this under netbsd / openbsd…
There was a problem hiding this comment.
NetBSD does not have ifconfig -g but you could perhaps use this instead (also works on FreeBSD):
ifconfig -l | xargs -n1 | sed -E "/^($(ifconfig -C | tr \ \|))/ d"
There was a problem hiding this comment.
thanks for the help, @suominen
so the question now is, which of these parts can we execute in the shell, and which should we execute in python
i think we could execute ifconfig -C only once and cache the result for the lifetime of cloud-init's run with @lru_cache
ifconfig -l otoh, could be different every time, no less thanks to our doings
There was a problem hiding this comment.
Thanks for the thoughts! Just to set expectations, I would expect us to land this PR without a BSD implementation as it isn't supported in the current code, and this is a refactor. Obviously the whole point of this refactoring exercise is to make it easy to identify and address the gaps in networking support (particularly on BSD), so this is an encouraging start!
OddBloke
force-pushed
the
net
branch
3 times, most recently
from
a57573a to
f8f5ed5
Compare
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks for putting this reference PR together @OddBloke I think it makes it a lot clearer how we can piece together solutions for each part of the net-refactor.
Minor unit test request from me just since it feels like low-hanging fruit in OpenNebula.
| net = ds.OpenNebulaNetwork( | ||
| context, system_nics_by_mac={MAC_1: 'enp0s25', MAC_2: 'enp1s2'}) | ||
| context, | ||
| mock.Mock(), |
There was a problem hiding this comment.
Since we are modifying the call signature of read_context_disk_dir to now accept and operate on a distro instance. can we have a test that asserts distro.networking.is_physical is called for the interfaces returned by get_interfaces_by_mac and extend this existing test to assert that distro.networking.is_physical is not called becaues system_nics_by_mac is provided.
There was a problem hiding this comment.
can we have a test that asserts distro.networking.is_physical is called for the interfaces returned by get_interfaces_by_mac
Yep, I can add tests for get_physical_nics_by_mac.
and extend this existing test to assert that distro.networking.is_physical is not called becaues system_nics_by_mac is provided.
system_nics_by_mac is only passed by test code, so I don't think we need to test it specifically; those tests would fail if the values being passed were not used. Conversely, we already have testing that mocks get_physical_nics_by_mac and relies on the output it is mocked to return (e.g. test_gen_conf_mtu); if it weren't being called, then these tests would fail. I will add a test that distro is passed through appropriately, that's a gap currently.
Does that sound reasonable?
There was a problem hiding this comment.
I've addressed these two testing gaps with my latest push.
4 participants
As the first refactor PR, this also includes the initial structure for tests.
LP: #1884619