add a flag to configure docker network #277

foolusion · 2019-02-06T20:49:58Z

I've implemented a minimal solution for adding a docker network.
This adds a flag --network to the create cluster command.

It will error if the network doesn't exist.
If the network flag is not passed it uses the default bridge network.

We can work from here on design and implementation.

resolves #273

k8s-ci-robot · 2019-02-06T20:49:59Z

Welcome @foolusion! It looks like this is your first PR to kubernetes-sigs/kind 🎉

k8s-ci-robot · 2019-02-06T20:50:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

OWNERS

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 · 2019-02-06T20:50:12Z

Hi @foolusion. Thanks for your PR.

I'm waiting for a kubernetes-sigs or 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2019-02-06T20:50:26Z

@foolusion: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

I've implemented a minimal solution for adding a docker network.
This adds a flag --network to the create cluster command.

It will error if the network doesn't exist.
If the network flag is not passed it uses the default bridge network.

We can work from here on design and implementation.

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.

neolit123

thanks for this PR @foolusion
mostly LGTM, one major topic is evaluating that we want to pass the network flag transparently from the kind CLI to Docker.

/priority important-longterm

pkg/cluster/internal/meta/meta.go

neolit123 · 2019-02-06T20:55:25Z

cmd/kind/create/cluster/createcluster.go

@@ -51,6 +52,7 @@ func NewCommand() *cobra.Command {
 		},
 	}
 	cmd.Flags().StringVar(&flags.Name, "name", cluster.DefaultName, "cluster context name")
+	cmd.Flags().StringVar(&flags.Network, "network", "", "network to use for cluster")


could we use something more descriptive here, e.g. outlining that this network will be used for the containers.
e.g. container network to use for the cluster

listing possible values?

also, what is the state of other container runtimes in terms of supporting such a flag; what are the alternatives in naming and values?
e.g. do CRI-O / containerd have a --network where the Docker values such as "bridge" will make sense?

I checked out a few of the other container runtimes.

rkt has a --net flag that you can pass one or more networks I believe it uses CNI.
docker seems to also support the shorter --net flag

CRI-O, containerd do not support networking directly, you must configure it through runc or CNI.

thanks for checking.
CRI-O and containerd are the 2 more active CR after docker in the k8s space, while rkt is not used a lot.

neolit123 · 2019-02-06T21:00:45Z

pkg/cluster/nodes/create.go

