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

Update weave to 2.0.1 #2829

Merged
merged 1 commit into from
Jul 15, 2017
Merged

Update weave to 2.0.1 #2829

merged 1 commit into from
Jul 15, 2017

Conversation

marccarre
Copy link
Contributor

@marccarre marccarre commented Jun 29, 2017

Description:

Upgrade from 1.9.8 to 2.0.1, see full list of changes here:

Steps:

As the diff may be hard to digest, here are the steps I followed for this PR:

  1. I identified the changes made in kops YAML templates vs. static YAMLs provided with weave:

     $ curl -fsSLo 1.6.yaml https://github.com/weaveworks/weave/releases/download/v1.9.8/weave-daemonset-k8s-1.6.yaml
     $ curl -fsSLo 1.5.yaml https://github.com/weaveworks/weave/releases/download/v1.9.8/weave-daemonset.yaml
     $ diff 1.6.yaml k8s-1.6.yaml.template
     $ diff 1.5.yaml pre-k8s-1.6.yaml.template
    

    These highlighted the below changes:

    1. for labels:

       +   labels:
       +     role.kubernetes.io/networking: "1"
      
    2. for resources (both for weave and weave-npc):

       -               cpu: 10m
       ---
       +               cpu: 100m
       +               memory: 200Mi
       +             limits:
       +               cpu: 100m
       +               memory: 200Mi
      
    3. for env:

       +           env:
       +             - name: IPALLOC_RANGE
       +               value: {{ .KubeControllerManager.ClusterCIDR }}
       +             {{- if .Networking.Weave.MTU }}
       +             - name: WEAVE_MTU
       +               value: "{{ .Networking.Weave.MTU }}"
       +             {{- end }}
      
  2. I then re-applied the above changes to the latest (2.0.1) YAML files:

     $ curl -fsSLo k8s-1.6.yaml.template https://github.com/weaveworks/weave/releases/download/v2.0.1/weave-daemonset-k8s-1.6.yaml
     $ curl -fsSLo pre-k8s-1.6.yaml.template https://github.com/weaveworks/weave/releases/download/v2.0.1/weave-daemonset.yaml
    

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 29, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @marccarre. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2017
@chrislovecnm chrislovecnm self-assigned this Jun 29, 2017
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

You will need to bump the weave version number in upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

How have you tested upgrades? 1.5 -> 1.6 for intance with weave. Some CNI changes have given us challenges.

Appreciate the update!

@marccarre
Copy link
Contributor Author

You will need to bump the weave version number in upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

Done. Thanks for the review 👍

@chrislovecnm
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 30, 2017
@justinsb
Copy link
Member

justinsb commented Jul 1, 2017

Thanks @marccarre for doing this, and for documenting how you do this. Awesome!

So the formatting change is what makes this diff look really big. I'm not sure whether we want to switch formats. The problematic diff here is a one-off, in future we can just do a straight diff between the weave old & new versions and apply any changes to our version. Honestly, I don't know.

More problematically: weaveworks/weave#3049

I did some massaging and I think the diff is:

4a5,6
>   labels:
>     name: weave-net
7d8
< kind: ClusterRole
8a10
> kind: ClusterRole
10a13,14
>   labels:
>     name: weave-net
13c17
<   - ""
---
>   - ''
31d34
< kind: ClusterRoleBinding
32a36
> kind: ClusterRoleBinding
34a39,40
>   labels:
>     name: weave-net
36d41
<   apiGroup: rbac.authorization.k8s.io
38a44
>   apiGroup: rbac.authorization.k8s.io
47a54,55
>   labels:
>     name: weave-net
57d64
<           image: weaveworks/weave-kube:1.9.8
59a67,73
>           env:
>             - name: HOSTNAME
>               valueFrom:
>                 fieldRef:
>                   apiVersion: v1
>                   fieldPath: spec.nodeName
>           image: 'weaveworks/weave-kube:2.0.1'
65a80,82
>           resources:
>             requests:
>               cpu: 10m
81,91c98,99
<           resources:
<             requests:
<               cpu: 10m
<         - env:
<             - name: HOSTNAME
<               valueFrom:
<                 fieldRef:
<                   apiVersion: v1
<                   fieldPath: spec.nodeName
<           image: weaveworks/weave-npc:1.9.8
<           name: weave-npc
---
>         - name: weave-npc
>           image: 'weaveworks/weave-npc:2.0.1'
100d107
<       serviceAccountName: weave-net
102,103c109,110
<         seLinuxOptions:
<           type: spc_t
---
>         seLinuxOptions: {}
>       serviceAccountName: weave-net
105,106c112,113
<         - key: node-role.kubernetes.io/master
<           effect: NoSchedule
---
>         - effect: NoSchedule
>           operator: Exists
109c116,117
<           emptyDir: {}
---
>           hostPath:
>             path: /var/lib/weave
124a133,134
>   updateStrategy:
>     type: RollingUpdate
  • The version update we obviously want
  • I think we probably want to stick with our more explicit tolerations; we can be more specific / obvious
  • Unsure on seLinuxOptions, but we should do it
  • The hostPath is important and we should take that
  • The HOSTNAME env change I think is a mistake Should hostname be on weave-npc? weaveworks/weave#3049
  • The updateStrategy is harmless I think

@marccarre
Copy link
Contributor Author

The HOSTNAME env change I think is a mistake weaveworks/weave#3049

This has been fixed in weaveworks/weave#3051, and I have just added it to this PR as well.

@marccarre
Copy link
Contributor Author

In case it could help, below is additional context on some of the points listed above:


I think we probably want to stick with our more explicit tolerations; we can be more specific / obvious

FYI, this is has been added to 2.0.0 as a minor improvement so that Net can be scheduled everywhere:

* Relaxed Kubernetes tolerations for Weave Net's daemonset in order to
  match any node (previously, only taints directed at master). #3018

Unsure on seLinuxOptions, but we should do it

Similar questioning on our side (see here and here), but we decided to remove it by default because of weaveworks/weave#2990.


The hostPath is important and we should take that

FYI, this has been added to 2.0.0 under other new features:

* `weave-kube` now stores data about IP allocation in `/var/lib/weave`
  on the host instead of in a Kubernetes volume.  This means that the
  data will persist across pod deletion and re-creation, e.g. during
  an upgrade of Weave Net, which makes restarts more
  reliable. #2610,#2967

The updateStrategy is harmless I think

For what it's worth, I have personally tested this under a 3-nodes Kubernetes v1.6.1 cluster, upgrading from Net 1.9.8 to 2.0.0, and it worked like a charm.

@aledbf
Copy link
Member

aledbf commented Jul 6, 2017

Unsure on seLinuxOptions, but we should do it

This change is required to fix weave on coreos

@justinsb
Copy link
Member

justinsb commented Jul 6, 2017

Could I persuade you to follow our existing yaml format (i.e. de-dent it by one level, and use ---)? Because then the diff is readable. I've realized that it isn't just for this code review, but also for our users who may want to check what changed :-)

@justinsb
Copy link
Member

Going to merge and apply the formatting. I think that should make everyone happy :-)

@justinsb justinsb merged commit 0044a32 into kubernetes:master Jul 15, 2017
@justinsb
Copy link
Member

Thanks for the update @marccarre :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants