Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

pkg/asset: calico: Enable hostPort for calico networking & update cniVersion to 0.3.1 #711

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

weikinhuang
Copy link
Contributor

No description provided.

@coreosbot
Copy link

Can one of the admins verify this patch?

5 similar comments
@coreosbot
Copy link

Can one of the admins verify this patch?

@coreosbot
Copy link

Can one of the admins verify this patch?

@coreosbot
Copy link

Can one of the admins verify this patch?

@coreosbot
Copy link

Can one of the admins verify this patch?

@coreosbot
Copy link

Can one of the admins verify this patch?

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 15, 2017
@weikinhuang
Copy link
Contributor Author

I have tested on my local cluster bootstrapped with bootkube and the --experimental-calico-network-policy flag. Per the comments in #698 and #697 this configuration won't be loaded by default on an existing cluster running calico because 10-calico.conf will be loaded ahead of 10-calico.conflist.

This should work out of the box for new clusters. For existing clusters 10-calico.conf must be deleted on every node and the daemonset must be restarted.

@weikinhuang weikinhuang force-pushed the calico-hostport branch 5 times, most recently from 53ee478 to 0094350 Compare September 15, 2017 01:18
@weikinhuang
Copy link
Contributor Author

I signed the CLA

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 15, 2017
"nodename": "__KUBERNETES_NODE_NAME__",
"ipam": {
"name": "k8s-pod-network",
"cniVersion": "0.3.0",
Copy link

@squeed squeed Sep 19, 2017

Choose a reason for hiding this comment

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

This should be 0.3.1 - we fixed a small bug. It doesn't require any other changes.

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 thing

@squeed
Copy link

squeed commented Sep 19, 2017

FWIW, the changes in here will also work with Flannel. I can file a PR to update that.

@dghubble
Copy link
Contributor

@squeed this change is made to flannel in #697, but paired with changes in the flannel-cni image to cleanup the old CNI config in a way that is backwards compatible whether folks want to fix hostPort or not.

@weikinhuang weikinhuang force-pushed the calico-hostport branch 2 times, most recently from fa9e3a3 to 9f7f143 Compare September 19, 2017 18:08
@weikinhuang
Copy link
Contributor Author

Updated cniVersion

@dghubble
Copy link
Contributor

bootkube won't properly support Calico networking until #714. Installing policy-only Calico on top of cluster using flannel connectivity, this may work because the new CNI config is ordered first. But its a hack on top of a hack.

Let's test this following #714, in a setup that does not use flannel at all and runs Calico networking alone.

@kubernetes-retired kubernetes-retired deleted a comment from coreosbot Sep 28, 2017
@dghubble
Copy link
Contributor

dghubble commented Oct 2, 2017

Rebase please

@weikinhuang
Copy link
Contributor Author

done

@dghubble
Copy link
Contributor

coreosbot run e2e

{
"type": "portmap",
"capabilities": {
"portMappings": true
Copy link
Contributor

Choose a reason for hiding this comment

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

adding

"externalSetMarkChain": "KUBE-MARK-MASQ"

will reuse existing iptables chains

Copy link

Choose a reason for hiding this comment

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

Not yet - we have to do a plugins release first :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

latest bootkube uses https://github.com/projectcalico/cni-plugin/releases/tag/v1.11.1 which already includes portmap plugin. Isn't it enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pending support for CNI_OLD_CONF_NAME

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to wait for it?

  1. is bootkube concerned with compatibility with existing clusters?
  2. Should you name it 05-calico.conflist it will be loaded ahead of existing 10-calico.conf as per https://github.com/kubernetes/kubernetes/blob/v1.8.4/pkg/kubelet/network/cni/cni.go#L107

@redbaron
Copy link
Contributor

redbaron commented Dec 6, 2017

Successfully bootstrapped clusters with bootkube 0.9, this change and calico

@dghubble
Copy link
Contributor

We're also interested in this, are we waiting for the 3.x version release?

Yes.

Do we need to wait for it?

We waited for flannel-cni to provide a mechanism to rotate between CNI configs (and cleanup the old manifest) to make migrations easier for downstreams. Its true you can name a new config to be alphabetically first and cleanup the old config manually or later (not great). Depends how long we'd like to wait.

@redbaron
Copy link
Contributor

redbaron commented Dec 12, 2017

We waited for flannel-cni

TBH I fail to see what value flannel adds comparing to calico mode where it does ipip tunneling. vxlan gives L2 overlay network, but 99.999% of users don't need L2 in kube

@squeed
Copy link

squeed commented Dec 12, 2017

Flannel doesn't need to rotate configs, only Kubelet does - and it will always use the "first" CNI configuration file, for which it scans every 5 seconds. The challenge, then, is ensuring that a network created with config A can be deleted with config B.

In this case, it will work, since CNI delete is idempotent, and the only change is adding the Portmap plugin.

@redbaron
Copy link
Contributor

@squeed , OMG, you say, that kubelet doesn't "memoize' cni config for each network it created so that it can reuse it at tear down time? That is unfortunate oversight.

@dghubble
Copy link
Contributor

Looks like config cleanup is now in https://github.com/projectcalico/calico/releases/tag/v2.6.4

@weikinhuang
Copy link
Contributor Author

@dghubble should I update the version number, or do you want to do that in a separate PR, and I'll rebase this on top after a merge?

@dghubble
Copy link
Contributor

dghubble commented Dec 21, 2017

Let's keep the version bump separate for revertability. Kicked off e2e tests in #818

@dghubble
Copy link
Contributor

Gah, the separate sidecar image didn't get bumped to v1.11.1

@weikinhuang
Copy link
Contributor Author

haha, I'll rebase after #819 is merged

@weikinhuang
Copy link
Contributor Author

This has been rebased against #819

@weikinhuang
Copy link
Contributor Author

ping @dghubble

@dghubble
Copy link
Contributor

dghubble commented Jan 4, 2018 via email

@diegs
Copy link
Contributor

diegs commented Jan 4, 2018

ok to test

- name: CNI_CONF_NAME
value: 10-calico.conflist
- name: CNI_OLD_CONF_NAME
value: 10-calico.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is the default in the sidecar so it could be left off.

dghubble added a commit to poseidon/terraform-render-bootstrap that referenced this pull request Jan 6, 2018
* Ask the Calico sidecar to add a CNI conflist to each node
(for calico and portmap plugins). Cleans up Switch from CNI conf to conflist
* https://github.com/projectcalico/cni-plugin/blob/v1.11.2/k8s-install/scripts/install-cni.sh
* Related kubernetes-retired/bootkube#711
@dghubble dghubble self-requested a review January 8, 2018 15:00
Copy link
Contributor

@dghubble dghubble left a comment

Choose a reason for hiding this comment

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

I've been running this and its been good. 👍

rel. poseidon/terraform-render-bootstrap#36

@dghubble
Copy link
Contributor

dghubble commented Jan 8, 2018

Release notes blurb:

@dghubble dghubble merged commit 4e38084 into kubernetes-retired:master Jan 8, 2018
justaugustus pushed a commit to justaugustus/typhoon-azure that referenced this pull request Feb 12, 2018
* node_exporter service endpoints run on hostNetwork port 9100
* Re-evaluate after kubernetes-retired/bootkube#711
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.

8 participants