)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":8064,"name":"Jake Yip","email":"jake.yip@ardc.edu.au","username":"jake"},"change_message_id":"80bdc741ee3c76a25b32337bb807e30c893c2bdd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"99ef4343_ed6d8d4d","updated":"2025-06-24 08:42:41.000000000","message":"-1 for visibility","commit_id":"52acc1854574e10247ba6a4928614ba3af96622b"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"82e427e7d0ea7d5fdbbe28fbb23bb2b7ad1af0a1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"58d00444_e24ef81d","updated":"2025-06-30 11:06:03.000000000","message":"Looks sensible to me","commit_id":"fd8f03bcb404e3c6f2dfd970da30ba8e631e455d"}],"magnum_capi_helm/driver.py":[{"author":{"_account_id":8064,"name":"Jake Yip","email":"jake.yip@ardc.edu.au","username":"jake"},"change_message_id":"80bdc741ee3c76a25b32337bb807e30c893c2bdd","unresolved":true,"context_lines":[{"line_number":593,"context_line":"        return self._get_label_bool(cluster, \"auto_scaling_enabled\", False)"},{"line_number":594,"context_line":""},{"line_number":595,"context_line":"    def _get_autoscale_values(self, cluster, nodegroup):"},{"line_number":596,"context_line":"        # Cluster labels determine autoscaling enabled for any nodegroup."},{"line_number":597,"context_line":"        min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":598,"context_line":"        self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":599,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"cbbec8cc_4ad76f4f","line":596,"range":{"start_line":596,"start_character":0,"end_line":596,"end_character":2},"updated":"2025-06-24 08:42:41.000000000","message":"I am unsure what this means. do you mean \n1) the cluster label \u0027auto_scaling_enabled\u0027\n2) or the nodegroup properties determine autoscaling enabled (as with your comment below)","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"a5fa5b37fbc8f4f997f603f21a020ed4a944bce1","unresolved":false,"context_lines":[{"line_number":593,"context_line":"        return self._get_label_bool(cluster, \"auto_scaling_enabled\", False)"},{"line_number":594,"context_line":""},{"line_number":595,"context_line":"    def _get_autoscale_values(self, cluster, nodegroup):"},{"line_number":596,"context_line":"        # Cluster labels determine autoscaling enabled for any nodegroup."},{"line_number":597,"context_line":"        min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":598,"context_line":"        self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":599,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"78a9b179_ce2e2069","line":596,"range":{"start_line":596,"start_character":0,"end_line":596,"end_character":2},"in_reply_to":"cbbec8cc_4ad76f4f","updated":"2025-06-26 04:26:22.000000000","message":"It was `1)`, but this comment is not relevant in this function. Removed.","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"160e5ec546713f41a53b7d73aa365f731a9dad34","unresolved":true,"context_lines":[{"line_number":668,"context_line":""},{"line_number":669,"context_line":"        # If min/max node counts are not defined on the default"},{"line_number":670,"context_line":"        # worker group then fall back to equivalent cluster labels"},{"line_number":671,"context_line":"        if self._is_default_worker_nodegroup(cluster, nodegroup):"},{"line_number":672,"context_line":"            # NOTE(dalees): We do not want to override the"},{"line_number":673,"context_line":"            # nodegroup.min_node_count with label values since labels are"},{"line_number":674,"context_line":"            # already copied for default-worker."}],"source_content_type":"text/x-python","patch_set":2,"id":"902e9e73_dd9aa95c","line":671,"updated":"2025-06-10 21:12:19.000000000","message":"This may be a problem - existing clusters have non-default nodegroups created with labels inherited from cluster (or set directly) with the desired min_node_count and max_node_count.\n\nThe values of these nodegroups is the default of min_node_count\u003d0, max_node_count\u003dNone.\n\nSo to maintain backwards compatibility, it seems we need to default to labels for these, if the fields are invalid or default.","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"5444f2507966d07505e12d10147b045e663bc786","unresolved":false,"context_lines":[{"line_number":668,"context_line":""},{"line_number":669,"context_line":"        # If min/max node counts are not defined on the default"},{"line_number":670,"context_line":"        # worker group then fall back to equivalent cluster labels"},{"line_number":671,"context_line":"        if self._is_default_worker_nodegroup(cluster, nodegroup):"},{"line_number":672,"context_line":"            # NOTE(dalees): We do not want to override the"},{"line_number":673,"context_line":"            # nodegroup.min_node_count with label values since labels are"},{"line_number":674,"context_line":"            # already copied for default-worker."}],"source_content_type":"text/x-python","patch_set":2,"id":"84f362f7_d62b7591","line":671,"in_reply_to":"902e9e73_dd9aa95c","updated":"2025-06-11 00:12:22.000000000","message":"Updated to now apply the defaulting behaviour to all nodegroups.","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":8064,"name":"Jake Yip","email":"jake.yip@ardc.edu.au","username":"jake"},"change_message_id":"80bdc741ee3c76a25b32337bb807e30c893c2bdd","unresolved":true,"context_lines":[{"line_number":704,"context_line":"            f\"{(nodegroup.node_count, min_nodes, max_nodes)}\""},{"line_number":705,"context_line":"        )"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"        if min_nodes is None:"},{"line_number":708,"context_line":"            return"},{"line_number":709,"context_line":""},{"line_number":710,"context_line":"        # ClusterAPI Provider OpenStack (CAPO)"},{"line_number":711,"context_line":"        # doesn\u0027t support scale to zero yet."}],"source_content_type":"text/x-python","patch_set":2,"id":"ea83ee57_d0011c02","line":708,"range":{"start_line":707,"start_character":0,"end_line":708,"end_character":18},"updated":"2025-06-24 08:42:41.000000000","message":"is `min_nodes \u003d None` valid and when will that be? it seems to default to node_count.","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"a5fa5b37fbc8f4f997f603f21a020ed4a944bce1","unresolved":false,"context_lines":[{"line_number":704,"context_line":"            f\"{(nodegroup.node_count, min_nodes, max_nodes)}\""},{"line_number":705,"context_line":"        )"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"        if min_nodes is None:"},{"line_number":708,"context_line":"            return"},{"line_number":709,"context_line":""},{"line_number":710,"context_line":"        # ClusterAPI Provider OpenStack (CAPO)"},{"line_number":711,"context_line":"        # doesn\u0027t support scale to zero yet."}],"source_content_type":"text/x-python","patch_set":2,"id":"2b21adcd_939fecf7","line":708,"range":{"start_line":707,"start_character":0,"end_line":708,"end_character":18},"in_reply_to":"ea83ee57_d0011c02","updated":"2025-06-26 04:26:22.000000000","message":"Hmm agreed, removed.\n\nIt was a hangover from refactoring line 697: https://github.com/openstack/magnum-capi-helm/blob/60dc96c4dae8628e92c20b1ca594c4cf10eba5e4/magnum_capi_helm/driver.py#L697","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":8064,"name":"Jake Yip","email":"jake.yip@ardc.edu.au","username":"jake"},"change_message_id":"80bdc741ee3c76a25b32337bb807e30c893c2bdd","unresolved":true,"context_lines":[{"line_number":1132,"context_line":"    def create_nodegroup(self, context, cluster, nodegroup):"},{"line_number":1133,"context_line":"        nodegroup.status \u003d fields.ClusterStatus.CREATE_IN_PROGRESS"},{"line_number":1134,"context_line":"        self._validate_allowed_flavor(context, nodegroup.flavor_id)"},{"line_number":1135,"context_line":"        if self._get_autoscale_enabled(cluster):"},{"line_number":1136,"context_line":"            min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":1137,"context_line":"            self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":1138,"context_line":"        nodegroup.save()"}],"source_content_type":"text/x-python","patch_set":2,"id":"468b48fa_93ace00f","line":1135,"range":{"start_line":1135,"start_character":0,"end_line":1135,"end_character":2},"updated":"2025-06-24 08:42:41.000000000","message":"this is kind of weird because currently I can do `openstack coe nodegroup create --min-nodes 1 --max-nodes 0 \u003ccluster\u003e \u003cnodegroup\u003e`","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"a5fa5b37fbc8f4f997f603f21a020ed4a944bce1","unresolved":true,"context_lines":[{"line_number":1132,"context_line":"    def create_nodegroup(self, context, cluster, nodegroup):"},{"line_number":1133,"context_line":"        nodegroup.status \u003d fields.ClusterStatus.CREATE_IN_PROGRESS"},{"line_number":1134,"context_line":"        self._validate_allowed_flavor(context, nodegroup.flavor_id)"},{"line_number":1135,"context_line":"        if self._get_autoscale_enabled(cluster):"},{"line_number":1136,"context_line":"            min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":1137,"context_line":"            self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":1138,"context_line":"        nodegroup.save()"}],"source_content_type":"text/x-python","patch_set":2,"id":"7bdfdc05_89b19a0a","line":1135,"range":{"start_line":1135,"start_character":0,"end_line":1135,"end_character":2},"in_reply_to":"468b48fa_93ace00f","updated":"2025-06-26 04:26:22.000000000","message":"The actual `nodegroup create` is done in Magnum conductor via an async RPC call, which does limit the validation the driver can do.\n\nSo I would expect this to result in a new nodegroup created and ending up in `state: CREATE_FAILED` with `status_reason: Max node count must be greater than or equal to min node count`.\n\nMagnum Conductor also raises an Exception: `ERROR oslo_messaging.rpc.server magnum.common.exception.NodeGroupInvalidInput: Max node count must be greater than or equal to min node count`","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":8064,"name":"Jake Yip","email":"jake.yip@ardc.edu.au","username":"jake"},"change_message_id":"80bdc741ee3c76a25b32337bb807e30c893c2bdd","unresolved":true,"context_lines":[{"line_number":1134,"context_line":"        self._validate_allowed_flavor(context, nodegroup.flavor_id)"},{"line_number":1135,"context_line":"        if self._get_autoscale_enabled(cluster):"},{"line_number":1136,"context_line":"            min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":1137,"context_line":"            self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":1138,"context_line":"        nodegroup.save()"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        self._update_helm_release(context, cluster)"}],"source_content_type":"text/x-python","patch_set":2,"id":"84ce3f97_ef1dd65c","line":1137,"range":{"start_line":1137,"start_character":0,"end_line":1137,"end_character":2},"updated":"2025-06-24 08:42:41.000000000","message":"I think I\u0027m missing something - why is there a need to change this from `_validate_allowed_node_counts(cluster, nodegroup)` to `_validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)`?\n\nthis seems to result in every call needing an additional\n`min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)`?","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"a5fa5b37fbc8f4f997f603f21a020ed4a944bce1","unresolved":true,"context_lines":[{"line_number":1134,"context_line":"        self._validate_allowed_flavor(context, nodegroup.flavor_id)"},{"line_number":1135,"context_line":"        if self._get_autoscale_enabled(cluster):"},{"line_number":1136,"context_line":"            min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":1137,"context_line":"            self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":1138,"context_line":"        nodegroup.save()"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        self._update_helm_release(context, cluster)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9106beac_f4c18118","line":1137,"range":{"start_line":1137,"start_character":0,"end_line":1137,"end_character":2},"in_reply_to":"84ce3f97_ef1dd65c","updated":"2025-06-26 04:26:22.000000000","message":"Thanks - I see your point, definitely could be improved.\n\nMy initial reasoning was that the function `_validate_allowed_node_counts` was doing two jobs - validation and returning returning the final min and max values to be used.\n\nI want this function to be really clear about what it does and do one job, instead of being used for multiple purposes, however I\u0027m not sure I improved it, here.\n\nI\u0027ll revise the function refactor and split it into another commit so it can be reviewed separately, it\u0027s not relevant for this autoscaling min/max change even if I consider it addressing a code smell.","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"ff7d837093124ee016cf671131f570ea23e5e86d","unresolved":false,"context_lines":[{"line_number":1134,"context_line":"        self._validate_allowed_flavor(context, nodegroup.flavor_id)"},{"line_number":1135,"context_line":"        if self._get_autoscale_enabled(cluster):"},{"line_number":1136,"context_line":"            min_nodes, max_nodes \u003d self._get_node_counts(cluster, nodegroup)"},{"line_number":1137,"context_line":"            self._validate_allowed_node_counts(nodegroup, min_nodes, max_nodes)"},{"line_number":1138,"context_line":"        nodegroup.save()"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        self._update_helm_release(context, cluster)"}],"source_content_type":"text/x-python","patch_set":2,"id":"69b06f88_23cf1a5c","line":1137,"range":{"start_line":1137,"start_character":0,"end_line":1137,"end_character":2},"in_reply_to":"9106beac_f4c18118","updated":"2025-06-26 05:03:33.000000000","message":"Moved to I4bba6ccfda4bb188aba0fde9352e07a0348843e9","commit_id":"3051971a36ac69a74f09d020798da67a79ac5465"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"82e427e7d0ea7d5fdbbe28fbb23bb2b7ad1af0a1","unresolved":true,"context_lines":[{"line_number":676,"context_line":"        # TODO(dalees): This *should* be nodegroup label, not cluster."},{"line_number":677,"context_line":"        #               However, they are the same for default-worker."},{"line_number":678,"context_line":"        # For non-default nodegroups, cluster label is still used the field"},{"line_number":679,"context_line":"        # should be updated instead of setting labels."},{"line_number":680,"context_line":"        if min_nodes \u003d\u003d 0:"},{"line_number":681,"context_line":"            min_nodes \u003d self._get_label_int(cluster, \"min_node_count\", 0)"},{"line_number":682,"context_line":"        # Finally, if there is no max node count from any source, disable"}],"source_content_type":"text/x-python","patch_set":4,"id":"a9f82ec0_b6995b9b","line":679,"updated":"2025-06-30 11:06:03.000000000","message":"Is this comment missing a word?","commit_id":"fd8f03bcb404e3c6f2dfd970da30ba8e631e455d"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"df1d07ee14246442c2bb4aaea05ee4045b5809a0","unresolved":true,"context_lines":[{"line_number":676,"context_line":"        # TODO(dalees): This *should* be nodegroup label, not cluster."},{"line_number":677,"context_line":"        #               However, they are the same for default-worker."},{"line_number":678,"context_line":"        # For non-default nodegroups, cluster label is still used the field"},{"line_number":679,"context_line":"        # should be updated instead of setting labels."},{"line_number":680,"context_line":"        if min_nodes \u003d\u003d 0:"},{"line_number":681,"context_line":"            min_nodes \u003d self._get_label_int(cluster, \"min_node_count\", 0)"},{"line_number":682,"context_line":"        # Finally, if there is no max node count from any source, disable"}],"source_content_type":"text/x-python","patch_set":4,"id":"17997157_7aea01ef","line":679,"in_reply_to":"a9f82ec0_b6995b9b","updated":"2025-07-14 04:10:48.000000000","message":"thanks, I\u0027ll update or remove this.","commit_id":"fd8f03bcb404e3c6f2dfd970da30ba8e631e455d"},{"author":{"_account_id":36412,"name":"Scott Davidson","email":"sdavidson327@gmail.com","username":"sd109"},"change_message_id":"82e427e7d0ea7d5fdbbe28fbb23bb2b7ad1af0a1","unresolved":true,"context_lines":[{"line_number":678,"context_line":"        # For non-default nodegroups, cluster label is still used the field"},{"line_number":679,"context_line":"        # should be updated instead of setting labels."},{"line_number":680,"context_line":"        if min_nodes \u003d\u003d 0:"},{"line_number":681,"context_line":"            min_nodes \u003d self._get_label_int(cluster, \"min_node_count\", 0)"},{"line_number":682,"context_line":"        # Finally, if there is no max node count from any source, disable"},{"line_number":683,"context_line":"        # autoscaling by setting min/max to the same value."},{"line_number":684,"context_line":"        if max_nodes is None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"e7fc1fb5_9d42ee78","line":681,"updated":"2025-06-30 11:06:03.000000000","message":"Based on the above TODO, why can\u0027t we use nodegroup instead of cluster here?","commit_id":"fd8f03bcb404e3c6f2dfd970da30ba8e631e455d"},{"author":{"_account_id":14394,"name":"Dale Smith","email":"dale@catalystcloud.nz","username":"dalees"},"change_message_id":"df1d07ee14246442c2bb4aaea05ee4045b5809a0","unresolved":true,"context_lines":[{"line_number":678,"context_line":"        # For non-default nodegroups, cluster label is still used the field"},{"line_number":679,"context_line":"        # should be updated instead of setting labels."},{"line_number":680,"context_line":"        if min_nodes \u003d\u003d 0:"},{"line_number":681,"context_line":"            min_nodes \u003d self._get_label_int(cluster, \"min_node_count\", 0)"},{"line_number":682,"context_line":"        # Finally, if there is no max node count from any source, disable"},{"line_number":683,"context_line":"        # autoscaling by setting min/max to the same value."},{"line_number":684,"context_line":"        if max_nodes is None:"}],"source_content_type":"text/x-python","patch_set":4,"id":"8c9341c0_6201055c","line":681,"in_reply_to":"e7fc1fb5_9d42ee78","updated":"2025-07-14 04:10:48.000000000","message":"Yeah, I had some reasons here, but it leaves a strange behaviour when creating a new nodegroup (cluster labels from creation would be used instead of nodegroup labels, if the fields aren\u0027t set).\n\nI was avoiding adding a parameter to `self._get_label*()` for the nodegroup, but perhaps this is now necessary.","commit_id":"fd8f03bcb404e3c6f2dfd970da30ba8e631e455d"}]}