Change in vdsm[master]: net: ovs: remove initial nic ip, dont touch unchanged networks

phoracek at redhat.com phoracek at redhat.com
Thu Oct 29 12:18:37 UTC 2015


Petr Horáček has posted comments on this change.

Change subject: net: ovs: remove initial nic ip, dont touch unchanged networks
......................................................................


Patch Set 6:

(9 comments)

https://gerrit.ovirt.org/#/c/46692/6/vdsm_hooks/ovs/ovs_before_network_setup_ip.py
File vdsm_hooks/ovs/ovs_before_network_setup_ip.py:

Line 89:         _run_dhclient(iface, blockingdhcp, ipv6.defaultRoute, 6)
Line 90:     iproute2._addSourceRoute(net_dev)
Line 91: 
Line 92: 
Line 93: def _remove_ip_config(iface, ipv4, ipv6, vdsm=True):
> such an awful arg name begs for a docstring
i forgot to remove this one, not needed
Line 94:     net_dev = NetDevice(iface, iproute2, ipv4=ipv4, ipv6=ipv6)
Line 95:     DynamicSourceRoute.addInterfaceTracking(net_dev)
Line 96:     DhcpClient(iface).shutdown()
Line 97:     iproute2._removeSourceRoute(net_dev, DynamicSourceRoute)


Line 100:             ipwrapper.addrFlush(iface)
Line 101: 
Line 102: 
Line 103: def _drop_ip_config(iface):
Line 104:     """Remove IP configuration of a new nic attached to VDSM"""
> nics are controlled by Vdsm, not attached to it.
Done
Line 105:     if os.path.exists(os.path.join('/sys/class/net', iface)):
Line 106:         kill_dhclient(iface, family=4)  # kill_dhclient flushes IP
Line 107:         kill_dhclient(iface, family=6)
Line 108: 


Line 114:         ipv4 = _get_ipv4_model(attrs)
Line 115:         ipv6 = _get_ipv6_model(attrs)
Line 116:         port = attrs.get('nic') or attrs.get('bonding')
Line 117:         blocking_dhcp = 'blockingdhcp' in attrs
Line 118:         return top_dev, ipv4, ipv6, port, blocking_dhcp
> Tuples are not bad by themselves, but I'd like to see a container object wi
Done
Line 119: 
Line 120:     ip_config_to_set = {}
Line 121:     ip_config_to_remove = {}
Line 122: 


Line 128:         else:
Line 129:             top_dev, ipv4, ipv6, port, blocking_dhcp = _gather_ip_config(attrs)
Line 130: 
Line 131:             if net in init_nets:  # if network was edited
Line 132:                 init_top_dev, init_ipv4, init_ipv6, _, _ = \
> We can do without slashes.
Done
Line 133:                     _gather_ip_config(init_nets[net])
Line 134: 
Line 135:                 # drop IP of newly attached nics
Line 136:                 if init_nets[net].get('nic') != attrs.get('nic') is not None:


Line 132:                 init_top_dev, init_ipv4, init_ipv6, _, _ = \
Line 133:                     _gather_ip_config(init_nets[net])
Line 134: 
Line 135:                 # drop IP of newly attached nics
Line 136:                 if init_nets[net].get('nic') != attrs.get('nic') is not None:
> I failed to interpret this.
yourself or inside interpreter?

it means: (original nic is not the same as the new one) and (new nic is not None). = we changed nic, not just removed it.

    In [1]: init_nets = {'net1': {'nic': 'dummy_1'}}
    
    In [2]: init_nets['net1'].get('nic') != {'nic': 'dummy_1'}.get('nic') is not None
    Out[2]: False
    
    In [3]: init_nets['net1'].get('nic') != {'nic': None}.get('nic') is not None
    Out[3]: False

    In [4]: init_nets['net1'].get('nic') != {'nic': 'dummy_2'}.get('nic') is not None
    Out[4]: True
Line 137:                     _drop_ip_config(attrs.get('nic'))
Line 138: 
Line 139:                 # if IP config is to be changed or net top device was changed,
Line 140:                 # remove initial configuration and set the new one if any


Line 135:                 # drop IP of newly attached nics
Line 136:                 if init_nets[net].get('nic') != attrs.get('nic') is not None:
Line 137:                     _drop_ip_config(attrs.get('nic'))
Line 138: 
Line 139:                 # if IP config is to be changed or net top device was changed,
> a network's top device?
Done
Line 140:                 # remove initial configuration and set the new one if any
Line 141:                 if (ipv4 != init_ipv4 or ipv6 != init_ipv6 or
Line 142:                         attrs.get('vlan') != init_nets[net].get('vlan')):
Line 143:                     ip_config_to_remove[init_top_dev] = ipv4, ipv6


Line 136:                 if init_nets[net].get('nic') != attrs.get('nic') is not None:
Line 137:                     _drop_ip_config(attrs.get('nic'))
Line 138: 
Line 139:                 # if IP config is to be changed or net top device was changed,
Line 140:                 # remove initial configuration and set the new one if any
> Please be more verbose. Is it "any configuration"?
Done
Line 141:                 if (ipv4 != init_ipv4 or ipv6 != init_ipv6 or
Line 142:                         attrs.get('vlan') != init_nets[net].get('vlan')):
Line 143:                     ip_config_to_remove[init_top_dev] = ipv4, ipv6
Line 144:                     if ipv4 or ipv6:


Line 146:                                                      blocking_dhcp)
Line 147:             else:  # if network was added
Line 148:                 # drop IP of newly attached nics
Line 149:                 if attrs.get('nic') is not None:
Line 150:                     _drop_ip_config(attrs.get('nic'))
> You can attrs.get('nic') first, store it in a variable and then see if it i
Done
Line 151: 
Line 152:                 # set networks IP configuration if any
Line 153:                 if ipv4 or ipv6:
Line 154:                     ip_config_to_set[top_dev] = (ipv4, ipv6, port,


Line 151: 
Line 152:                 # set networks IP configuration if any
Line 153:                 if ipv4 or ipv6:
Line 154:                     ip_config_to_set[top_dev] = (ipv4, ipv6, port,
Line 155:                                                  blocking_dhcp)
> Again, tuples.
Done
Line 156: 
Line 157:     hooking.log('Remove IP configuration of: %s' % ip_config_to_remove)
Line 158:     hooking.log('Set IP configuration: %s' % ip_config_to_set)
Line 159:     for iface, (ipv4, ipv6) in ip_config_to_remove.iteritems():


-- 
To view, visit https://gerrit.ovirt.org/46692
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9682c75cfb79f75b64cb8cb587fc3ed01cf15c8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan 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: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list