-	)
+	}
+	if network != "" {
+		extraArgs = append(extraArgs, "--network", network)


this binds a kind CLI flag to the underlying CR (docker).
i guess we need to evaluate if we want this coupling for the 1.0 release.

yeah, i'm iffy on this. 🤔
I this probably does make sense as a flag, I guess, but as a rule of thumb we should prefer config... 🤔

If it was in the config would it be at the top level?
Is there any setup where your nodes would be in different networks?

sorry, time flew by so fast :(

i'm not sure.. I could see this (EDIT: this = per node network options) maybe, we could start top level and do an override.

Either way we're mostly decoupled from docker at the user interface right now, and I do intend to support podman etc., I meant to investigate further if there was a better way to specify this or any other work arounds.

I do see how this is a feature we need to enable.

pkg/cluster/nodes/create.go

BenTheElder · 2019-02-06T21:15:04Z

pkg/cluster/internal/meta/meta.go

+	return c.network
+}
+
+func (c *ClusterMeta) SetNetwork(network string) {


cluster meta is not supposed to be mutable.

I should really clean up how create works :|

Ah, I didn't want to add anything to the NewClusterMeta func yet since it would require more changes. I can add it as a variadic param to NewClusterMeta since network is not required and existing code doesn't break.

er what I really meant by this, is that clusterMeta is a terrible name, this is really just the cluster key (name) and some utils right now, I suspect Create(...) is the right place to plumb this through.

moved the network config into the cluster.Context. When you use cluster.NewContext you can pass optional parameters like network. This is so we won't mutate the ClusterMeta struct. fixed doc and nit issues.

k8s-ci-robot · 2019-02-21T23:15:28Z

@foolusion: PR needs rebase.

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.

BenTheElder · 2019-02-22T04:11:35Z

pkg/cluster/context.go

+type Option func(*contextConfig)
+
+// WithNetwork sets the container network to be used when creating a cluster.
+func WithNetwork(network string) func(*contextConfig) {


is there any reason this should be an option on context instead of Create()? do you forsee other calls needing this?

👍 for this pattern in general.

I didn't know if anything else was calling Create so I didn't want to change the parameters. Either way works. I haven't dug into the code enough to see how/where it is used.

BenTheElder · 2019-02-24T23:51:04Z

I think we should add this in the network struct proposed in #340 instead.

foolusion · 2019-02-25T17:12:16Z

I think we should add this in the network struct proposed in #340 instead.

Would the only change be removing the flag and setting the value from the config or do you have a different idea?

DazWorrall · 2019-03-04T09:47:53Z

Out of scope for this PR but I wanted to mention it somewhere: for our use case being able to use --net-alias is also important, so somewhere to hang container-runtime specific configuration would be really useful.

foolusion · 2019-03-06T17:23:15Z

I realized that I can just join the kind containers to a custom network after creation. For example docker network connect my-custom-network kind-control-plane Maybe that makes this PR less useful. Wondering what your thoughts are @BenTheElder @neolit123

BenTheElder · 2019-03-07T04:21:27Z

Apologies -- I've been heads down on a big refactor (#360) of cluster creation to make extending this sort of thing easier hopefully... I hoped to get it in before too many PRs adding more to the existing cluster creation code piled up. (should be more or less going in now finally..)

I think a field in config for runtime specific options makes sense, ideally we should continue to do this one step above flags so we at least leave eg the docker client library open as an option in the future:

maybe:

apiVersion: kind.sigs.k8s.io/v1alpha3
kind: Cluster
dockerOptions:
  net: "foo"
   ...

BenTheElder · 2019-05-15T03:03:02Z

superseded by #484, it turns out there are some other issues with using a non-default docker network (the behavior changes on docker's end a bit and we need to work around it)

thanks again for the PR, hopefully some variant of this feature will be in 0.4

* Change unbalance logic * Clean code * Clean code * Add validation * Fix validation --------- Co-authored-by: stg <[email protected]>

add a flag to configure docker network

924c858

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 6, 2019

k8s-ci-robot requested review from BenTheElder and neolit123 February 6, 2019 20:50

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 Feb 6, 2019

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 6, 2019

neolit123 reviewed Feb 6, 2019

View reviewed changes

k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 6, 2019

BenTheElder reviewed Feb 6, 2019

View reviewed changes

pkg/cluster/context: move network config here.

0a6a592

moved the network config into the cluster.Context. When you use cluster.NewContext you can pass optional parameters like network. This is so we won't mutate the ClusterMeta struct. fixed doc and nit issues.

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2019

BenTheElder reviewed Feb 22, 2019

View reviewed changes

DazWorrall mentioned this pull request Feb 25, 2019

RFC: v1alpha3 config proposal #340

Closed

BenTheElder closed this May 15, 2019

stg-0 added a commit to stg-0/kind that referenced this pull request Sep 11, 2023

Change unbalance validation (kubernetes-sigs#277)

a67b884

* Change unbalance logic * Clean code * Clean code * Add validation * Fix validation --------- Co-authored-by: stg <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a flag to configure docker network #277

add a flag to configure docker network #277

foolusion commented Feb 6, 2019 •

edited

Loading

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

neolit123 left a comment

neolit123 Feb 6, 2019 •

edited

Loading

foolusion Feb 6, 2019

neolit123 Feb 6, 2019

neolit123 Feb 6, 2019

BenTheElder Feb 6, 2019

foolusion Feb 7, 2019

BenTheElder Feb 22, 2019 •

edited

Loading

BenTheElder Feb 6, 2019

foolusion Feb 6, 2019

BenTheElder Feb 22, 2019

k8s-ci-robot commented Feb 21, 2019

BenTheElder Feb 22, 2019

foolusion Feb 25, 2019

BenTheElder commented Feb 24, 2019

foolusion commented Feb 25, 2019

DazWorrall commented Mar 4, 2019

foolusion commented Mar 6, 2019

BenTheElder commented Mar 7, 2019

BenTheElder commented May 15, 2019

add a flag to configure docker network #277

add a flag to configure docker network #277

Conversation

foolusion commented Feb 6, 2019 • edited Loading

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 24, 2019

foolusion commented Feb 25, 2019

DazWorrall commented Mar 4, 2019

foolusion commented Mar 6, 2019

BenTheElder commented Mar 7, 2019

BenTheElder commented May 15, 2019

foolusion commented Feb 6, 2019 •

edited

Loading

neolit123 Feb 6, 2019 •

edited

Loading

BenTheElder Feb 22, 2019 •

edited

Loading