Change in vdsm[master]: confNet: ifcfg's VLAN should be set only on vlan cfg

[Dan Kenigsberg]

Dan Kenigsberg has uploaded a new change for review.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

confNet: ifcfg's VLAN should be set only on vlan cfg

If a misguided script calls addNetwork with explicit VLAN option, it
sohuld not be blindly copied to the top-most device.
We set VLAN=yes on the ifcfg script for the vlan device, and that's
that.

Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Bug-Id: BZ#847733
Signed-off-by: Dan Kenigsberg 
---
M vdsm/configNetwork.py
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/7411/1

diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py
index 3f99a66..2ef0a8a 100755
--- a/vdsm/configNetwork.py
+++ b/vdsm/configNetwork.py
@@ -384,7 +384,7 @@
         if mtu:
             cfg = cfg + 'MTU=%d\n' % mtu
         cfg += 'NM_CONTROLLED=no\n'
-        BLACKLIST = ['TYPE', 'NAME', 'DEVICE', 'bondingOptions',
+        BLACKLIST = ['TYPE', 'NAME', 'DEVICE', 'VLAN', 'bondingOptions',
                      'force', 'blockingdhcp',
                      'connectivityCheck', 'connectivityTimeout',
                      'implicitBonding']

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/590/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: oVirt Jenkins CI Server

Antoni Segura Puimedon has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 1: (1 inline comment)

small typo fix in the commit message.

....................................................
Commit Message
Line 6: 
Line 7: confNet: ifcfg's VLAN should be set only on vlan cfg
Line 8: 
Line 9: If a misguided script calls addNetwork with explicit VLAN option, it
Line 10: sohuld not be blindly copied to the top-most device.
s/sohuld/should/
Line 11: We set VLAN=yes on the ifcfg script for the vlan device, and that's
Line 12: that.
Line 13: 
Line 14: Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 1: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: oVirt Jenkins CI Server

Mark Wu has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 1: I would prefer that you didn't submit this

After looking at the code, I found this problem could be cause by any wrong option specified by the misguided script. Why do we only blacklist "VLAN"? How about covert the BLACKLIST into WHITELIST?  Actually,  I am wondering if there's any option need passthrough vdsm and be written into the the conf file. If no, we should remove the parameter 'kwargs' from _createConfFile or even addNic, addVlan, addBonding and addBridge.

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 1:

You have raised a valid issue here. The original intention was to keep vdsm's API as a stupid pass-through on top of initscripts. E.g. if someone ever wanted to set IPV6ADDR, they could do it with no changes to vdsm. However, the code has rotten. The pass-through should have stopped at the top-level device (bridge), but currently "options" is duplicated to all ifcfg files, which is mostly useless.

We need to clear this mess, one way or another. Going towards a stricter, well-defined, API is what we need, anyway. Particularly, if we are to re-implement network config over netcf/NM

--
To view, visit http://gerrit.ovirt.org/7411
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has abandoned this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Abandoned

Completely out of date

[Dan Kenigsberg]

Dan Kenigsberg has restored this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Restored

it's very partial, but let us restore it.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7318/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6416/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7200/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 2: Verified+1

Moti Asayag has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/7411/2//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: confNet: ifcfg's VLAN should be set only on vlan cfg
Line 8: 
Line 9: If a misguided script calls addNetwork with explicit VLAN option, it
Line 10: sohuld not be blindly copied to the top-most device.
s/sohuld/should
Line 11: We set VLAN=yes on the ifcfg script for the vlan device, and that's
Line 12: that.
Line 13: 
Line 14: This is a very limitted hack of a wider problem: unrecongnized network

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7338/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6436/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7220/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 3: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

Patch Set 3: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: confNet: ifcfg's VLAN should be set only on vlan cfg
......................................................................

confNet: ifcfg's VLAN should be set only on vlan cfg

If a misguided script calls addNetwork with explicit VLAN option, it
should not be blindly copied to the top-most device.
We set VLAN=yes on the ifcfg script for the vlan device, and that's
that.

This is a very limited hack of a wider problem: unrecognized network
options are copied to the bridge's ifcfg file.

Bug-Url: https://bugzilla.redhat.com/1051150
Change-Id: Ia2c9d6c86e73104424ca8ac901bf09e4b98a47ab
Signed-off-by: Dan Kenigsberg 
Reviewed-on: http://gerrit.ovirt.org/7411
Reviewed-by: Antoni Segura Puimedon 
---
M vdsm/netconf/ifcfg.py
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Antoni Segura Puimedon: Looks good to me, but someone else must approve
  Dan Kenigsberg: Verified; Looks good to me, approved