Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AKS enable autoscaling #3174

Closed
wants to merge 14 commits into from

Conversation

Lucretius
Copy link
Contributor

@Lucretius Lucretius commented Apr 4, 2019

This bumps the containerservice SDK - fixing a few pieces that the new API changed.

The goal here is to get the kubernetes VMSS working, which would resolve #2900

I have added a test to both the AKS resource and the AKS data source but I am making a few assumptions I need to validate.

  1. If autoscaling is enabled is the type always VirtualMachineScaleSets (conversely, if it is not enabled is it always AvailabilitySet)? This link seems to affirm this assumption.
  2. Is the allowed min_count 0 or 1? I've set it to 0 in the test, so if it is not allowed that test should fail. Min count must be 1 or greater. Setting to 0 caused the API to error with an invalid min_count value.

@Lucretius Lucretius changed the title [WIP] Aks enable autoscaling AKS enable autoscaling Apr 8, 2019
@Lucretius
Copy link
Contributor Author

I suppose we can start reviewing this, with the existing test coverage coupled with the new tests we should be fine to just run this against the acceptance test suite.

@Lucretius
Copy link
Contributor Author

I am testing this PR out now, using the local build to create some kubernetes clusters. Hold off on any review until I work the kinks out of this.

@Lucretius
Copy link
Contributor Author

So, I haven't run the full test suite as I am still using just a personal subscription but I have run the new tests (both with VMSS autoscale off and VMSS autoscale on) and they've passed. I learned that I cannot have min_count and max_count set for just a VMSS without autoscaling and have updated the docs and resource rules to reflect this.

This PR includes the work to both allow VMSS as well as allow autoscaling (I had originally thought these things one and the same otherwise I would have split it up). Feel free to dig in and poke holes in the code now :)

@ghost ghost removed the waiting-response label Apr 21, 2019
@rbankole
Copy link

rbankole commented May 3, 2019

when can we expect this to be merged into master?

@Krueladin
Copy link
Contributor

Krueladin commented May 9, 2019

@katbyte Is anyone looking at adding this anytime soon? Thanks!

@jlpedrosa
Copy link
Contributor

Hi @katbyte @Lucretius

One note. Please keep in mind that this will introduce some issues. Azure API behaves differently if the type of agent pools are AvailabilitySets or VMScaleSets. If the cluster is created with VMScaleSets, the API stops allowing the same request being resent, claiming that any modification to the agent pool must be done through the agent pool API.

"message": "Node pool update via managed cluster not allowed. Use per nodepool operations."

So for old clusters it will work as before, but clusters using this feature will fail to update or if the first request fails to create the cluster (timeout, network error... what ever) terraform will never be able to create the cluster.

@Lucretius Lucretius closed this May 19, 2019
@ghost
Copy link

ghost commented Jun 19, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AKS with VMSS
5 participants