Bug 956994

Summary: iface-start/destroy should add checkpoint of interface's status before execute
Product: Red Hat Enterprise Linux 7 Reporter: EricLee <bili>
Component: libvirtAssignee: Michal Privoznik <mprivozn>
Status: CLOSED NOTABUG QA Contact: Virtualization Bugs <virt-bugs>
Severity: medium Docs Contact:
Priority: medium    
Version: 7.0CC: acathrow, bili, cwei, dallan, dyuan, honzhang, laine, mzhan
Target Milestone: rc   
Target Release: ---   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 956995 (view as bug list) Environment:
Last Closed: 2014-01-29 14:01:35 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 956995    

Description EricLee 2013-04-26 07:26:57 UTC
Description of problem:
iface-start/destroy should add checkpoint of interface's status before execute, or that will cause iface-start/destroy an interface which defined with static IP(contain ipv4 and ipv6) always return 0(successfully) even the interface is already in active/inactive status.

Version-Release number of selected component (if applicable):
# rpm -qa libvirt qemu-kvm; uname -r
qemu-kvm-1.4.0-2.1.el7.x86_64
libvirt-1.0.4-1.1.el7.x86_64
3.9.0-0.rc7.52.el7.x86_64

How reproducible:
100%

Steps:
1. Stop the NetworkManager service.
Prepare a interface xml(replace MAC addr to your own one):
# cat net-eth2-ipv4.xml
<interface type='ethernet' name='eth2'>
  <start mode='onboot'/>
  <mac address='00:1b:21:39:8b:19'/>
  <protocol family='ipv4'>
    <ip address='10.66.6.91' prefix='23'/>
  </protocol>
</interface>

2. Undefine the pre-existing interface eth2(using iface-destroy, and iface-undefine)

3. Define new eth2 with static IP

# virsh iface-define net-eth2-ipv4.xml
Interface eth2 defined from net-eth2-ipv4.xml

# virsh iface-start eth2
Interface eth2 started

