Change in vdsm[master]: ifcfg: consume bond slaves owned by NetworkManager
osvoboda at redhat.com
osvoboda at redhat.com
Mon Apr 25 12:39:13 UTC 2016
Ondřej Svoboda has posted comments on this change.
Change subject: ifcfg: consume bond slaves owned by NetworkManager
......................................................................
Patch Set 3:
(4 comments)
https://gerrit.ovirt.org/#/c/56364/3//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2016-04-19 19:08:08 +0200
Line 4: Commit: Ondřej Svoboda <osvoboda at redhat.com>
Line 5: CommitDate: 2016-04-20 15:34:44 +0200
Line 6:
Line 7: ifcfg: consume bond slaves owned by NetworkManager
> I do not understand how this commit message is related to the code.
I admit I wrote quite a long introduction in order to point out the change in the last sentence.
Line 8:
Line 9: On ifcfg systems NetworkManager uses the "ifcfg-rh" plugin to persist
Line 10: its "connections" (one could say "profiles") in the same directory
Line 11: as initscripts do: /etc/sysconfig/network-scripts/ (in contrast to
Line 21: monitor-connection-files=true
Line 22:
Line 23: To let NetworkManager know VDSM wants to consume its devices we
Line 24: have to create ifcfg files with NM_CONTROLLED=no for all of them,
Line 25: not just those that differ from the expected configuration.
> as far as I can see, we already do that, and we take the nic up. with your
Currently, we don't write ifcfg files for slaves that are already attached to a bond (until my new test_consume_nm_bond this wasn't covered by tests).
Please see code in a comment I attached to ifcfg.py, it is much more readable and clearly shows the slaves are either ifdowned + having ifcfg written + ifup'ed, or left intact (while writing ifcfg files that would otherwise be missed).
Line 26:
Line 27: Change-Id: I77a600cd3bfddc0da48ae801bb121a8093254ae2
Line 28: Bug-Url: https://bugzilla.redhat.com/1304509
https://gerrit.ovirt.org/#/c/56364/3/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:
Line 158:
Line 159: for slave in bond.slaves:
Line 160: if slave.name in nicsToAdd:
Line 161: ifdown(slave.name) # nics must be down to join a bond
Line 162: # NetworkManager compatibility: ifcfg file must be written (with
> the comment is misplaced or at least unclear.
The comment may obscure the primary purpose of this patch. And that is to _actually write_ ifcfgs for bond slaves at all times (I don't understand your last statement, frankly).
Line 163: # NM_CONTROLLED=no) for all NICs we want to consume.
Line 164: self.configApplier.addNic(slave)
Line 165: if slave.name in nicsToAdd:
Line 166: _exec_ifup(slave)
PS3, Line 165: if slave.name in nicsToAdd:
This isn't pretty; I think that instead of breaking the 'if' block to two I should have an 'else' clause like this:
if slave.name in nicsToAdd:
ifdown(slave.name) # nics must be down to join a bond
self.configApplier.addNic(slave)
_exec_ifup(slave)
else:
# NetworkManager compatibility: we have to write
# ifcfg files for all
# slaves, otherwise NM wouldn't give them up.
self.configApplier.addNic(slave)
--
To view, visit https://gerrit.ovirt.org/56364
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I77a600cd3bfddc0da48ae801bb121a8093254ae2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list