From 55fd12a47caba23db96a34a8a8ee603cae12f841 Mon Sep 17 00:00:00 2001 From: Bharat Kunwar Date: Fri, 5 Jun 2020 11:44:25 +0000 Subject: [PATCH] [fix] Use default_ng_worker.node_count for patches At present, when a cluster is patched without node_count (e.g. when health_status and health_status_reason are patched), the node_count in the cluster_update invocation contains cluster.node_count instead of default_ng_worker.node_count which causes unexpected behaviour when a cluster has additional nodegroups (e.g. the default-worker size gets incremented by 1 if the additional nodegroup has 1 worker each time the cluster is patched). This PS fixes this issue by using default_ng_worker.node_count if path "/node_count" is not present in the patch object. Story: 2005266 Task: 39962 Change-Id: I0df5149860604f0adf52701df371e34a6e667b9d --- doc/source/user/index.rst | 24 ++++++----- magnum/api/controllers/v1/cluster.py | 12 +++++- .../unit/api/controllers/v1/test_cluster.py | 40 +++++++++++++++++++ ...ng-worker-node-count-a88911a0b7a760a7.yaml | 6 +++ 4 files changed, 70 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/default-ng-worker-node-count-a88911a0b7a760a7.yaml diff --git a/doc/source/user/index.rst b/doc/source/user/index.rst index 2d3c475c84..fe93028dce 100644 --- a/doc/source/user/index.rst +++ b/doc/source/user/index.rst @@ -700,17 +700,19 @@ follows. 'discovery_url' cannot be replaced or delete. The table below summarizes the possible change to a cluster. - +---------------+-----+------------------+-----------------------+ - | Attribute | add | replace | remove | - +===============+=====+==================+=======================+ - | node_count | no | add/remove nodes | reset to default of 1 | - +---------------+-----+------------------+-----------------------+ - | master_count | no | no | no | - +---------------+-----+------------------+-----------------------+ - | name | no | no | no | - +---------------+-----+------------------+-----------------------+ - | discovery_url | no | no | no | - +---------------+-----+------------------+-----------------------+ + +---------------+-----+-------------------+-----------------------+ + | Attribute | add | replace | remove | + +===============+=====+===================+=======================+ + | node_count | no | add/remove nodes | reset to default of 1 | + | | | in default-worker | | + | | | nodegroup. | | + +---------------+-----+-------------------+-----------------------+ + | master_count | no | no | no | + +---------------+-----+-------------------+-----------------------+ + | name | no | no | no | + +---------------+-----+-------------------+-----------------------+ + | discovery_url | no | no | no | + +---------------+-----+-------------------+-----------------------+ The 'cluster-update' operation cannot be initiated when another operation is in progress. diff --git a/magnum/api/controllers/v1/cluster.py b/magnum/api/controllers/v1/cluster.py index 6caf99ad25..7d95d55617 100755 --- a/magnum/api/controllers/v1/cluster.py +++ b/magnum/api/controllers/v1/cluster.py @@ -629,7 +629,17 @@ class ClustersController(base.Controller): delta.add(field) validation.validate_cluster_properties(delta) - return (cluster, new_cluster.node_count, + + # NOTE(brtknr): cluster.node_count is the size of the whole cluster + # which includes non-default nodegroups. However cluster_update expects + # node_count to be the size of the default_ng_worker therefore return + # this value unless the patch object says otherwise. + node_count = cluster.default_ng_worker.node_count + for p in patch: + if p['path'] == '/node_count': + node_count = p.get('value') or new_cluster.node_count + + return (cluster, node_count, new_cluster.health_status, new_cluster.health_status_reason) @expose.expose(None, types.uuid_or_name, status_code=204) diff --git a/magnum/tests/unit/api/controllers/v1/test_cluster.py b/magnum/tests/unit/api/controllers/v1/test_cluster.py index d6c8fa642b..29c2483c8b 100644 --- a/magnum/tests/unit/api/controllers/v1/test_cluster.py +++ b/magnum/tests/unit/api/controllers/v1/test_cluster.py @@ -14,6 +14,7 @@ import datetime from unittest import mock from oslo_config import cfg +from oslo_serialization import jsonutils from oslo_utils import timeutils from oslo_utils import uuidutils from wsme import types as wtypes @@ -27,6 +28,7 @@ from magnum import objects from magnum.tests import base from magnum.tests.unit.api import base as api_base from magnum.tests.unit.api import utils as apiutils +from magnum.tests.unit.db import utils as db_utils from magnum.tests.unit.objects import utils as obj_utils CONF = magnum.conf.CONF @@ -287,6 +289,8 @@ class TestPatch(api_base.FunctionalTest): def _sim_rpc_cluster_update(self, cluster, node_count, health_status, health_status_reason, rollback=False): cluster.status = 'UPDATE_IN_PROGRESS' + cluster.health_status = health_status + cluster.health_status_reason = health_status_reason default_ng_worker = cluster.default_ng_worker default_ng_worker.node_count = node_count default_ng_worker.save() @@ -316,6 +320,42 @@ class TestPatch(api_base.FunctionalTest): self.assertEqual(self.cluster_obj.cluster_template_id, response['cluster_template_id']) + @mock.patch('oslo_utils.timeutils.utcnow') + def test_replace_health_status_ok(self, mock_utcnow): + new_health_status = 'HEALTHY' + new_health_status_reason = {'api': 'ok'} + health_status_reason_dumps = jsonutils.dumps(new_health_status_reason) + test_time = datetime.datetime(2000, 1, 1, 0, 0) + mock_utcnow.return_value = test_time + old_node_count = self.cluster_obj.default_ng_worker.node_count + db_utils.create_test_nodegroup(cluster_id=self.cluster_obj.uuid, + name='non_default_ng') + response = self.patch_json('/clusters/%s' % self.cluster_obj.uuid, + [{'path': '/health_status', + 'value': new_health_status, + 'op': 'replace'}, + {'path': '/health_status_reason', + 'value': health_status_reason_dumps, + 'op': 'replace'}]) + self.assertEqual('application/json', response.content_type) + self.assertEqual(202, response.status_code) + + response = self.get_json('/clusters/%s' % self.cluster_obj.uuid) + self.assertEqual(new_health_status, response['health_status']) + self.assertEqual(new_health_status_reason, + response['health_status_reason']) + + new_node_count = self.cluster_obj.default_ng_worker.node_count + self.assertEqual(old_node_count, new_node_count) + + return_updated_at = timeutils.parse_isotime( + response['updated_at']).replace(tzinfo=None) + self.assertEqual(test_time, return_updated_at) + # Assert nothing else was changed + self.assertEqual(self.cluster_obj.uuid, response['uuid']) + self.assertEqual(self.cluster_obj.cluster_template_id, + response['cluster_template_id']) + @mock.patch('oslo_utils.timeutils.utcnow') def test_replace_ok_by_name(self, mock_utcnow): new_node_count = 4 diff --git a/releasenotes/notes/default-ng-worker-node-count-a88911a0b7a760a7.yaml b/releasenotes/notes/default-ng-worker-node-count-a88911a0b7a760a7.yaml new file mode 100644 index 0000000000..f0a78a2905 --- /dev/null +++ b/releasenotes/notes/default-ng-worker-node-count-a88911a0b7a760a7.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes an edge case where when a cluster with additional nodegroups is + patched with health_status and health_status_reason, it was leading to the + default-worker nodegroup being resized.