# ifconfig eth2
eth2: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 10.66.6.91  netmask 255.255.254.0  broadcast 10.66.7.255
        inet6 fe80::21b:21ff:fe39:8b19  prefixlen 64  scopeid 0x20<link>
        ether 00:1b:21:39:8b:19  txqueuelen 1000  (Ethernet)
        RX packets 130589  bytes 10167550 (9.6 MiB)
        RX errors 0  dropped 25  overruns 0  frame 0
        TX packets 5662  bytes 1257190 (1.1 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device memory 0xf4820000-f4840000  

# virsh iface-list --all
Name                 State      MAC Address
--------------------------------------------
eth0                 active     d83:85:7e:61:9b
eth1                 active     00:1b:21:39:8b:18
eth2                 active     00:1b:21:39:8b:19
lo                   active     00:00:00:00:00:00

4. do not destroy eth2, re-start it:
# virsh iface-start eth2
Interface eth2 started

# echo $?
0

# virsh iface-start eth2
Interface eth2 started

# echo $?
0

# virsh iface-start eth2
Interface eth2 started

# echo $?
0

Always start successfully even the interface is in active status.

Actual results:
As steps

Expected results:
Iface-start should fail when interface is already in active status.

Additional info:

If interface is in active status,
# ifup eth2
RTNETLINK answers: File exists
will get some suitable reply for the user, but iface-start just masked it and return nothing.

For destroy problem, they(iface-destroy and ifdown) are working as the same operation.

IMO, if we want more suitable custom taste for this feature, we'd better add this checkpoint for it.


Even more, iface-define also need do some checkpoint for existing interface to modify the ifcfg-* file careless:
# virsh iface-list --all
Name                 State      MAC Address
--------------------------------------------
eth0                 active     d83:85:7e:61:9b
eth1                 active     00:1b:21:39:8b:18
lo                   active     00:00:00:00:00:00

define a static IP interface named eth0 will also successfully without any prompt.
# cat net-static-eth0.xml
<interface type='ethernet' name='eth0'>
  <start mode='onboot'/>
  <protocol family='ipv4'>
    <ip address='10.66.4.54' prefix='23'/>
  </protocol>
  <protocol family='ipv6'>
    <ip address='3ffe::200' prefix='64'/>
  </protocol>
</interface>

# virsh iface-define net-static-eth0.xml
Interface eth0 defined from net-static-eth0.xml

That will change ifcfg-eth0 file:
# cat /etc/sysconfig/network-scripts/ifcfg-eth0
DEVICE=eth0
ONBOOT=yes
BOOTPROTO=none
IPADDR=10.66.4.54
NETMASK=255.255.254.0
IPV6INIT=yes
IPV6_AUTOCONF=no
DHCPV6=no
IPV6ADDR=3ffe::200/64

And start eth0 again:
# virsh iface-start eth0
Interface eth0 started

that will cause iface-dumpxml get un-coincident xml info with ifconfig eth0:
# virsh iface-dumpxml eth0
<interface type='ethernet' name='eth0'>
  <mac address='d83:85:7e:61:9b'/>
  <protocol family='ipv4'>
    <ip address='10.66.5.8' prefix='22'/>
    <ip address='10.66.4.54' prefix='23'/>    --- will cause the interface have two ip addr
  </protocol>
  <protocol family='ipv6'>
    <ip address='3ffe::200' prefix='64'/>     --- and also for ipv6
    <ip address='fe80:ad3:85ff:fe7e:619b' prefix='64'/>
  </protocol>
</interface>

# ifconfig eth0
eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 10.66.5.8  netmask 255.255.252.0  broadcast 10.66.7.255
        inet6 3ffe::200  prefixlen 64  scopeid 0x0<global>
        inet6 fe80:ad3:85ff:fe7e:619b  prefixlen 64  scopeid 0x20<link>
        ether d83:85:7e:61:9b  txqueuelen 1000  (Ethernet)
        RX packets 94448  bytes 8385061 (7.9 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 2192  bytes 632688 (617.8 KiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 17

Comment 3 Michal Privoznik 2013-12-11 09:17:08 UTC
Patches proposed upstream:

https://www.redhat.com/archives/libvir-list/2013-December/msg00611.html

Comment 4 Michal Privoznik 2013-12-12 14:26:56 UTC
Patches were reject upstream. I mean, it's no longer true that one can't ifup an active interface. In fact, it's desired to ifup an active interface as the interface can re-request DHCP (and other stuff) without need to bring the interface down and thus losing connectivity. Hence I'm tentative to close this one. Eric, what's your opinion?

Comment 5 EricLee 2013-12-13 01:52:46 UTC
(In reply to Michal Privoznik from comment #4)
> Patches were reject upstream. I mean, it's no longer true that one can't
> ifup an active interface. In fact, it's desired to ifup an active interface
> as the interface can re-request DHCP (and other stuff) without need to bring
> the interface down and thus losing connectivity. Hence I'm tentative to
> close this one. Eric, what's your opinion?

Hi Michal,

I understand your words: it's desired to ifup an active interface as the interface can re-request DHCP (and other stuff) without need to bring the interface down and thus losing connectivity, and we can drop that.

However, the other problem what I mentioned in "Additional info" still should be fixed:
Even more, iface-define also *need do some checkpoint for existing interface* to modify the ifcfg-* file carelessly:
# virsh iface-list --all
Name                 State      MAC Address
--------------------------------------------
eth0                 active     d83:85:7e:61:9b
eth1                 active     00:1b:21:39:8b:18
lo                   active     00:00:00:00:00:00

define a static IP interface named eth0 will also successfully without any prompt.
# cat net-static-eth0.xml
<interface type='ethernet' name='eth0'>
  <start mode='onboot'/>
  <protocol family='ipv4'>
    <ip address='10.66.4.54' prefix='23'/>
  </protocol>
  <protocol family='ipv6'>
    <ip address='3ffe::200' prefix='64'/>
  </protocol>
</interface>

# virsh iface-define net-static-eth0.xml
Interface eth0 defined from net-static-eth0.xml

That will change ifcfg-eth0 file:
# cat /etc/sysconfig/network-scripts/ifcfg-eth0
DEVICE=eth0
ONBOOT=yes
BOOTPROTO=none
IPADDR=10.66.4.54
NETMASK=255.255.254.0
IPV6INIT=yes
IPV6_AUTOCONF=no
DHCPV6=no
IPV6ADDR=3ffe::200/64

And start eth0 again:
# virsh iface-start eth0
Interface eth0 started

that will cause iface-dumpxml get un-coincident xml info with ifconfig eth0:
# virsh iface-dumpxml eth0
<interface type='ethernet' name='eth0'>
  <mac address='d83:85:7e:61:9b'/>
  <protocol family='ipv4'>
    <ip address='10.66.5.8' prefix='22'/>
    <ip address='10.66.4.54' prefix='23'/>    --- will cause the interface have two ip addr
  </protocol>
  <protocol family='ipv6'>
    <ip address='3ffe::200' prefix='64'/>     --- and also for ipv6
    <ip address='fe80:ad3:85ff:fe7e:619b' prefix='64'/>
  </protocol>
</interface>

# ifconfig eth0
eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 10.66.5.8  netmask 255.255.252.0  broadcast 10.66.7.255
        inet6 3ffe::200  prefixlen 64  scopeid 0x0<global>
        inet6 fe80:ad3:85ff:fe7e:619b  prefixlen 64  scopeid 0x20<link>
        ether d83:85:7e:61:9b  txqueuelen 1000  (Ethernet)
        RX packets 94448  bytes 8385061 (7.9 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 2192  bytes 632688 (617.8 KiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 17


Thanks,
EricLee

Comment 6 Laine Stump 2014-01-29 14:01:35 UTC
virsh iface-define is no more careless than virsh define or virsh net-define - none of them checks for an existing domain/network/interface of the same name prior to making the new definition, as re-defining something existing is equivalent to editing it. As is normal with a powerful tool, it is easy to shoot yourself in the foot.

What *does* already exist, is a method of taking a snapshot of the network configuration state prior to making any modifications:

   virsh iface-begin

will save a copy of all the ifcfg-* files in /etc/sysconfig/network-scripts. You can then do all of the virsh iface-define/undefine you like, and the results will be made directly into /etc/sysconfig/network-scripts. But if you decide you don't want those changes, you can run:

   virsh iface-rollback

and the original ifcfg-* files will be put back in place. Once you are certain that the configuration is as you want it, you run:

   virsh iface-commit

to make the changes permanent. If you reboot the system before running virsh iface-commit, all network config changes that you made after the virsh iface-begin will be automatically rolled back.

This seems to me to be about as much of a safeguard as can be added without changing the basic API in a non-compatible way.

Based on that, I'm closing this as NOTABUG. If you disagree, feel free to re-open.