Skip to content

add support for availability zone in OVN cms options #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mastier wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add support for availability zone in OVN cms options #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,16 @@ options:
.
NOTE: It is also possible to enable this option on several OVN chassis
applications at the same time, e.g. on 2 out of 3.
customize-failure-domain:
type: boolean
default: false
description: |
Juju propagates availability zone information to charms from the
underlying machine provider such as MAAS and this option allows the
charm to use JUJU_AVAILABILITY_ZONE to set default_availability_zone for
OVN nodes. This option overrides the default-availability-zone charm
config setting only when the Juju provider sets JUJU_AVAILABILITY_ZONE.
Copy link
Copy Markdown
Contributor

@fnordahl fnordahl Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 307 and 308 appear to contain copy pasta that does not apply to the ovn-dedicated-chassis nor ovn-chassis charms.

Copy link
Copy Markdown
Contributor Author

@mastier mastier Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed that now @fnordahl

Copy link
Copy Markdown
Contributor

@fnordahl fnordahl Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it, the text still mentions configuration options not available in these charms.

More details: https://docs.openstack.org/neutron/latest/admin/ovn/availability_zones.html
nagios_context:
default: "juju"
type: string
Expand Down
4 changes: 4 additions & 0 deletions lib/charms/ovn_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,10 @@ def _get_ovn_cms_options(self):
if self.options.card_serial_number:
cms_opts.append(
f'card-serial-number={self.options.card_serial_number}')
if self.options.customize_failure_domain and \
'JUJU_AVAILABILITY_ZONE' in os.environ:
cms_opts.append(
f'availability-zones={os.environ["JUJU_AVAILABILITY_ZONE"]}')
return cms_opts

def render_nrpe(self):
Expand Down
10 changes: 10 additions & 0 deletions unit_tests/test_lib_charms_ovn_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import collections
import copy
import io
import os
import textwrap
import subprocess
import unittest.mock as mock
Expand Down Expand Up @@ -764,6 +765,7 @@ def setUp(self):
'vpd-device-spec': '',
'pmd-cpu-set': '',
'ovn-source': 'distro',
'customize-failure-domain': False,
})
self.patch_object(ovn_charm.OVNConfigurationAdapter,
'_ovs_dpdk_cpu_overlap_check')
Expand Down Expand Up @@ -1097,6 +1099,7 @@ def setUp(self):
'[{"bus": "pci", "vendor_id": "beef", "device_id": "cafe"}]',
'ovn-source': 'distro',
'ovs-exporter-channel': '',
'customize-failure-domain': False,
})

def test_optional_openstack_metadata(self):
Expand Down Expand Up @@ -1734,6 +1737,13 @@ def test_assess_exporter_no_channel_not_installed(self):
self.refresh.assert_not_called()
self.remove.assert_not_called()

@mock.patch.dict(os.environ, {"JUJU_AVAILABILITY_ZONE": "az1"}, clear=True)
Copy link
Copy Markdown
Contributor

@fnordahl fnordahl Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@mastier mastier Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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'})

Copy link
Copy Markdown
Contributor Author

@mastier mastier Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@fnordahl fnordahl Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@mastier mastier Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@fnordahl fnordahl Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var = somevalue
self.patch_object(module, 'attribute')
self.attribute.side_effect = var

def test_configure_customize_failure_domain(self):
self.target.options.customize_failure_domain = True
self.assertEqual(
self.target._get_ovn_cms_options(),
['enable-chassis-as-gw', 'availability-zones=az1'])


class TestOVNChassisCharmOvsExporter(Helper):

Expand Down