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 <osvob...@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 <osvob...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches