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

Support for using hostPort when using calico #3206

Merged
merged 2 commits into from
Sep 22, 2017

Conversation

felipejfc
Copy link
Contributor

For enabling hostPort we need to turn on portmap cni plugin.
In this PR I updated calico and calico-cni images to latest version which already includes the portmap binary, and then I only needed to modify the cni config file to enable it and change its extension from .conf to .conflist.

This is related to:
#3132

I think we should do the same for kube-router, flannel and weave (are there any other cni plugin supported by kops?)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 15, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @felipejfc. 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.

@felipejfc
Copy link
Contributor Author

felipejfc commented Aug 15, 2017

/assign @caseydavenport

@felipejfc
Copy link
Contributor Author

/assign @justinsb

@justinsb
Copy link
Member

@caseydavenport are you able to have a look at the calico side of things here? It seems we have a lot of calico changes all trying to get in at the same time - is there an official calico manifest we should be synchronizing with?

The only kops thing is that we also should bump the version in bootstrapchannelbuilder.go

@k8s-github-robot
Copy link

@felipejfc PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017
@chrislovecnm
Copy link
Contributor

@blakebarnett if you want to take a peak as well

@blakebarnett
Copy link

👍

@caseydavenport
Copy link
Member

Sorry for the long delay, I've been OOO for a couple of weeks and I'm still catching up on my backlog.

@justinsb there are official manifests at docs.projectcalico.org and a PR to add hostport support to those manifests as well: projectcalico/calico#903

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.

We need to bump the version number of calico in the bootstrap code as well, or kops will not apply the changes to existing clusters. Let me know if you are not familiar with the version I am talking about in go

@zimbatm
Copy link

zimbatm commented Aug 31, 2017

@chrislovecnm do you mind giving me pointers to the bootstrap code? I'm facing the same issue and trying to cook a solution.

Copy link
Contributor

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

LGTM (as far as Calico changes are concerned)
Sidenote: I looked into the 'snat' setting for the portmap plugin because I noticed that the PR that @caseydavenport has up with similar changes for Calico had that set false. I came to the conclusion and ran it by Casey that it is ok to have that set and Calico even sets the sysctl flag that it depends on being set.

@felipejfc
Copy link
Contributor Author

felipejfc commented Sep 1, 2017

@chrislovecnm I guess that you mean I have to set calico version to something other than 2.4.1 (even though I'm using it as image version), something like: version := "2.4.1-kops", right?

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Sep 1, 2017

Look at the file history, I cannot remember the semantics of the numbering, but yes we did a version with a -kops in it at onne point.

@chrislovecnm
Copy link
Contributor

/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 Sep 15, 2017
…ico portmap compatibility to existing clusters
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 15, 2017
@felipejfc
Copy link
Contributor Author

@chrislovecnm sorry for the delay changing the version in bootstrapchannel, I just pushed it. Can you validate the changes? thanks

@chrislovecnm
Copy link
Contributor

@blakebarnett per your thumbs up

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. .

@k8s-github-robot k8s-github-robot merged commit b3f26f8 into kubernetes:master Sep 22, 2017
@ringods
Copy link

ringods commented Oct 2, 2017

@chrislovecnm @felipejfc I am using kops 1.8.0-alpha-1 for the additional systemd manifest support in hooks. But this PR breaks my clusters. Calico CNI is not coming up on my k8s 1.6.4 cluster.

Looking at the changes in this PR, I see v2.4.1 listed in k8s-1.6.yaml.template, but v2.4.1-kops.1 in the bootstrapchannelbuilder.go file. I have v2.4.1 running on my failed node, with /etc/cni/net.d/10-calico.conflist as config file. It is a conflist extension, so probably not picked up with the plain v2.1.4 container.

Searching on quay.io, I don't see any image calico/node tagged v2.4.1-kops.1. Am I missing something?

@blakebarnett
Copy link

looks like a dev tag left in accidentally.

@ringods
Copy link

ringods commented Oct 2, 2017

@blakebarnett reading back the conversation of this PR from around 1 Sept, it doesn't seem like it happened accidentally.

@blakebarnett
Copy link

Yeah you're right, I just didn't recognize that tag from when I was working on a different PR that also needed to update this. Guess it's the new convention.

@felipejfc
Copy link
Contributor Author

@ringods the image version should be 2.4.1 in the daemonset not v2.4.1-kops.1

@ringods
Copy link

ringods commented Oct 3, 2017

@felipejfc it is quay.io/calico/node:v2.4.1 in the daemonset. But what is that other version number then? Still, I have CNI+Calico problems all over the place on my 1.6.4 cluster with kops 1.8.0-alpha. Should k8s 1.6 clusters still be supported by this kops version?

@felipejfc
Copy link
Contributor Author

@ringods I guess it should be... Can you detail the problems a little more? like, are the calico pods Running? do they log any error?

The other number is just for kops to know that it has to apply the updates to clusters that were already running with calico v2.4.1 before my PR was merged.

k8s-github-robot pushed a commit that referenced this pull request Dec 15, 2017
Automatic merge from submit-queue.

Support for hostPort when using canal

Similar to: #3206

Without this, we are unable to get `hostPort` working with `canal`. The same is true for `flannel`, but this does add support for plain flannel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

10 participants