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.

Sign up for GitHub

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

Jump to bottom

feat: implement AzureMachinePool #483

Merged
merged 5 commits into from
May 4, 2020
Merged

feat: implement AzureMachinePool #483

merged 5 commits into from
May 4, 2020

Conversation

devigned
Copy link
Contributor

@devigned devigned commented Apr 3, 2020 •
edited
Loading

This PR implements machine pools for CAPZ using Azure Virtual Machine Scale Sets. In this PR, an /exp path is added similar to CAPI to be the home for the AzureMachinePool API and controller. The controller, api and service model borrows heavily from AzureMachine (there's a fair bit of copypasta, and I think there are opportunities to dry up a lot of this code).

TODOs

  • add networking details
  • add tests
  • add docs

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #472

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add support for CAPI MachinePools via AzureMachinePools using Virtual Machine Scale Sets

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 3, 2020
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Apr 3, 2020
@devigned devigned force-pushed the machinepool branch 5 times, most recently from 2a8537a to 1d79dc5 Compare April 6, 2020 23:50
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2020
@devigned devigned force-pushed the machinepool branch 2 times, most recently from 2ff7025 to 2b58559 Compare April 21, 2020 16:27
@devigned
Copy link
Contributor Author

When deleting a cluster, machinepool is not deleted. This needs to be resolved upstream and can be remedied by deleting the machinepool, then deleting the cluster.

kubernetes-sigs/cluster-api#2952

@devigned devigned force-pushed the machinepool branch 2 times, most recently from 00c424e to 329b52c Compare April 22, 2020 19:01
@devigned devigned changed the title [wip] implement AzureMachinePool feat: implement AzureMachinePool Apr 22, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2020
@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented May 1, 2020 •
edited
Loading

@devigned I'm sorry to do this to you but... How doable would it be to break down this huge PR into individual commits for all the different components it touches (adding types, controllers, generated files, tests, mocks, docs, etc.) to facilitate reviews? (still one PR, just break down the commit history into smaller commits)

@devigned
Copy link
Contributor Author

devigned commented May 1, 2020

@CecileRobertMichon, what do you think of this separation:

  • docs
  • feature
  • tests & mocks
  • flavor

rbitia
rbitia reviewed May 1, 2020
View reviewed changes
Copy link
Contributor

@rbitia rbitia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I wrote these comments from a view point of someone who is trying out machine pools after creating a cluster with the capz quickstart.

docs/topics/machinepools.md Outdated Show resolved Hide resolved
docs/topics/machinepools.md Outdated Show resolved Hide resolved
docs/topics/machinepools.md
"subscriptionId": "subscriptionID",
"aadClientId": "clientID",
"aadClientSecret": "secret",
"resourceGroup": "capz",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make it easy for first time users and change the rg, cluster name etc, to what they set in the CAPI book quickstart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just there for example. The flavor actually has environment vars which match the Quickstart.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good!

@devigned devigned mentioned this pull request May 1, 2020
Closed
CecileRobertMichon
CecileRobertMichon approved these changes May 1, 2020
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/hold for docs review

Beautiful work @devigned 💯

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2020
@CecileRobertMichon
Copy link
Contributor

/hold cancel
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, devigned

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 4d682a1 into kubernetes-sigs:master May 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v0.5 milestone May 4, 2020
@devigned devigned deleted the machinepool branch May 4, 2020 20:29
This was referenced May 13, 2020
Closed
Open
@devigned devigned mentioned this pull request May 21, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juan-lee juan-lee juan-lee left review comments

@rbitia rbitia rbitia left review comments

@alexeldeib alexeldeib alexeldeib left review comments

@detiber detiber detiber left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon approved these changes

@awesomenix awesomenix Awaiting requested review from awesomenix

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@CecileRobertMichon CecileRobertMichon

@juan-lee juan-lee

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.4.x
Development

Successfully merging this pull request may close these issues.

Implement AzureMachinePools using VMSS
8 participants
@devigned @CecileRobertMichon @juan-lee @k8s-ci-robot @detiber @vincepri @alexeldeib @rbitia