Change in vdsm[master]: vdsm: allow hooks to pass down dictionaries in json format

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: allow hooks to pass down dictionaries in json format
......................................................................


Patch Set 6: Code-Review-1

(4 comments)

....................................................
File tests/functional/networkTests.py
Line 1560:                     network_config = json.load(cookie_file)
Line 1561:                     self.assertTrue('networks' in network_config,
True)
Line 1562:                     self.assertTrue('bondings' in network_config,
True)
Line 1563:                     self.assertTrue(NETWORK_NAME in
network_config['networks'],
Line 1564:                                     True)
I think that Assaf has noted this earlier the RunningConfig is going to work only work
with unified_persistence turned on.
Line 1565: 
Line 1566:                 delete_networks = {NETWORK_NAME: {'remove': True}}
Line 1567:                 self.vdsm_net.setupNetworks(delete_networks,
Line 1568:                                             {}, {})


....................................................
File vdsm/configNetwork.py
Line 645:                                get('bonding') in bondings)
Line 646:                 raise ConfigNetworkError(ne.ERR_LOST_CONNECTION,
Line 647:                                          'connectivity check failed')
Line 648: 
Line 649:     running_config = netconfpersistence.RunningConfig()
I can only repeat my request: do not pass *only* the current_config to the hook. Passing
the requested changes is important for giving context to the hook script (for example, it
has to do some "last final touches" to the modified networks). The hook could
figure out the current configuration on its own.
Line 650:     hooks.after_network_setup({'networks': running_config.networks,
Line 651:                                'bondings': running_config.bonds})
Line 652: 
Line 653: 


....................................................
File vdsm/vdsmd.8.in
Line 88:    "networks": {"virtnet": {"bonding" :
"bond0", "bridged": true, "vlan":27}},
Line 89:     "options": {"conectivityCheck":false}
Line 90:   }
Line 91: .fi
Line 92: 
It would be easier and safer to simply say that network, bonding, and options (please use
this order) are as defined in the arguments to setupNetworks. The above example is good,
but I am afraid that the following details should be placed in a documentation of the
setupNetwork verb, not the hook.

I feel kinda bad, since only now do I understand what you meant in the troubles of
documenting all the different arguments.
Line 93: .B bondings specifies any bonding adapter that must be built upon several
Line 94: physical adapters, or removed when we pass the "remove":true option.
Line 95: 
Line 96: .B networks specifies any network that must be setup, and it's parameters.


Line 158: sent by the caller of the hook. For example if before_nic_hotplug is called
Line 159: with custom: {qos: '0.5', color: 'red'} then qos and color will
be directly
Line 160: available as environment variables when before_nic_hotplug is called.
Line 161: 
Line 162: 
intentional?
Line 163: .SS Hook execution
Line 164: before_vdsm_start and after_vdsm_stop scripts are executed as user
Line 165: .I root.
Line 166: All the other hooks are executed as user


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie07c511e9740fd19a1c27baf87e91f9a427d0dcd
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Miguel Angel Ajo Pelayo <majopela(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Miguel Angel Ajo Pelayo <majopela(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes