Edward Haas has posted comments on this change.
Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................
Patch Set 8: Code-Review-1
(8 comments)
https://gerrit.ovirt.org/#/c/65065/8/lib/vdsm/network/libvirt.py
File lib/vdsm/network/libvirt.py:
PS8, Line 45: bridge
There is a hidden assumption now for the caller: If bridge is not None, then network ==
bridge.
I guess with the suggestion from line 62, this can be treated.
Line 58: determines respectively the presence of bridge element
Line 59: or interface subelement.
Line 60: """
Line 61:
Line 62: if bridge and iface:
It will be cleaner to just have two funcs then, one for bridged and one for iface,
extracting the common code to their own funcs.
Line 63: raise AttributeError('Cannot set both bridge and iface at once')
Line 64:
Line 65: netName = LIBVIRT_NET_PREFIX + network
Line 66:
https://gerrit.ovirt.org/#/c/65065/8/tests/network/libvirt_test.py
File tests/network/libvirt_test.py:
PS8, Line 41: Unit
How about 'CreateXml' ?
PS8, Line 101: Integration
How about 'CreateNetwork'?
Line 100: @attr(type='integration')
Line 101: class LibvirtIntegrationTests(TestCaseBase):
Line 102:
Line 103: @ValidateRunningAsRoot
Line 104: def _test_create_net(self, **net_attrs):
Private funcs/methods at the bottom please.
Line 105: net_def = libvirt.createNetworkDef(NETWORK, **net_attrs)
Line 106: with libvirt_net(NETWORK, net_def):
Line 107: nets = libvirt.networks()
Line 108: self.assertIn(NETWORK, nets)
Line 106: with libvirt_net(NETWORK, net_def):
Line 107: nets = libvirt.networks()
Line 108: self.assertIn(NETWORK, nets)
Line 109: self.assertEqual(nets[NETWORK], net_attrs)
Line 110: nets = libvirt.networks()
Why do you need to check it is cleaned all the time?
I think a single test that checks that the removal is successful is enough, all others can
assume it is fine.
Line 111: self.assertNotIn(NETWORK, nets)
Line 112:
Line 113: def test_create_net_with_bridge(self):
Line 114: self._test_create_net(
Line 131:
Line 132: @contextmanager
Line 133: def libvirt_net(net, net_def):
Line 134: try:
Line 135: libvirt.createNetwork(net_def)
If it failed during creation, what is there to remove?
Or can it fail and leave leftovers?
Line 136: yield
Line 137: finally:
https://gerrit.ovirt.org/#/c/65065/8/vdsm/virt/vmdevices/graphics.py
File vdsm/virt/vmdevices/graphics.py:
Even with
https://gerrit.ovirt.org/#/c/65098 , the displayNetwork will be broken now.
1. We cannot use IP address anymore for binding, as the addresses are different from host
to host.
2. As far as I remember, the libvirt translation between network/bridge name to an IP
address is not working with OVS bridges (please correct me by checking this), so the
display network cannot be resolved when using OVS libvirt network.
It was one of the reasons we convinced ourself that dropping libvirt db is a good step.
Line 1: #
Line 2: # Copyright 2008-2014 Red Hat, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
--
To view, visit
https://gerrit.ovirt.org/65065
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes