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

Azure MachineSet: VM ScaleSet or AvailabilitySet #519

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Azure MachineSet: VM ScaleSet or AvailabilitySet #519

merged 1 commit into from
Oct 2, 2020

Conversation

dkistner
Copy link
Member

What this PR does / why we need it:
Add support for VirtualMachineScaleSet Orchestration Mode VM (VMO) in the AzureMachineClass.

Introduce a generic MachineSet configuration field in AzureMachineClass.
The machineSet field can be used to configure either a VMO (VirtualMachineScaleSet Orchestration Mode VM) or an AvailabilitySet for a MachineClass. Or whatever else will come in the future :)

The availabilitySets field will be kept for a while but is now deprecated in favour of the machineSet field.
The Azure compute sdk is also updated to support VMO.

Release note:

The machine-controller-manager supports now machines attached to Azure VirtualMachineScaleSet Orchestration Mode VM (VMO).

The field `availabilitySets` in the `AzureMachineClass` is now deprecated in favour of the field `machineSet`, which allow to configure AvailabilitySets and VirtualMachineScaleSet Orchestration Mode VM (VMO). The field will be removed in the future.

@dkistner dkistner requested review from ggaurav10 and a team as code owners September 23, 2020 11:30
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 23, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 23, 2020
@hardikdr
Copy link
Member

Thanks for the PR @dkistner, we are kind of stuck at a couple of other stuff, will take a look soon.

@prashanth26
Copy link
Contributor

prashanth26 commented Sep 30, 2020 •
edited

/assign @AxiomSamarth for review

@gardener-robot gardener-robot added the needs/review Needs review label Sep 30, 2020
@AxiomSamarth
Copy link
Collaborator

@prashanth26

I went through the code and based on the code comments, I could read and comprehend the code. I do not have any comments as such. However, I understood from the PR that the spec of AzureMachineClass has been introduced with a new field machineSet that can be used to configure the VMO or the availabilitySet. This will help me to implement my Azure OOT accordingly. Hope the intention to ask me to review this was for the same.

@dkistner
Copy link
Member Author

dkistner commented Oct 1, 2020

@AxiomSamarth Yes basically I added the machineset field to have a simple interface for any kind of machineset which will come. We need to add support for VMOs now, but we will still have the availabilitysets. I just deprecated the old field for only availability sets.

prashanth26
prashanth26 approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Oct 1, 2020
@prashanth26
Copy link
Contributor

/ok-to-test

@prashanth26 prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 1, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 1, 2020
hardikdr
hardikdr reviewed Oct 1, 2020
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @dkistner , just a minor comment, looks great otherwise.

pkg/apis/machine/types.go Show resolved Hide resolved
@dkistner
Azure MachineSet: VM ScaleSet or AvailabilitySet
af10b4e 
Add generic MachineSet configuration in AzureMachineClass.
The machineSet field can be used to configure either a VMO or an AvailabilitySet for a MachineClass.

The availabilitySets field will be kept for a while but is now deprecated in favour of the machineset field.
The Azure compute sdk is also updated to support VMSS/VMO.
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 2, 2020
@hardikdr
Copy link
Member

hardikdr commented Oct 2, 2020

/ok-to-test

@hardikdr hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 2, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 2, 2020
@hardikdr hardikdr merged commit abf38c8 into gardener:master Oct 2, 2020
@unmarshall unmarshall mentioned this pull request Jun 9, 2023
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardikdr hardikdr hardikdr left review comments

@MSSedusch MSSedusch MSSedusch approved these changes

@AxiomSamarth AxiomSamarth AxiomSamarth approved these changes

@prashanth26 prashanth26 prashanth26 approved these changes

@ggaurav10 ggaurav10 Awaiting requested review from ggaurav10

Assignees

@AxiomSamarth AxiomSamarth

Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@dkistner @hardikdr @prashanth26 @AxiomSamarth @MSSedusch @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot