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

removed datadisk to allow scale after upgrade #3482

Merged
merged 1 commit into from
Jul 24, 2018
Merged

removed datadisk to allow scale after upgrade #3482

merged 1 commit into from
Jul 24, 2018

Conversation

shanalily
Copy link
Member

@shanalily shanalily commented Jul 16, 2018 •
edited
Loading

What this PR does / why we need it:
When upgrading then scaling a cluster with v0.19.1 or the current master branch, the deployment fails because dataDisk.CreateOption for master nodes in the scale template is "empty" instead of "attach". This part of the template is not needed for scaling agent pools and can be removed.

Which issue this PR fixes: fixes #3478

Special notes for your reviewer:
I had help from @JackQuincy though he may not be able to help with this immediately. I will need more help with testing.

If applicable:

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

Release note:

@ghost ghost assigned shanalily Jul 16, 2018
@CecileRobertMichon
Copy link
Contributor

running tests at https://jenkins.azure-containers.io/view/acs-engine/job/k8s-upgrade-and-scale/

@ghost ghost assigned jackfrancis Jul 23, 2018
@acs-bot
Copy link

acs-bot commented Jul 23, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shanalily
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jackfrancis

If they are not already assigned, you can assign the PR to them by writing /assign @jackfrancis in a comment when ready.

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

@codecov
Copy link

codecov bot commented Jul 23, 2018 •
edited
Loading

Codecov Report

Merging #3482 into master will increase coverage by 0.04%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #3482      +/-   ##
==========================================
+ Coverage   55.46%   55.51%   +0.04%     
==========================================
  Files         105      105              
  Lines       16039    16047       +8     
==========================================
+ Hits         8896     8908      +12     
+ Misses       6392     6382      -10     
- Partials      751      757       +6

@acs-bot acs-bot added the size/M label Jul 23, 2018
@jackfrancis
Copy link
Member

@shanalily FYI I rebased this off of master and force-pushed to this branch so that we can run a few more tests that resemble current master. Thanks again for this, lgtm pending those tests!

jackfrancis
jackfrancis approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@shanalily
Copy link
Member Author

@jackfrancis thanks for looking at this!

@jackfrancis jackfrancis merged commit 271a58c into Azure:master Jul 24, 2018
@ghost ghost removed the in progress label Jul 24, 2018
PaulCharlton added a commit to ElementAnalytics/acs-engine that referenced this pull request Jul 26, 2018
@PaulCharlton
Merge branch 'master' of https://github.com/Azure/acs-engine
60659e8 
* 'master' of https://github.com/Azure/acs-engine: (59 commits)
  Docs: Update user guide list to include Windows, update description of clusters (Azure#3473)
  update to Azure CNI v1.0.10 (Azure#3551)
  Adding 'make dev' equivalent for Windows (Azure#3471)
  print out ubuntu ver in e2e (Azure#3555)
  fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271)
  Bump ginkgo to a tagged release (Azure#3554)
  Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496)
  removing rbac error checking from merge fn (Azure#3530)
  Change dns healthcheck to look at external domain (Azure#3282)
  DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474)
  Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208)
  revert --image-pull-policy=IfNotPresent for win (Azure#3553)
  --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552)
  Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543)
  disable Azure CNI network monitor addon default (Azure#3550)
  only do az vm list for k8s (Azure#3540)
  Retire Swarm E2E for PR test coverage (Azure#3539)
  retire Azure CDN for container image repository proxying (Azure#3535)
  removed datadisk to allow scale after upgrade (Azure#3482)
  Pump k8s-azure-kms version (Azure#3531)
  ...
PaulCharlton added a commit to ElementAnalytics/acs-engine that referenced this pull request Jul 26, 2018
@PaulCharlton
Merge branch 'master' into element-platform-master
5ec376a 
* master: (59 commits)
  Docs: Update user guide list to include Windows, update description of clusters (Azure#3473)
  update to Azure CNI v1.0.10 (Azure#3551)
  Adding 'make dev' equivalent for Windows (Azure#3471)
  print out ubuntu ver in e2e (Azure#3555)
  fix an issue where networkPlugin was not defined correctly when using calico or cilium (Azure#3271)
  Bump ginkgo to a tagged release (Azure#3554)
  Reenable AzureFile tests for Windows on K8s 1.11.1, resolves Azure#3439 (Azure#3496)
  removing rbac error checking from merge fn (Azure#3530)
  Change dns healthcheck to look at external domain (Azure#3282)
  DOCUMENTATION: Fix Documented Default Value for clusterSubnet (Azure#3474)
  Document required manual calico 2.6.3 -> calico 3.1.1 upgrade when upgrading from < 0.17.0-provisioned clusters (Azure#3208)
  revert --image-pull-policy=IfNotPresent for win (Azure#3553)
  --image-pull-policy=IfNotPresent for kubectl run commands (Azure#3552)
  Kubernetes: --max-pods=30 should be Azure CNI-only (Azure#3543)
  disable Azure CNI network monitor addon default (Azure#3550)
  only do az vm list for k8s (Azure#3540)
  Retire Swarm E2E for PR test coverage (Azure#3539)
  retire Azure CDN for container image repository proxying (Azure#3535)
  removed datadisk to allow scale after upgrade (Azure#3482)
  Pump k8s-azure-kms version (Azure#3531)
  ...
@shanalily
Copy link
Member Author

shanalily commented Aug 9, 2018 •
edited
Loading

Any idea when this will be added to a release? I'm needing to use a branch in my project still to get upgrade followed by scaling up to work.

I'm not too concerned right now since master seems to be in good shape.

@jackfrancis
Copy link
Member

@shanalily do you mean an acs-engine release?

@shanalily
Copy link
Member Author

shanalily commented Aug 9, 2018 •
edited
Loading

@jackfrancis Yep, sorry that was unclear.

@jackfrancis
Copy link
Member

ETA next week w/ v0.21.0

@shanalily
Copy link
Member Author

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jackfrancis jackfrancis jackfrancis approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

Assignees

@jackfrancis jackfrancis

@shanalily shanalily

Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scaling a cluster after a kubernetes upgrade
4 participants
@shanalily @CecileRobertMichon @acs-bot @jackfrancis