Add cli flags for multinode

[fabriziopandini]

This PR adds CLI flags to change the number of nodes without being forced to use the config files

Fixes: #133

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabriziopandini
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

[neolit123]

i will defer to @BenTheElder and @munnerz on this one.

at this point this is already possible using the config, so this is just a CLI convenience and there are always useful flags that we can add, but my philosophy is "config only, unless something does not make sense to be in the config".

i also think that the argument of "i cannot write config file in my testing setup and i need to use flags only " is invalid. a read-only setup would not work well in the first place and we can technically make kind read a config from stdin too.

[BenTheElder]

Two comments. Thanks for this!

[BenTheElder]

/hold
Seeing as this doesn't seem to be blocking our current users, did you have need of this feature Fabrizio? If not maybe it will make more sense against the next config type anyhow.

I'm on the fence, I generally dislike flags that cannot be combined with other flags and think that itself makes poor UX, but flags are less effort than config 🤔

[BenTheElder]

(also sorry, I believe I requested this initially, but the eagerness people have had to go ahead and use the feature anyhow has been surprising, perhaps we can avoid the messes of combining config and flags, and avoid disjoint flags)

[neolit123]

I'm on the fence, I generally dislike flags that cannot be combined with other flags and think that itself makes poor UX, but flags are less effort than config

so in terms of the test jobs we want to add to test-infra these flags will definitely help.
i.e. we don't need pull a repo that stores configs for the sake of creating a custom cluster with topology X.

yet we still have to pull a repo for the test-cmd script, so we might as well store the topology in a config, so that we don't have to touch test infra if a topology changes.

[fabriziopandini]

@BenTheElder @munnerz @neolit123 thanks for the comments, hopefully, everything is addressed

flags vs config

I agree this is not blocking users, but I think that what discussed in #133 is still valid and many users will benefit from this PR.

This IMO compensates the fact that flags and config can't be used together, and so I'm giving another try for getting this in 😉

Feel free to override.

new config version (as discussed in slack)

IMO we are ready for another round on config and probably this will fix also the flag vs confg thing, but only if in the target solution the node list will go away.
Happy to discuss this in the meeting or on another issue!

[neolit123]

/lgtm
/hold

[timothysc]

This seems generally useful for testing, what's the reservation?

[BenTheElder]

This seems generally useful for testing, what's the reservation?

We've still been discussing how not to dig a hole mixing config and flags. Testing w/ @neolit123 has done pretty well using config only so far so it doesn't seem urgent.

Something like #274 in conjunction with this probably makes more sense. We've been discussing reshaping the config to make the single implicit control plane load balancer (for HA) more explicit.

[timothysc]

fwiw as a user if I want to quick-turn a parameter, having a flag is always easier for override.

config is great for default, but if you readily tweek parameters... then not so much.

I'm hoping @luxas library can help with that in the future.

[BenTheElder]

Fair.

In some respects this will behave a little more like clusterctl as we add things like #62, with a definition for each node (though nodes can be repeated).

Mixing number of node flags with specific node config won't go well, unless component config pulls some real magic :^)

Just logically the behavior for merging these is confusing. I think initially we can't allow mixing these flags with config.

If I do config:

nodes:
- role: worker
  replicas: 3
  image: foo
- role: control-plane
  image: bar

kind create cluster --config=config.yaml --num-worker-nodes=5

Then what happens?

(with this PR --num-worker-nodes will be silently ignored in favor of the config)

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[neolit123]

/unassign

[neolit123]

i'm near 50/50 on this feature.

NOTE: i'm not seeing users complain about the lack of these flags and the kind config is fairly simple to use, so slightly leaning towards not having these flags in the stock kind CLI for now.

[k8s-ci-robot]

@fabriziopandini: 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.

[k8s-ci-robot]

@fabriziopandini: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kind-conformance-parallel-1-14	51b5678	link	/test pull-kind-conformance-parallel-1-14

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[BenTheElder]

I'm going to +1 on we probably don't need this, we can revisit in the future if it becomes an issue