Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Rationalize AvailabilityProfile default #3065

Merged
merged 10 commits into from May 29, 2018

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented May 23, 2018

What this PR does / why we need it: We were setting AvailabilityProfile to VMSS by default. However, VMSS is not supported for K8s < 1.10. This PR ensures that we set AvailabilityProfile to AvailabilitySet if using K8s < 1.10.

Backwards compatibility should not be affected because we were validating before to make sure that VMSS was not set with k8s < 1.10. This change just makes it easier for the user by setting a default that works with validation instead of requiring an override.

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

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

@ghost ghost added the in progress label May 23, 2018
@acs-bot acs-bot added the size/S label May 23, 2018
@codecov
Copy link

codecov bot commented May 24, 2018

Codecov Report

Merging #3065 into master will increase coverage by 0.04%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3065      +/-   ##
==========================================
+ Coverage   51.49%   51.53%   +0.04%     
==========================================
  Files          99       99              
  Lines       15026    15017       -9     
==========================================
+ Hits         7737     7739       +2     
+ Misses       6582     6572      -10     
+ Partials      707      706       -1
Impacted Files Coverage Δ
pkg/api/vlabs/validate.go 47.02% <0%> (+0.79%) ⬆️
pkg/acsengine/defaults.go 83.67% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5f5b3...37b728a. Read the comment docs.

@CecileRobertMichon
Copy link
Contributor Author

@sozercan could you please take a look at this PR?

@acs-bot acs-bot added size/M and removed size/S labels May 24, 2018
}

// validation for instanceMetadata using VMSS on Kubernetes
if a.OrchestratorProfile.OrchestratorType == Kubernetes && (agentPoolProfile.AvailabilityProfile == VirtualMachineScaleSets || len(agentPoolProfile.AvailabilityProfile) == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This validation was duplicated

sozercan
sozercan previously approved these changes May 24, 2018
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

lgtm

@jackfrancis
Copy link
Member

/approve
/lgtm

@acs-bot acs-bot added the lgtm label May 29, 2018
@acs-bot
Copy link

acs-bot commented May 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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

@CecileRobertMichon CecileRobertMichon merged commit 77bd483 into Azure:master May 29, 2018
@ghost ghost removed the ready label May 29, 2018
@CecileRobertMichon CecileRobertMichon deleted the vmss-default branch June 6, 2018 19:13
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.

None yet

4 participants