AKS: making the count field optional
#7746
Conversation
tombuildsstuff
changed the title
count field optiona;count field optional
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
|
Can one of the admins verify this patch? |
|
@jluk FYI. On reflection given only a subset of fields can be updated for both AKS and a Node Pool, would it make sense for Update to have a more specific API endpoint/Swagger definition at some point? |
...erservice/resource-manager/Microsoft.ContainerService/stable/2019-06-01/managedClusters.json
Outdated
Show resolved
Hide resolved
...erservice/resource-manager/Microsoft.ContainerService/stable/2019-06-01/managedClusters.json
Show resolved
Hide resolved
|
thanks @gtxistxgao for review, + @xizhamsft for vis @tombuildsstuff having that discussion with a few internal folks wrt a separated upgrade API. let me get back to you on that one. |
tombuildsstuff
force-pushed
the
b/aks-node-pool-count
branch
from
1a7f92e
to
ec9bb83
Compare
|
@tombuildsstuff speaking with @gtxistxgao and @xizhamsft i don't think you'll need this PR anymore as the originally set agentpool count will now be ignored when CA is enabled so you shouldn't run into conflicts. |
Based on the test run yesterday this is still needed, since the
As such I think this is still needed? |
| @@ -676,6 +676,9 @@ | |||
| "x-ms-examples": { | |||
| "Create/Update Agent Pool": { | |||
| "$ref": "./examples/AgentPoolsCreate_Update.json" | |||
There was a problem hiding this comment.
do we need to update the json file name? since we separate create and update scenario now if I understand correctly.
There was a problem hiding this comment.
these are different examples - one covering Create & Update (using a full object) and another covering a Delta Update, where these optional fields aren't specified
In this example the count field is required for a manually scaled cluster (in the Create/Update example) - but cannot be sent for an automatic cluster (in the Update example)
There was a problem hiding this comment.
There is no difference between using full object and delta update in Updating scenario. Using full object, the "count" and "vmsize" is also not a required value.
There was a problem hiding this comment.
May be better to remove the Update in the AgentPoolsCreate_Update.json since it does not applies to Update. But it's out of this PR's scope.
|
Looks like the PR is good to merge after last open comment is resolved |
| "type": "Microsoft.ContainerService/managedClusters/agentPools", | ||
| "name": "agentpool1", | ||
| "properties": { | ||
| "provisioningState": "Succeeded", |
There was a problem hiding this comment.
If this is an example of updating agent pool, the provisioningState in the response could be "Updating", "Upgrading". It cannot be "Succeed"
There was a problem hiding this comment.
this is copied from the Create/Update response - so I'd suggest if that doesn't match the API definition it'd be worth the service team opening a separate PR for that, since it sounds like the API definition could be out of date?
...esource-manager/Microsoft.ContainerService/stable/2019-06-01/examples/AgentPools_Update.json
Outdated
Show resolved
Hide resolved
|
👋 is there anything else needed from our side to get this merged? Thanks! |
This PR makes the
countfield within the Node Pool Properties for a Kubernetes Cluster optional - since whilst it's Required at Creation time it's Optional during Update.This issue becomes apparently when creating an auto-scaled Node Pool, where you must specify the initial number of nodes, from which point onwards AKS will manage the number of nodes within the Node Pool
Fixes Azure/azure-sdk-for-go#6271