chore: Update calico manifests to v3.4.0 by song-jiang · Pull Request #311 · Azure/aks-engine

song-jiang · 2019-01-14T14:32:22Z

Reason for Change:

This PR updates calico manifests to v3.4.0

Issue Fixed:

None

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:
None.

welcome · 2019-01-14T14:32:25Z

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS-Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

msftclas · 2019-01-14T14:32:35Z

All CLA requirements met.

acs-bot · 2019-01-14T14:33:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @mboersma 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

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2019-01-14T15:32:15Z

Codecov Report

Merging #311 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #311   +/-   ##
=======================================
  Coverage   53.16%   53.16%           
=======================================
  Files          95       95           
  Lines       14244    14244           
=======================================
  Hits         7573     7573           
  Misses       6006     6006           
  Partials      665      665

sylr · 2019-01-14T18:42:13Z

Hi @song-jiang I found a lot of uneeded indenting in the yaml you updates, .i.e:

array:
- item
- item2

array:
  - item
  - item2

The dash - of the array items should stay aligned with the name of the array.

sylr · 2019-01-14T18:38:41Z

parts/k8s/addons/kubernetesmasteraddons-calico-daemonset.yaml

          "nodename": "__KUBERNETES_NODE_NAME__",
          "mtu": 1500,
-          "ipam": <calicoIPAMConfig>,
+          "ipam" : <calicoIPAMConfig>,


Unneeded whitespace.

Thanks @sylr for the review. The manifests has been auto generated for aks-engine from a calico manifest. I will fix the script to output a better format.

sylr · 2019-01-14T18:44:49Z

parts/k8s/addons/kubernetesmasteraddons-calico-daemonset.yaml

    metadata:
      labels:
        k8s-app: calico-typha
-        addonmanager.kubernetes.io/mode: "EnsureExists"


If you remove this the addon-manager won't create the object.

From my test, I can see calico-typha pod running by just having label addonmanager.kubernetes.io/mode: "EnsureExists" set at Deployment metadata layer. This is similar to Deployment calico-typha-horizontal-autoscaler below.

So I am thinking omitting this label for template.metadata should be fine. WDYT?

You're right 👍 I missed that it was not in the good section

song-jiang · 2019-01-16T17:31:50Z

@sylr Could you review it again? I have addressed all your comments.

sylr · 2019-01-16T21:11:31Z

parts/k8s/addons/kubernetesmasteraddons-calico-daemonset.yaml

-apiVersion: extensions/v1beta1
-kind: Deployment
+apiVersion: policy/v1beta1
+kind: PodDisruptionBudget


You should not include the PodDisruptionBudget object. It was added at a time but was removed because it provoked problems with the upgrade problem (see #300).

Also a PodDisruptionBudget can not be updated at the moment. It needs to be removed and recreated if you want to change the spec so it's better to not include it.

sylr · 2019-01-16T21:14:13Z

parts/k8s/addons/kubernetesmasteraddons-calico-daemonset.yaml

-        # the master to communicate with pods.
-        - effect: NoSchedule
-          operator: Exists
+        # Make sure calico-node gets scheduled on all nodes.


Please align all array items comments with the -

Fixing comment indentations by a script seems to be much harder. Let me try and come back to you later.

I fixed indentation manually. Will update script later. @sylr Please have another look. Thanks.

song-jiang · 2019-01-21T15:01:18Z

@sylr Could you help with another look?

song-jiang · 2019-01-24T12:14:18Z

@sylr Have you got chance to review my latest change?

sylr · 2019-01-24T19:02:10Z

@song-jiang sorry but I've been really overwhelmed at work recently. I don't think I'll have the time to review it more in the following days.

song-jiang · 2019-01-25T09:49:35Z

@sylr No worries. Thanks for update.

song-jiang · 2019-02-06T10:55:09Z

Close this PR since I am going to raise PR with calico 3.5

acs-bot added the size/XXL label Jan 14, 2019

song-jiang changed the title ~~Updated calico manifests to v3.4.0~~ chore: Update calico manifests to v3.4.0 Jan 14, 2019

song-jiang force-pushed the calico-v3.4-update branch from a032a11 to 567b0d1 Compare January 14, 2019 14:37

chore: Updated calico manifests to v3.4

dd0206a

song-jiang force-pushed the calico-v3.4-update branch from 567b0d1 to dd0206a Compare January 14, 2019 15:11

sylr reviewed Jan 14, 2019

View reviewed changes

Fix indentation for calico manifests.

491cc57

sylr suggested changes Jan 16, 2019

View reviewed changes

Remove PodDisruptionBudget and fix comment indentation.

1aeb1c9

song-jiang force-pushed the calico-v3.4-update branch from 3e08e20 to 1aeb1c9 Compare January 17, 2019 18:46

song-jiang closed this Feb 6, 2019

song-jiang deleted the calico-v3.4-update branch March 22, 2019 10:51

Conversation

song-jiang commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome bot commented Jan 14, 2019

Uh oh!

msftclas commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acs-bot commented Jan 14, 2019

Uh oh!

codecov bot commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sylr commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

song-jiang commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylr Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

song-jiang commented Jan 21, 2019

Uh oh!

song-jiang commented Jan 24, 2019

Uh oh!

sylr commented Jan 24, 2019

Uh oh!

song-jiang commented Jan 25, 2019

Uh oh!

song-jiang commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

song-jiang commented Jan 14, 2019 •

edited

Loading

msftclas commented Jan 14, 2019 •

edited

Loading

codecov bot commented Jan 14, 2019 •

edited

Loading

sylr commented Jan 14, 2019 •

edited

Loading

song-jiang commented Jan 16, 2019 •

edited

Loading

sylr Jan 16, 2019 •

edited

Loading