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

Flannel cross platform support #4299

Closed
wants to merge 9 commits into from

Conversation

nmiculinic
Copy link
Contributor

@nmiculinic nmiculinic commented Feb 25, 2019

  • Work in progress since I have to test resulting manifests (( waiting for the cluster to spin up... and debugging other issues on the fly ))

It's adapted from: https://raw.githubusercontent.com/coreos/flannel/a70459be0084506e4ec919aa1c114638878db11b/Documentation/kube-flannel.yml

  • CNI installation has been moved from parallel container to initContainer per official kube-flannel.yml template
  • Also the image has been changed to use only the single image, not specific one for flannel CNI (( which does pretty much the same thing ))

Note:
This breaks custom overridden flannel_image_tag unless they follow flannel naming convention (( tag name-arch )). See PR for details. Probably better solution would be multiarch flannel image manifest, but this should do for now.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 25, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 25, 2019
@nmiculinic
Copy link
Contributor Author

Related to #4294 #4065

@nmiculinic nmiculinic changed the title [WIP] Flannel cross [WIP] Flannel cross platform support Feb 25, 2019
@lwolf
Copy link
Contributor

lwolf commented Feb 25, 2019

Probably better solution would be multiarch flannel image manifest, but this should do for now.

FYI I have a github+travic-ci job set up to pack official flannel images into the manifest (arm+arm64+amd64).
https://github.com/lwolf/docker-multiarch
https://hub.docker.com/r/lwolf/flannel

@nmiculinic
Copy link
Contributor Author

Can we integrate it somehow into upstream?

@lwolf
Copy link
Contributor

lwolf commented Feb 25, 2019

Definitely, if there are no restrictions like - "use only official upstream docker images" or something.
Feel free to test it in your branch

@nmiculinic
Copy link
Contributor Author

I meant the flannel docker image upstream, but yeah. I'll try it out

@nmiculinic nmiculinic changed the title [WIP] Flannel cross platform support Flannel cross platform support Feb 25, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2019
@nmiculinic
Copy link
Contributor Author

Removed WIP status, tested in on cluster and it's working

@Miouge1
Copy link
Contributor

Miouge1 commented Feb 26, 2019

I ran into similar problem with Calico (See PR #4253). I suppose that Flannel is not distributed on docker.io with multi arch detection?

@nmiculinic
Copy link
Contributor Author

This is the most official looking image I could find: https://hub.docker.com/r/quayio/coreos-flannel/ ... and it's not updated nor maintained. To the best of my knowledge, flannel isn't distributed on docker.io

@nmiculinic
Copy link
Contributor Author

What would it take to get this merged? What needs to be changed/added or should I close the PR?

@@ -6,8 +6,15 @@
src: "{{item.file}}.j2"
dest: "{{kube_config_dir}}/{{item.file}}"
with_items:
- {name: flannel, file: cni-flannel-rbac.yml, type: sa}
- {name: kube-flannel, file: cni-flannel.yml, type: ds}
- { name: flannel, file: cni-flannel-rbac.yml, type: sa }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to introduce a space between { and name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

- { name: flannel, file: cni-flannel-rbac.yml, type: sa }
- { name: kube-flannel, file: cni-flannel.yml, type: ds }
vars:
flannel_arches:
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about putting this as a group_var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that seems reasonable

---
apiVersion: extensions/v1beta1
kind: DaemonSet
metadata:
name: kube-flannel
name: kube-flannel-ds-{{ arch }}
Copy link
Contributor

Choose a reason for hiding this comment

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

When upgrading a cluster this can be a problem since we will end up with kube-flannel and kube-flannel-ds-amd64 on the same nodes? Can we do something about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...not sure. Options:

(1) leave amd64 without -arch prefix and hope for the best
(2) make breaking change and put it into changelog
(3) put multiarch flannel behind feature flag which is turned off by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be anything wrong with adding a task to delete the old daemonset (kube-flannel) after verifying the new one is running? It seems that's the approach taken in a number of places in this codebase when upgrading components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhockenbury can you give me an example where this approach is taken and I shall apply the same trick here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option could be to add the arch prefix only not non amd64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think I added proper cleaning in pre-upgrade stage

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 17, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 16, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

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. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants