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.

Sign up for GitHub

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

Jump to bottom

Add ability to set various cilium flags through CLI #8928

Merged
merged 3 commits into from
Jul 8, 2020
Merged

Add ability to set various cilium flags through CLI #8928

merged 3 commits into from
Jul 8, 2020

Conversation

olemarkus
Copy link
Member

Needed for a new e2e cilium test

@k8s-ci-robot k8s-ci-robot added 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. labels Apr 16, 2020
@olemarkus
Copy link
Member Author

/assign @rifelpet

@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch from 2010b0c to 112470d Compare April 16, 2020 20:09
@olemarkus olemarkus changed the title Add ability to set various cilium flags through CLI WIP: Add ability to set various cilium flags through CLI Apr 16, 2020
@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 Apr 16, 2020
@olemarkus
Copy link
Member Author

Want to get etcdManaged in as well.

@rifelpet
Copy link
Member

rifelpet commented Apr 16, 2020 •
edited
Loading

In order to test the etcdManaged under e2e tests we'll have to figure out how to add an additional etcdCluster named cilium. I'm struggling to come up with a simple solution for that...

@olemarkus
Copy link
Member Author

Yeah. I am wondering if the only feasible option is a proper cli flag.

@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch from 112470d to b4c0c77 Compare April 17, 2020 08:30
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 17, 2020
@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch 2 times, most recently from c758461 to 7cd8f6d Compare April 17, 2020 09:04
@olemarkus olemarkus changed the title WIP: Add ability to set various cilium flags through CLI Add ability to set various cilium flags through CLI Apr 17, 2020
@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 Apr 17, 2020
@rifelpet
Copy link
Member

/lgtm

Thoughts on the new cli flag @justinsb or anyone else?
/assign @justinsb

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2020
@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch from 7cd8f6d to b3ea2b6 Compare May 25, 2020 06:03
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020
@rifelpet
Copy link
Member

/retest

@olemarkus
Copy link
Member Author

How far away is this one from approval now?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2020
@olemarkus olemarkus mentioned this pull request Jun 10, 2020
Merged
@hakman
Copy link
Member

hakman commented Jun 20, 2020

I am debating here that it may be the easiest way to get people started with Cilium + Etcd. It is not just good for kubetest, but would generate the correct config automatically, instead of doing some steps prone to user error.
From what I see this seems to be the way to activate a particular config for a certain network plugin.

@olemarkus
Copy link
Member Author

It may be an idea to rethink this a bit. Nodeport and ENI is the most important features to test. For networking values, these two also makes more sense perhaps.

etcd is less important to test. Also, if etcd-manager works well, it may also be an idea to switch that on by default, which means that cli flag is perhaps not that useful.

@hakman
Copy link
Member

hakman commented Jun 20, 2020

My take on this is that you should have a default as close as possible to upstream that will work without issues to create smaller test clusters. Think about adding various --networking=cilium-<type> flags to create more complex setups.
In any case, this is mostly a nit. Most work on this is really ok. Let's see also if @johngmyers has some feedback.

@johngmyers
Copy link
Member

johngmyers commented Jun 20, 2020 •
edited
Loading

There appears to be precedent in flannel-vxlan vs. flannel-udp for putting it in --networking. Unfortunately, Cilium appears to have three independent options, so how would one say one wants Etcd and Nodeport, but not ENI?

We could do something like --networking=cilium=etcd,nodeport but that's getting unwieldy. The only other idea I have is a --networking-option flag of type []string.

@olemarkus
Copy link
Member Author

With #9425 the more complex option will be on by default. ENI mode could be interesting to test still though.

@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch from fe6a79a to 2b8a13e Compare July 6, 2020 16:50
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2020
@olemarkus
Copy link
Member Author

Rebased to #9490

@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch 2 times, most recently from 227186c to 892d2c6 Compare July 7, 2020 06:12
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 7, 2020
@hakman
Copy link
Member

hakman commented Jul 7, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2020
Ole Markus With added 3 commits July 7, 2020 21:04
Add ability to set various cilium flags through CLI
cf9a5c0 
Needed for a new e2e cilium test
Add networking provider for using etcd-manager for cilium
aab5054 
This is the only feasible way of adding the additional etcd cluster for a cilium e2e test
Document the cilium-etcd networking provider
d084d9d
@olemarkus olemarkus force-pushed the cilium-set-cluster-flags branch from 892d2c6 to d084d9d Compare July 7, 2020 19:06
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2020
@hakman
Copy link
Member

hakman commented Jul 7, 2020 •
edited
Loading

@rifelpet I think this is ready, maybe you can take a look at it again. Thanks!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2020
@johngmyers
Copy link
Member

/lgtm

@rifelpet
Copy link
Member

rifelpet commented Jul 8, 2020 •
edited
Loading

I took a final pass and it looks good to me and is pretty straight forward 👍 thanks for sticking with it through all the rebases.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, rifelpet

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit 51592e0 into kubernetes:master Jul 8, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rifelpet rifelpet rifelpet left review comments

@joshbranham joshbranham Awaiting requested review from joshbranham

@robinpercy robinpercy Awaiting requested review from robinpercy

Assignees

@justinsb justinsb

@rifelpet rifelpet

@johngmyers johngmyers

@hakman hakman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
6 participants
@olemarkus @rifelpet @hakman @johngmyers @k8s-ci-robot @justinsb