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

Use zone-info while scaling-up the node-group from zero #38

Merged
merged 1 commit into from
May 7, 2020

Conversation

hardikdr
Copy link
Member

@hardikdr hardikdr commented May 3, 2020

What this PR does / why we need it: This PR uses the zone-information for scaling the node-groups from zero.

Which issue(s) this PR fixes:
Fixes #34

Special notes for your reviewer:

Release note:

Autoscaler uses zone-information while scaling-up the node-group from zero

@hardikdr hardikdr requested a review from prashanth26 as a code owner May 3, 2020 12:36
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 3, 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 May 3, 2020
@@ -946,6 +946,7 @@ type AzureVirtualMachineProperties struct {
OsProfile AzureOSProfile
NetworkProfile AzureNetworkProfile
AvailabilitySet AzureSubResource
Zone *int
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a workaround for now.
We should ideally re-vendor the MCM, but it seems challenging due to recently updated Azure-dependencies in MCM, I would prefer to do it separately.

@@ -424,15 +428,19 @@ func (m *McmManager) GetMachineDeploymentNodeTemplate(machinedeployment *Machine
GPU: azureInstance.GPU,
}
region = mc.Spec.Location
if mc.Spec.Properties.Zone != nil {
// Do not re-use the zone before re-vendoring mcm.
zone = mc.Spec.Location + strconv.Itoa(*mc.Spec.Properties.Zone)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yet to test this change for Azure.

Copy link
Member Author

@hardikdr hardikdr May 6, 2020

Choose a reason for hiding this comment

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

I tested this PR with Azure now, works well.

@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 May 6, 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 May 6, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 6, 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 May 6, 2020
@hardikdr hardikdr merged commit ec9bd3d into gardener:machine-controller-manager May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoscaler may not scale from zero for statefulsets.
5 participants