Change in vdsm[master]: Handle correctly nic names like p33p1.

Juan Hernandez has uploaded a new change for review.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Handle correctly nic names like p33p1.

Currently these types of network interface names are not handled
correctly in the "nicSort" method because it assumes that the name
is composed by a name (like "eth") and a number. As a result
anything after the first number is removed.

Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
---
M vdsm/configNetwork.py
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/99/799/1
--
To view, visit http://gerrit.ovirt.org/799
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez

Juan Hernandez has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

I found this while adding a freshly installed F16 as a node. The NIC happens to be named p33p1.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

oh dear, that's pretty bad. Do we have a downstream bug for this?

Federico, why sorted(nics) was not enough to solve https://bugzilla.redhat.com/show_bug.cgi?id=732272 ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Juan Hernandez has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

I think that "Consistent Device Naming" is new with Fedora 15 and is not yet used in RHEL 6, so this shouldn't happen downstream.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

Juan, RHEV-3.0 should have supported such names, see bug 682790. And it was supported until this unfortunate side effect of bug 732272.

Would you open a vdsm bug on this? What exactly is the mishandling of the current code?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Juan Hernandez has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

Dan, I found the issue in Fedora 16, using the RPMS prepared by Federico. I think that the issue will be exactly the same in RHEL, but I didn't test it. Do you want me to open the bug for Fedora or for RHEL?

The nicSort method wants to extract the name ("eth" from "eth0") and the number ("0" from "eth0") from nic names in order to sort them by name. If a name like "p33p1" is given then it will extract "p" and "33". Later the same method rebuilds the nic concatenating the name and the number, this results in "p33" in my case.

I added ^ and $ anchors to the regular expression used, to make sure that names like "p33p1" will not match and will not be split/rebuilt incorrectly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

Juan, I'd like to see a rhel-6.3 bug.

I understood your patch; I was asking about the perceived bug from from a user PoV. The nic stays down after adding a network using it?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Juan Hernandez has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1:

Sorry for my the misunderstanding Dan.

The problem appears when registering the host. This is the relevant line in vds_bootstrap.log:

vds_bootstrap.67033.log:Wed, 28 Dec 2011 18:25:49 DEBUG    INFO:root:Adding bridge rhevm with vlan=, bonding=, nics=['p33p1']. bondingOptions=None, options={'blockingdhcp': 'true', 'IPADDR': '192.168.100.2', 'NM_CONTROLLED': 'no', 'PREFIX': '24', 'BOOTPROTO': 'none', 'ONBOOT': 'yes'}
vds_bootstrap.67033.log:WARNING:root:/sbin/ifup: configuration for p33 not found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Handle correctly nic names like p33p1.
......................................................................

Patch Set 1: (1 inline comment)

....................................................
Commit Message
Line 7: Handle correctly nic names like p33p1.
commit message should start with BZ#770834

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

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

(1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 133:     nics_rexp = re.compile("^([A-Za-z]+)([0-9]+)$")
This is not correct because it's going to filter out interfaces like "p33p1".
The original sed code is:

sed -e '/ifcfg-[A-Za-z0-9#._-]+$/ { s/^ifcfg-//g;s/[0-9]/ &/}'

This means that "p33p1" should be split as:

$ echo ifcfg-p33p1 \
        | sed -e '/ifcfg-[A-Za-z0-9#._-]+$/ { s/^ifcfg-//g;s/[0-9]/ &/}'
 p 33p1

The ordering then is the same:

sort -k 1,1 -k 2n

(It should be what we already have in the python code)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 133:     nics_rexp = re.compile("^([A-Za-z]+)([0-9]+)$")
The original sed is using the $ anchor, like I did in python. Without that anchor the regular expression matches only "p33".

Try it without the anchors:

python -c 'import re; s = re.compile("([A-Za-z]+)([0-9]+)"); m = s.match("p33p1"); print(m.group(1));'
 python -c 'import re; s = re.compile("([A-Za-z]+)([0-9]+)"); m = s.match("p33p1"); print(m.group(2));'

This gives "p" and "33". The "p1" part is gone.

With the anchors it will not match, and that is exactly what I wanted. That way in the loop the complete nic name is added back to the list.

The solution is not perfect, but it is better than what we have now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 133:     nics_rexp = re.compile("^([A-Za-z]+)([0-9]+)$")
Given your fix (anchors) consider "p33p1" and "p331":

echo -e "ifcfg-p33p1\nifcfg-p331" \
   | sed -e '/ifcfg-[A-Za-z0-9#._-]+$/ { s/^ifcfg-//g;s/[0-9]/ &/}' | sort -k 1,1 -k 2n
 p 33p1
 p 331

print nicSort(["p33p1", "p331"])
 ['p331', 'p33p1']

The order is different.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 133:     nics_rexp = re.compile("^([A-Za-z]+)([0-9]+)$")
That is right. I didn't intend to solve the sorting problem, just the "p1" part of the interface name disappearing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 3:

Now it splits the nic name into the text and numeric parts, and then it sorts different nic names sorting according alphabetical order for text components like "eth" or "p" and according to the numeric values of numeric components line "0" or "33". This will work for any number of components, even for things like "a0b1c33".

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

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

(1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 157:     return sorted(nics, key=splitNicForSorting)
Given "p33p10" and "p33p2":

echo -e "ifcfg-p33p2\nifcfg-p33p10" \
   | sed -e '/ifcfg-[A-Za-z0-9#._-]+$/ { s/^ifcfg-//g;s/[0-9]/ &/}' \
   | sort -k 1,1 -k 2n
 p 33p10
 p 33p2

print sorted(["p33p10", "p33p2"], key=splitNicForSorting)
 ['p33p2', 'p33p10']

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 157:     return sorted(nics, key=splitNicForSorting)
That is an error in the sed script, i guess, it is not sorting correctly according to the second number. It is like sorting "eth0" and "eth1" as [ 'eth1', 'eth0' ]. Try the sed script with "p33p10", "p33p2" and "p33p20":

echo -e "ifcfg-p33p2\nifcfg-p33p10\nifcfg-p33p20" | sed -e '/ifcfg-[A-Za-z0-9#._-]+$/ { s/^ifcfg-//g;s/[0-9]/ &/}' | sort -k 1,1 -k 2n
p 33p10
p 33p2
p 33p20

It is not sorting correctly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

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

(1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 157:     return sorted(nics, key=splitNicForSorting)
we should be just like initscripts' sort. If they have a bug, please open a BZ# on them. I do not think that the "correctness" of the sort is important. I care only about consistency with initscripts - so until they fix the BZ you are going to open, we need to be conform to their sort function.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has posted comments on this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/configNetwork.py
Line 157:     return sorted(nics, key=splitNicForSorting)
I don't see any reason to open a bug on initscripts as I didn't detect any problem there. The problem was/is in vds_bootstrap.py and ultimately in configNetwork.py.

I don't really know if the correctness of the sort is important. What is really important is to make sure that no part of the interface name is lost. For that patch set 2 is enough.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez

Change in vdsm[master]: BZ#770834 - Handle correctly nic names like p33p1

Juan Hernandez has abandoned this change.

Change subject: BZ#770834 - Handle correctly nic names like p33p1
......................................................................

Patch Set 3: Abandoned

In favor of 1104.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I716f5d7d137325abaa0022a40f482c9ad270855a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Juan Hernandez