add support for availability zone in OVN cms options#82
add support for availability zone in OVN cms options#82mastier wants to merge 1 commit intoopenstack-charmers:masterfrom
Conversation
fnordahl
left a comment
fnordahl
left a comment
There was a problem hiding this comment.
The approach generally look OK, there are a couple of comments in-line.
Please also refer to staged gerrit reviews with passing tests for the ovn-chassis and ovn-dedicated-chassis charms consuming this layer change.
| underlying machine provider such as MAAS and this option allows the | ||
| charm to use JUJU_AVAILABILITY_ZONE to set default_availability_zone for | ||
| Nova nodes. This option overrides the default-availability-zone charm | ||
| config setting only when the Juju provider sets JUJU_AVAILABILITY_ZONE. |
There was a problem hiding this comment.
Line 307 and 308 appear to contain copy pasta that does not apply to the ovn-dedicated-chassis nor ovn-chassis charms.
There was a problem hiding this comment.
I don't see it, the text still mentions configuration options not available in these charms.
| self.refresh.assert_not_called() | ||
| self.remove.assert_not_called() | ||
|
|
||
| @mock.patch.dict(os.environ, {"JUJU_AVAILABILITY_ZONE": "az1"}, clear=True) |
There was a problem hiding this comment.
Please refrain from using the patch decorator. As you can see from the rest of this file, none of the test methods use it. Use the test_utils.PatchHelper.patch or test_utils.PatchHelper.patch_object methods instead.
There was a problem hiding this comment.
@fnordahl seems I cannot use it
for self.patch_object
==============================
Failed 1 tests - output below:
File "/usr/lib/python3.10/unittest/mock.py", line 1437, in __enter__
File "/usr/lib/python3.10/unittest/mock.py", line 1400, in get_original
original = target.__dict__[name]
TypeError: unhashable type: 'dict'
for self.patch
File "/usr/lib/python3.10/unittest/mock.py", line 1606, in _get_target
raise TypeError(
TypeError: Need a valid target to patch. You supplied: environ({'PYTHONIOENCODING': 'utf-8', 'PYTHONHASHSEED': '0', 'HOME': '/home/ubuntu', 'TOX_ENV_NAME': 'py3', 'CHARM_LAYERS_DIR': '/home/ubuntu/charm-layer-
ovn/layers', 'CHARM_INTERFACES_DIR': '/home/ubuntu/charm-layer-ovn/interfaces', 'PIP_DISABLE_PIP_VERSION_CHECK': '1', 'TERM': 'linux', 'JUJU_REPOSITORY': '/home/ubuntu/charm-layer-ovn/build', 'TOX_ENV_DIR': '/home
/ubuntu/charm-layer-ovn/.tox/py3', 'COLUMNS': '213', 'PATH': '/home/ubuntu/charm-layer-ovn/.tox/py3/bin:/home/ubuntu/.local/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/local/bin:/usr/bin:/bin:/usr/games:/usr/local/ga
mes:/snap/bin', 'LANG': 'en_US.UTF-8', 'TOX_WORK_DIR': '/home/ubuntu/charm-layer-ovn/.tox', 'VIRTUAL_ENV': '/home/ubuntu/charm-layer-ovn/.tox/py3', 'PWD': '/home/ubuntu/charm-layer-ovn', 'LINES': '38'})
There was a problem hiding this comment.
ok, @fnordahl
We cannot do it this ways
def test_configure_customize_failure_domain(self):
environ = copy.deepcopy(os.environ)
environ["JUJU_AVAILABILITY_ZONE"] = "az1"
self.patch_object(os, 'environ', return_value=environ)
self.target.options.customize_failure_domain = True
self.assertEqual(
self.target._get_ovn_cms_options(),
['enable-chassis-as-gw', 'availability-zones=az1'])
The mock won't get the values of the dict if called get()
File "/usr/lib/python3.10/os.py", line 651, in get_exec_path
raise ValueError(
ValueError: env cannot contain 'PATH' and b'PATH' keys
it will be empty.
We may need add test_utils.PatchHelper.patch_dict to the test_utils.
Otherwise this mock won't behave like dict
Or do you have better idea?
my suggestion
openstack/charms.openstack#6
There was a problem hiding this comment.
There is no need for a separate method to patch dicts, the generic provisions is sufficient.
You can for example use side_effect instead of return_value to provide your dict, or you could return a mock.MagicMock instance and attach the return value to get, or you could provide a helper function and provide that as a side effect.
There was a problem hiding this comment.
Ok. I was thinking about using side_effect, but I was not sure how to use it. Let me look at that and go back to you. Thank you.
There was a problem hiding this comment.
var = somevalue
self.patch_object(module, 'attribute')
self.attribute.side_effect = var
|
@fnordahl It seems I cannot use it on dict, when using self.patch when using self.patch_object |
You're invoking the Don't forget to address the comment about erroneous copy paste in the config option description. |
LP#2002449 That fix enables AZ support in OVN via external-ids:ovn-cms-options It relies on JUJU_AVAILABILITY_ZONE env variable.
2 participants
LP#2002449 That fix enables AZ support in OVN via external-ids:ovn-cms-options It relies on JUJU_AVAILABILITY_ZONE env variable.