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