[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

@@ -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.

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)

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

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.