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

removing the vnet from scale templates #2994

Merged
merged 2 commits into from
May 17, 2018
Merged

Conversation

JackQuincy
Copy link
Contributor

@JackQuincy JackQuincy commented May 17, 2018

This allows a cluster to scale even if the user has added a subnet to the vnet

What this PR does / why we need it:
This removes the vnet from update deployments for agents. This allows scaling operations to run on a cluster the user has added a subnet to the vnet.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Azure/AKS#349
If applicable:

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

Release note:

NONE

This allows a cluster to scale even if the user has added a subnet to the vnet
@ghost ghost assigned JackQuincy May 17, 2018
@ghost ghost added the in progress label May 17, 2018
@JackQuincy
Copy link
Contributor Author

back compat shouldn't be needed. This is called by the scale command so no change in user behavior so I wasn't planning on writing docs, but I could be persuaded.

@CecileRobertMichon
Copy link
Contributor

How does this PR relate to #2095?

@JackQuincy
Copy link
Contributor Author

This would also fix that issue.

@codecov
Copy link

codecov bot commented May 17, 2018

Codecov Report

Merging #2994 into master will increase coverage by <.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2994      +/-   ##
==========================================
+ Coverage   50.75%   50.76%   +<.01%     
==========================================
  Files          97       97              
  Lines       14682    14692      +10     
==========================================
+ Hits         7452     7458       +6     
- Misses       6533     6536       +3     
- Partials      697      698       +1
Impacted Files Coverage Δ
pkg/acsengine/transform/transform.go 63.05% <63.63%> (-0.13%) ⬇️

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 36fac61...4f92b53. Read the comment docs.

@JackQuincy
Copy link
Contributor Author

Should I be concerned by the dcos and openshift failures I might of affected open shift expect the error doesn't make sense to me if that was the case. And I didn't affect dcos. I actually improved code coverage marginally.

@CecileRobertMichon
Copy link
Contributor

@JackQuincy probably not, both openshift and dcos e2e have been unstable this week. I restarted the tests to make sure.

@JackQuincy
Copy link
Contributor Author

This is ready for review when someone has the time

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.

@JackQuincy
Copy link
Contributor Author

scale test passed I'm going to merge after lunch if no objections

@CecileRobertMichon
Copy link
Contributor

/approve

@acs-bot
Copy link

acs-bot commented May 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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
Copy link
Contributor

/lgtm

@jackfrancis
Copy link
Member

/lgtm

@CecileRobertMichon
Copy link
Contributor

@JackQuincy is this also going to change upgrade behavior (ie. upgrade without vnet)?

@JackQuincy
Copy link
Contributor Author

@CecileRobertMichon yes for the agent parts of it, no for the master parts. Honestly we should do a small refactor here to make removing the unneeded nrp resources it's own method that gets called by the transform for scaling, that way it could be referenced by master upgrade and avoid the issues I had here previously, like load balancer services have a short outage around the template deployment.

@JackQuincy JackQuincy merged commit 74e1ee1 into master May 17, 2018
@ghost ghost removed the ready label May 17, 2018
@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented May 17, 2018

I'm running an upgrade test at https://jenkins.azure-containers.io/view/acs-engine%20ad-hoc/job/k8s-upgrade/96/console and it's been stuck on the first master deployment for a while... let's keep watching it to make sure it succeeds.

Edit: the first master deployment finished, it's upgrading the agents now.

JackQuincy added a commit that referenced this pull request May 17, 2018
This allows a cluster to scale even if the user has added a subnet to the vnet
@jackfrancis jackfrancis deleted the vnet-scaling-issues branch May 29, 2018 20:46
CecileRobertMichon pushed a commit that referenced this pull request Jun 1, 2018
* acs-engine deploy: model validation and required fields in --help

* Forced apimodel validation to avoid incongruences (fixes #2025)

* Added more details about required fields while deploying (fixes #2491)

* WIP - Adding windows agent pool scaling

* Revert "WIP - Adding windows agent pool scaling" - wrong branch

This reverts commit 0eb7ece.

* WIP - Windows Scaling on Availability Sets and VMSS

* WIP - Naming format modified for vmss windows scaling

* Merge remote-tracking branch 'Azure/patch-release-v0.16.2' into windowsscale

* Fixing tests

* feat(*): Bumps client-go to v7.0.0

This updates client-go to v7.0.0 and adds the needed dependencies on
`k8s.io/api`. Also fixes a small issue with conflicting `uuid` library
versions

* testing re-vendor health

make build-vendor passed!

* removing the vnet from scale templates (#2994)

This allows a cluster to scale even if the user has added a subnet to the vnet

* Fix missing DefaultKubernetesClusterSubnet

* dos2unix

* Clean and go fmt

* Clean and go fmt

* Clean and go fmt

* Fix util_test.go

* Using StorageProfile.ImageReference.Publisher to understand if Windows Agent Pool in Scaling scenario

* lint
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.

4 participants