)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"e165bdcf6088436afa9802e85f91476a59801061","unresolved":false,"context_lines":[{"line_number":10,"context_line":""},{"line_number":11,"context_line":"API to get nodes list based on a given node group"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"2. GET /\u003cCluster\u003e/nodes/\u003cServerID\u003e"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"API to get a node detailed info based on cluster ID"},{"line_number":16,"context_line":"and server ID."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f493fa4_333adce6","line":13,"updated":"2020-05-05 04:55:02.000000000","message":"@Thomas, can you please help me understand if these two API endpoints you\u0027re expecting from Magnum side?\n\nAnd it would be nice if you can help me understand the response body format and the node object.\n\nTBH, a single /\u003cCluster\u003e/nodes/\u003cServerID\u003e is a bit weird for me without the support /\u003cCluster\u003e/nodes. Personally, I\u0027d like to have /\u003cCluster\u003e/nodes instead of /\u003cCluster\u003e/nodegroups/\u003cNodeGroup\u003e/nodes. To get node list of a particular NG, user should use /\u003cCluster\u003e/nodes?nodegroup\u003d\u003cNodeGroup\u003e, which makes more sense for me.\n\nHow do you think? @Spyros","commit_id":"e31b43e9ee61cc89fd26748a67d3218781d30b4b"},{"author":{"_account_id":30262,"name":"Thomas Hartland","email":"thomas.george.hartland@cern.ch","username":"tghartland"},"change_message_id":"3462348f99b21f40fdac52608594596cf50eebfd","unresolved":false,"context_lines":[{"line_number":10,"context_line":""},{"line_number":11,"context_line":"API to get nodes list based on a given node group"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"2. GET /\u003cCluster\u003e/nodes/\u003cServerID\u003e"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"API to get a node detailed info based on cluster ID"},{"line_number":16,"context_line":"and server ID."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f493fa4_9c62d8d7","line":13,"in_reply_to":"1f493fa4_333adce6","updated":"2020-05-05 10:38:48.000000000","message":"When listing nodes in a node group, the autoscaler\u0027s cloud provider code should return a list of the type \"Instance\" [1], the important fields are:\n\n* ProviderID (openstack:///\u003cserver-id\u003e)\n* State (Creating, Running, Deleting)\n* Error class (Out of quota or Other)\n* Error message\n\nSo what you have in Node.convert (\"uuid\", \"name\", \"node_group\", \"status\") has everything except a status reason for the error message.\n\nTo get the state in the autoscaler, the first thing I check is the statuses in the kube minions resource list (e.g openstack stack resource list kube-w5axvlfrz5lr-kube_minions-vv7bsojktpi7). But this does not exactly map to the three states expected by the autoscaler. Since all minion stacks go to UPDATE_IN_PROGRESS when resizing a node group, that state could be a creating node or a running node.\n\nTo figure out which state an UPDATE_IN_PROGRESS node is, I have to then check the stack resources of the minion itself, like [2], and check if \"kube-minion\" and \"node_config_deployment\" are complete. This makes checking this one status quite a long case [3], and I don\u0027t know how it would work with this API we\u0027re proposing. Maybe listing nodes could return the stack_status but also a node_state that is one of the three expected by the autoscaler?\n\n---\n\nRegarding cluster/nodes or cluster/nodegroup/nodes, I think the first is fine as long as the ?nodegroup\u003dxyz path works essentially like the second option. It shouldn\u0027t list everything and then filter the list, it should only query the requested node group.\n\nI\u0027m not sure how useful having /cluster/nodes/\u003cserver-id\u003e would be, at least right now. With what you have written so far, the only fields not being returned in the converted nodes list are the cluster ID and project ID, which are both already known. It would have to search through multiple node groups to find the requested node anyway, so at least for the autoscaler it would be more efficient to just list all the nodes and cache the node group IDs of all nodes, instead of making multiple calls to this API with a single node each time.\n\n---\n\n[1] https://github.com/kubernetes/autoscaler/blob/f11443819f46e934eebc7c87916aa8559acba083/cluster-autoscaler/cloudprovider/cloud_provider.go#L175-L222\n\n[2] openstack stack resource list $(openstack stack resource show kube-w5axvlfrz5lr-kube_minions-vv7bsojktpi7 0 -f value -c physical_resource_id)\n\n[3] https://github.com/tghartland/autoscaler/blob/eba060ff2c02bf10172441fbd0eec6ea20fa333a/cluster-autoscaler/cloudprovider/magnum/magnum_manager_resize.go#L310-L370","commit_id":"e31b43e9ee61cc89fd26748a67d3218781d30b4b"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"029ec80a42a02e3efe2625f8233f0d968876a29b","unresolved":false,"context_lines":[{"line_number":10,"context_line":""},{"line_number":11,"context_line":"API to get nodes list based on a given node group"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"2. GET /\u003cCluster\u003e/nodes/\u003cServerID\u003e"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"API to get a node detailed info based on cluster ID"},{"line_number":16,"context_line":"and server ID."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f493fa4_4434e94a","line":13,"in_reply_to":"1f493fa4_9c62d8d7","updated":"2020-05-06 03:54:45.000000000","message":"Thanks, Thomas. That\u0027s a very clear and detailed response. I really appreciate that. I will propose a new patchset based on your comments. Thank you.","commit_id":"e31b43e9ee61cc89fd26748a67d3218781d30b4b"}],"magnum/api/controllers/v1/cluster_actions.py":[{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"a027f4dc3a3fca85efab3e2ad464265958ad0877","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_8d62f6d6","line":82,"updated":"2020-04-30 08:32:20.000000000","message":"How about just use \"list\" or \"nodes\"?\nAnd are you guys happy to put the nodes listing as an action of a cluster?","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":30262,"name":"Thomas Hartland","email":"thomas.george.hartland@cern.ch","username":"tghartland"},"change_message_id":"3e5da48c3682b188d6269daa5d35fbff0f92684c","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_82044a80","line":82,"in_reply_to":"1f493fa4_02115a54","updated":"2020-05-04 14:53:48.000000000","message":"It\u0027s better to rely on information from the Magnum API rather than kubernetes node labels (which can be modified).\n\nFor reference, by default (1) is called for every node group every 2 minutes, and every 10 seconds for any node groups that are scaling up.\n\n(2) only needs to be called once per scale up ideally, or in the worst case once for each new node node per scale up (if there is a delay between each node being created). The result can then be cached forever.\n\nThat\u0027s why I suggest to prioritise querying a single node group.","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"4a326b527e3fb19dd461bfbda42bd0bb7d2c020b","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_f0f35ce6","line":82,"in_reply_to":"1f493fa4_2dec4ff2","updated":"2020-05-04 08:49:17.000000000","message":"@Spyros, I actually have already got a following patchset locally. But I\u0027m thinking the samething. We may won\u0027t create a new object dedicated for \"node\". In other words, we may need to build the nodes list in memory. Are you guys happy with this?","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":30262,"name":"Thomas Hartland","email":"thomas.george.hartland@cern.ch","username":"tghartland"},"change_message_id":"38372d7e8d69a91982b0439579a959998875ec6f","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_a7035873","line":82,"in_reply_to":"1f493fa4_7c93a301","updated":"2020-05-04 14:15:19.000000000","message":"There are two requirements for the autoscaler.\n\n(1) List all the nodes for a given node group.\n(2) Find the node group for a given node (by server ID).\n\nFor (1) it must be called often to check the current state, but the result of (2) can be cached so it only needs to be called when there is a new node added.\n\nGiven these requirements and considering the complexity/performance, it would be better to restrict the query to a single node group.\n\nPoint (2) would require looping over all node groups, but it can be done intelligently (check largest node groups first, cache as much as possible on each call) to minimise the number of calls.","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"4c7f598d2d261f6689c2af621506c55fe2607af0","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_d31d7874","line":82,"in_reply_to":"1f493fa4_82044a80","updated":"2020-05-05 05:03:36.000000000","message":"Team, please see my comments in PS3.","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":20498,"name":"Spyros Trigazis","email":"spyridon.trigazis@cern.ch","username":"strigazi"},"change_message_id":"f522ddc5f1b539386d9ea46a1338c3f05e50cf0f","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_cd8cfe37","line":82,"in_reply_to":"1f493fa4_8d62f6d6","updated":"2020-04-30 08:50:01.000000000","message":"FYI, the Nodes() method in the autoscaler is part of nodegroup. https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/cloud_provider.go#L105\n\nI think we primarily need it to list nodes of nodegroup.\n\n\nlist_nodes is fine","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":20498,"name":"Spyros Trigazis","email":"spyridon.trigazis@cern.ch","username":"strigazi"},"change_message_id":"f9b701d0ee2d05523779e351633797f19624b831","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_ef726949","line":82,"in_reply_to":"1f493fa4_96331e14","updated":"2020-05-02 13:12:10.000000000","message":"I think the functinality for the autoscaler is more important than the name at this point.\n\nThis is an API call the user shouln\u0027t need to do. If this is adverised to users more features will be requested. Imagine when you sell this API to the users and they request magnum\u0027s API to support PATCH.","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":20498,"name":"Spyros Trigazis","email":"spyridon.trigazis@cern.ch","username":"strigazi"},"change_message_id":"ae36e839ce7e84b331b7bd2a7c6bf9a501bba72d","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_02115a54","line":82,"in_reply_to":"1f493fa4_a7035873","updated":"2020-05-04 14:27:41.000000000","message":"For point (2) could we \"ask\" the kubernetes API about it? The kubernetes API knows the serverID and we can label each node with the nodegroup. It is not there yet, but it can be easily added. I think it makes sense since the server UUID is discovered from the node.\n\n\n(2) NodeGroupForNode(*apiv1.Node) (NodeGroup, error) https://github.com/kubernetes/autoscaler/blob/73a5cdf928d3b04ac5cbc456a60d5eb084f9cbc1/cluster-autoscaler/cloudprovider/cloud_provider.go#L60","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":20498,"name":"Spyros Trigazis","email":"spyridon.trigazis@cern.ch","username":"strigazi"},"change_message_id":"ad000d97623a47745dfa7d55b28b995c245df1ba","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_2dec4ff2","line":82,"in_reply_to":"1f493fa4_cd8cfe37","updated":"2020-05-04 07:45:55.000000000","message":"\u003e FYI, the Nodes() method in the autoscaler is part of nodegroup.\n \u003e https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/cloud_provider.go#L105\n \u003e \n \u003e I think we primarily need it to list nodes of nodegroup.\n \u003e \n \u003e \n \u003e list_nodes is fine\n\nAdditionally, since we have different stacks for different NGs, getting the nodes for all NGs will require multiple calls to heat and a transient state being build in memory.\n\n\nAny feedback based in my comment and not one names? (We can do nodes and stop discussing it.)","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":20498,"name":"Spyros Trigazis","email":"spyridon.trigazis@cern.ch","username":"strigazi"},"change_message_id":"3774fddc6d7393588df4badf263eca0090070bed","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_7c93a301","line":82,"in_reply_to":"1f493fa4_cd8cfe37","updated":"2020-05-04 13:43:33.000000000","message":"\u003e FYI, the Nodes() method in the autoscaler is part of nodegroup.\n \u003e https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/cloud_provider.go#L105\n \u003e \n \u003e I think we primarily need it to list nodes of nodegroup.\n \u003e \n \u003e \n \u003e list_nodes is fine\n\nSo I ask again, why diverge from the direction given by the autoscaler API and do a more error prone implementation?\n\nA client can do:\na.\nget all nodes for a given call\n\nb.\niterate over ngs and get all cluster nodes\neg. the autoscaler can do:\nNodeGroups() []NodeGroup https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/cloud_provider.go#L55\n\nand then\n\nNodes() ([]Instance, error) https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/cloud_provider.go#L145\n\nThe reverse lookup can be done from kubernetes (we have the nodes labeled) or the autoscaler can get all nodes.\n\n\nFeilong, Thomas, what do you think?","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":28022,"name":"Bharat Kunwar","email":"brtknr@bath.edu","username":"brtknr"},"change_message_id":"9dfb94b97c329483a54c39aaa7f2803fe63c809d","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_96331e14","line":82,"in_reply_to":"1f493fa4_cd8cfe37","updated":"2020-04-30 14:54:41.000000000","message":"or just \"nodes\"","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"},{"author":{"_account_id":6484,"name":"Feilong Wang","email":"hustemb@gmail.com","username":"flwang"},"change_message_id":"91be5502ede3c9a6964e393c7a725d2a145f76d6","unresolved":false,"context_lines":[{"line_number":79,"context_line":"    _custom_actions \u003d {"},{"line_number":80,"context_line":"        \u0027resize\u0027: [\u0027POST\u0027],"},{"line_number":81,"context_line":"        \u0027upgrade\u0027: [\u0027POST\u0027],"},{"line_number":82,"context_line":"        \u0027list_nodes\u0027: [\u0027GET\u0027],"},{"line_number":83,"context_line":"    }"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @base.Controller.api_version(\"1.7\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"1f493fa4_a1ecd129","line":82,"in_reply_to":"1f493fa4_d31d7874","updated":"2020-05-05 08:57:03.000000000","message":"Comments in PS2 commit message. Thanks.","commit_id":"b0a99778d91859d9970e709bc113f920918f4018"}]}