From 56a0fbfab7702f357ca977dd2a37baee388da217 Mon Sep 17 00:00:00 2001 From: Enhao Cui Date: Tue, 16 Feb 2021 18:00:09 -0800 Subject: [PATCH] Add Support for Updating Policy Resource with PUT NSX checks revision number for PUT requests. It rejects the request if revision number is not latest. This is helpful for preventing clients overwriting each other's change to the same object concurrently. Change-Id: I226782f268b129a8e086938d8ebf258c2abc017e (cherry picked from commit 4643ed66475c689e6a82359a5d8820944350580c) --- .../tests/unit/v3/policy/test_resources.py | 69 ++++++++++++++++++- vmware_nsxlib/v3/policy/core_defs.py | 15 ++++ vmware_nsxlib/v3/policy/core_resources.py | 51 +++++++++----- 3 files changed, 115 insertions(+), 20 deletions(-) diff --git a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py index 677d66c9..100205eb 100644 --- a/vmware_nsxlib/tests/unit/v3/policy/test_resources.py +++ b/vmware_nsxlib/tests/unit/v3/policy/test_resources.py @@ -87,6 +87,13 @@ class NsxPolicyLibTestCase(policy_testcase.TestPolicyApi): actual_dict = mock_api.call_args_list[call_num][0][0].body self.assertEqual(expected_dict, actual_dict) + def assert_called_with_def_and_kwargs(self, mock_api, expected_def, + call_num=0, **kwargs): + # verify the api & resource definition + self.assert_called_with_def(mock_api, expected_def, call_num=call_num) + actual_kwargs = mock_api.call_args_list[call_num][1] + self.assertDictEqual(actual_kwargs, kwargs) + def mock_get(self, obj_id, obj_name, **kwargs): obj_dict = { 'id': obj_id, @@ -3685,7 +3692,7 @@ class TestPolicyTier0Bgp(NsxPolicyLibTestCase): self.assert_called_with_def(api_call, expected_def) self.assertIsNone(result) - def test_update(self): + def test_update_patch(self): name = 'test' description = 'bgp' tier0_id = 't0' @@ -3740,6 +3747,66 @@ class TestPolicyTier0Bgp(NsxPolicyLibTestCase): self.assert_called_with_def(api_call, expected_def) self.assertIsNone(result) + def test_update_put(self): + name = 'test' + description = 'bgp' + tier0_id = 't0' + service_id = "default", + ecmp = True, + enabled = True, + graceful_restart_config = { + "mode": "DISABLE", + "timer": { + "restart_timer": 180, + "stale_route_timer": 600 + } + } + inter_sr_ibgp = False, + local_as_num = "65546", + multipath_relax = False, + route_aggregations = [{ + "prefix": "10.1.1.0/24"}, { + "prefix": "11.1.0.0/16", "summary_only": "false"}] + tags = [{"tag": "tag", "scope": "scope"}] + tenant = TEST_TENANT + with mock.patch.object(self.policy_api, "get", + return_value={}),\ + mock.patch.object(self.policy_api, + "update_with_put") as api_call: + result = self.resourceApi.update( + tier0_id, service_id, + name=name, + description=description, + ecmp=ecmp, + enabled=enabled, + graceful_restart_config=graceful_restart_config, + inter_sr_ibgp=inter_sr_ibgp, + local_as_num=local_as_num, + multipath_relax=multipath_relax, + route_aggregations=route_aggregations, + tags=tags, + tenant=tenant, + put=True, + revision=1) + expected_def = core_defs.BgpRoutingConfigDef( + tier0_id=tier0_id, + service_id=service_id, + name=name, + description=description, + ecmp=ecmp, + enabled=enabled, + graceful_restart_config=graceful_restart_config, + inter_sr_ibgp=inter_sr_ibgp, + local_as_num=local_as_num, + multipath_relax=multipath_relax, + route_aggregations=route_aggregations, + tags=tags, + tenant=tenant + ) + self.assert_called_with_def_and_kwargs( + api_call, expected_def, revision=1) + self.assertIsNotNone(result) + def test_get(self): tier0_id = 't0' service_id = 'default' diff --git a/vmware_nsxlib/v3/policy/core_defs.py b/vmware_nsxlib/v3/policy/core_defs.py index 505ec331..32a193a8 100644 --- a/vmware_nsxlib/v3/policy/core_defs.py +++ b/vmware_nsxlib/v3/policy/core_defs.py @@ -2485,6 +2485,21 @@ class NsxPolicyApi(object): def partial_updates_supported(self): return self.partial_updates + def update_with_put(self, resource_def, revision=None): + """Create or update a policy object with PUT + + During concurrent read-updates, using PUT can be safer as it requires + revision number in the object. Policy API rejects the request if + revision number is not latest + """ + path = resource_def.get_resource_path() + if resource_def.resource_use_cache(): + self.cache.remove(path) + body = resource_def.get_obj_dict() + if revision is not None: + body.update({"_revision": revision}) + return self.client.update(path, body) + def create_or_update(self, resource_def, partial_updates=False, force=False): """Create or update a policy object. diff --git a/vmware_nsxlib/v3/policy/core_resources.py b/vmware_nsxlib/v3/policy/core_resources.py index c95e144b..2a7c90e9 100644 --- a/vmware_nsxlib/v3/policy/core_resources.py +++ b/vmware_nsxlib/v3/policy/core_resources.py @@ -141,14 +141,19 @@ class NsxPolicyResourceBase(object, metaclass=abc.ABCMeta): return resource_def - def _update(self, allow_partial_updates=True, force=False, **kwargs): + def _update(self, allow_partial_updates=True, + force=False, put=False, revision=None, **kwargs): """Helper for update function - ignore attrs without explicit value""" + # DO NOT retry if caller specifies revision + max_attempts = (self.policy_api.client.max_attempts + if revision is None else 0) + @utils.retry_upon_exception( exceptions.StaleRevision, - max_attempts=self.policy_api.client.max_attempts) + max_attempts=max_attempts) def _do_update_with_retry(): if (allow_partial_updates and - self.policy_api.partial_updates_supported()): + self.policy_api.partial_updates_supported() and not put): policy_def = self._init_def(**kwargs) partial_updates = True else: @@ -158,8 +163,12 @@ class NsxPolicyResourceBase(object, metaclass=abc.ABCMeta): if policy_def.bodyless(): # Nothing to update - only keys provided in kwargs return - self.policy_api.create_or_update( - policy_def, partial_updates=partial_updates, force=force) + if put: + return self.policy_api.update_with_put( + policy_def, revision=revision) + else: + self.policy_api.create_or_update( + policy_def, partial_updates=partial_updates, force=force) return _do_update_with_retry() @@ -1774,20 +1783,24 @@ class NsxPolicyTier0BgpApi(NsxPolicyResourceBase): multipath_relax=IGNORE, route_aggregations=IGNORE, tags=IGNORE, - tenant=constants.POLICY_INFRA_TENANT): - self._update(name=name, - description=description, - tier0_id=tier0_id, - service_id=service_id, - ecmp=ecmp, - enabled=enabled, - graceful_restart_config=graceful_restart_config, - inter_sr_ibgp=inter_sr_ibgp, - local_as_num=local_as_num, - multipath_relax=multipath_relax, - route_aggregations=route_aggregations, - tags=tags, - tenant=tenant) + tenant=constants.POLICY_INFRA_TENANT, + put=False, + revision=None): + return self._update(name=name, + description=description, + tier0_id=tier0_id, + service_id=service_id, + ecmp=ecmp, + enabled=enabled, + graceful_restart_config=graceful_restart_config, + inter_sr_ibgp=inter_sr_ibgp, + local_as_num=local_as_num, + multipath_relax=multipath_relax, + route_aggregations=route_aggregations, + tags=tags, + tenant=tenant, + put=put, + revision=revision) class NsxPolicyTier0NatRuleApi(NsxPolicyResourceBase):