Add RFC: CLI command for creating a new cluster - #98
Conversation
Co-authored-by: Fernando Ripoll <fernando@giantswarm.io>
|
|
||
| If the user chooses to create the cluster immediately (by not specifying an output destination or format), we want to provide detailed progress on the creation. The user should be encouraged to quit watching the progress at any time, without any effect on the provisioning progress. | ||
|
|
||
| We may even introduce another subcommand to continue watching the creation progress at any time. |
There was a problem hiding this comment.
I would vote for this one, or -w flag either
There was a problem hiding this comment.
I don't think this should be included in this RFC. It's scope creep in my opinion. Watching for cluster creation isn't "technically" related to applying of the cluster app. There's a lot of layers and processes that are involved to make that happen. Instead, we already have the kubectl gs get clusters that should be preferred in my opinion.
| global: | ||
| metadata: | ||
| servicePriority: lowest | ||
| name: test01 | ||
| organization: testorg | ||
| description: Just a test cluster |
There was a problem hiding this comment.
In an ideal case, the CLI wouldn't require passing any config unless the user wants to override defaults.
For example, it would be very friendly if this were a correct and complete way to start a cluster:
kubectl gs create cluster --provider capa --name test01
Based on the discussion in giantswarm/roadmap#2999, name will be required and can be supplied via the CLI, but IIUC the others listed here could be defaulted (priority, org) or left empty (description). Is that true?
There was a problem hiding this comment.
I agree on the approach of having a valid config by providing minimal config, however, there will be a few infra-specific parameters (really not a lot) for the onprem providers that we can't default.
There was a problem hiding this comment.
@stone-z What you describe is really the intended behaviour. We aim to ask only the required details from the users. However, with kubectl gs create cluster --provider capa --name test01, the one thing I'm missing is the organization. How can we set a default for organization (and thus namespace)?
There was a problem hiding this comment.
this is a product decision but we could decide that needs to be a default org (using label or something for it)
There was a problem hiding this comment.
How can we set a default for organization (and thus namespace)?
My understanding was that there was already some concept of a "default" organization for an MC. If that's not the case, I suppose two flags would be required unless/until there was some declaration of a default.
there will be a few infra-specific parameters (really not a lot) for the onprem providers
It would be great to move that burden into the platform somewhere. I don't know what the current flags would be, but I can imagine there might be something about a CIDR range, for example. Right now we'd demand it from the user, but it would be much nicer if the platform defaulted it to "the next range of default size X not overlapping the ranges currently in use" (you get the idea). For a single MC plus its WCs it wouldn't be that hard to implement, but if it requires knowing about the rest of the infrastructure maybe it's not possible, which probably depends on the customer. Maybe we could default in some majority of the cases. For now, sure, users might have to specify one, but I'd say that specific example (CIDR ranges) makes end-user self service a lot more risky because a user might royally break things when just trying to create a sandbox environment.
There was a problem hiding this comment.
I'd not add a default org for now and it would also be a bit weird when a user is a member of several orgs but not all. Only in the single org member case we can clearly set a default.
There was a problem hiding this comment.
Honestly, thinking of this as a kubectl command, kubectl has a default/selected namespace, I kinda feel we should stay true to that if at all.
There was a problem hiding this comment.
After some talking in todays KaaS sync I went with the --namespace flag. I dislike that it's one more flag the user will have to set, as "default" will result in non-functioning clusters, but all other solutions seem too involved for now.
| name: test01 | ||
| organization: testorg | ||
| description: Just a test cluster | ||
| EOF |
There was a problem hiding this comment.
Opening a comment here to not sidetrack from the point of @stone-z.
I wanted to point out that for CAPV and CAPVCD we use the name of the cluster app as the cluster name. (I don't know if CAPA/CAPZ moved to this method too).
There was a problem hiding this comment.
In terms of logic, I think it's the other way around. The CLI command should take the cluster name from user input and use it for the App CR .metadata.name field.
| global: | ||
| metadata: | ||
| servicePriority: lowest | ||
| name: test01 | ||
| organization: testorg | ||
| description: Just a test cluster |
There was a problem hiding this comment.
I agree on the approach of having a valid config by providing minimal config, however, there will be a few infra-specific parameters (really not a lot) for the onprem providers that we can't default.
| description: Just a test cluster | ||
| EOF | ||
|
|
||
| While ex. 1 appears simpler and allows for easier re-use of a file-based configuration for a single user, ex. 2 has the benefit of being completely self-contained. It's a pure one-liner without the need for a separate file. This way it's easy to share the command, or copy the command from a website or web UI and paste it into a terminal. |
There was a problem hiding this comment.
I can see this getting outdated pretty quickly as the schema changes, how would we maintain it? Would it need to be included into the release notes?
There was a problem hiding this comment.
That's an interesting point. In a previous draft document I had an entire section about this, but decided to skip it here in order to not bloat the RFC. I'll copy it here:
Cluster app schema evolution and incompatibilities
As cluster apps' schemas evolve, we sometimes introduce incompatible changes. For example, we move properties into a different place. With the specification described above, we will encourage users to keep their cluster configuration values YAML files around for re-use, which will eventually lead into users facing incompatibilities.
Most important in this situation is that these incompatibilities get caught through the validation executed by the
kubectl gs create clustercommand. This way, the command will fail with an error. However, this requires that the schema used for validation does not allow arbitrary properties (additionalProperties) on any object.Outside the scope of this spec, we may think about further improvements to provide a smooth experience when creating clusters using config files form previous app versions. Some ideas:
- Add values migration into the CLI as a separate command, or even
- Automatically migrate values from previous schemas into the selected app version's schema
- Have a schema version and require/allow configuration to specify the schema version to validate against
For now, I think that the best approach we had was the yq migration command embedded in the changelog (example).
| ## Target use case | ||
|
|
||
| The most important use case we target with our CLI command is the creation of a short-lived cluster for testing purposes, as this is the likely the most frequent reason to create a cluster. | ||
|
|
||
| Production clusters, in contrast, are created much more rarely, often based on the experience gathered through testing with various clusters. Production clusters are also more likely to be provisioned through GitOps, while test clusters are often created and deleted ad-hoc. |
There was a problem hiding this comment.
I find it weird that we're not also targeting production / gitops clusters with this. Having multiple ways of creating clusters just leads to more confusion doesn't it?
|
|
||
| - A **dry run option** should allow "previewing" the result without actually writing any resources to the management cluster. | ||
|
|
||
| - A **connection with the management cluster** should only be required to write resources. For a dry run, no connection should be necessary. (However, an internet connection will be required to access public information from GitHub repositories.) |
There was a problem hiding this comment.
We currently rely on some defaulting from the MC in some cases I believe. This should be confirmed with the provider teams.
There was a problem hiding this comment.
There was a requirement to be logged in an MC of the same provider as the WC to create to get the latest cluster-app and default-apps-app versions if they weren't specified in the flags. I always found this behaviour counter-intuitive so I agree with the proposal. It is even more relevant with regards to hybrid environments (e.g. CAPZ MC with CAPV WC)
|
|
||
| The most important flags would be: | ||
|
|
||
| - `--provider`: (required) provider name. For example: capa, capvcd, capz, eks. |
There was a problem hiding this comment.
How will these be discovered by the user? How will cross-provider clusters be handled? (e.g. CAPV on CAPZ)
There was a problem hiding this comment.
That's an important question and I have no answer for it yet.
There was a problem hiding this comment.
Better make it required for now and not use the MC to implicitly determine the type. I'm saying this because we may have more CAPx-WC-on-CAPy-MC mixed cases in the future, and requiring the argument leads to the least surprise.
|
|
||
| This enables the target scenario as described earlier, by re-using most configuration values from one file, and providing the required difference (here: the cluster name) via standard input. The same could of course be achieved by passing a second YAML file via another `--config` flag instead. | ||
|
|
||
| In input validation, each source is evaluated against the app's values schema independently. We avoid replicating any values merging logic (as known from helm or the app platform) into the kubectl-gs CLI code base. |
There was a problem hiding this comment.
Wont this break in your above example unless the template.yaml has a default name that is always overwrote by a later config?
E.g. I'd really want to have a template.yaml that I pass in to all clusters that have the non-changing values and then I can do something like --set global.metadata.name=test02 for each cluster. But if the validation is applied to the template.yaml before the --set is added it'll fail because name isn't set.
Or maybe I've misunderstood what this mean?
There was a problem hiding this comment.
The validation I was thinking of was simply validating config YAML against the config schema. It seems you are thinking of a validation of a rendered manifest, right? Not sure the latter is needed on the CLI side.
|
|
||
| If the user chooses to create the cluster immediately (by not specifying an output destination or format), we want to provide detailed progress on the creation. The user should be encouraged to quit watching the progress at any time, without any effect on the provisioning progress. | ||
|
|
||
| We may even introduce another subcommand to continue watching the creation progress at any time. |
There was a problem hiding this comment.
I don't think this should be included in this RFC. It's scope creep in my opinion. Watching for cluster creation isn't "technically" related to applying of the cluster app. There's a lot of layers and processes that are involved to make that happen. Instead, we already have the kubectl gs get clusters that should be preferred in my opinion.
| If the user chooses to create the cluster immediately (by not specifying an output destination or format), we want to provide detailed progress on the creation. The user should be encouraged to quit watching the progress at any time, without any effect on the provisioning progress. | ||
|
|
||
| We may even introduce another subcommand to continue watching the creation progress at any time. | ||
|
|
There was a problem hiding this comment.
This RFC seems to be missing anything about how a cluster would be upgraded/modified after creation
(can I re-run the create command and it'll overwrite? Will it block me because a cluster with the same name already exists? If it doesn't block, how can I avoid accidentally overwriting someone else's cluster named test?)
I think this RFC needs to discuss how / if / where this command would fit into a gitops workflow. We're pretty actively encouraging our customers to use gitops and this approach seems to go against that advice.
- `--output` is changed to be the dry run output format (which was previously specified with `--format`) - `--format` is removed - removed default output format - output format `manifest` is renamed to `yaml` to use the kubectl convention - output format `config` is renamed to `config-yaml`
|
This draft got some great feedback so far. In the recent commits I addressed the parts I could fix/improve. Please read the commit messages for details. The main comment now includes a list of open questions. |
| So far, Giant Swarm users created workload clusters mainly using these two methods: | ||
|
|
||
| - Via the `kubectl gs template cluster` CLI command | ||
| - Via the web user interface (happa) |
There was a problem hiding this comment.
for the sake of completeness, there is also the subcommand gitops workload-cluster. Probably out of the scope of this RFC.
There was a problem hiding this comment.
More exactly it is kubectl gs gitops add workload-cluster.
Since it has been asked how this spec helps in a GitOps scenario, one open task is still to address that question.
Regarding: Explain how this command fits into a GitOps scenarioThe dry-run output mode named `config-yaml is suited well for use in a GitOps scenario. Taking our workload-clusters-fleet repository as an example, the config here is very close to what the specified command would produce. There is a difference though, and to add a workload cluster to a GitOps repository, more is needed. The difference: We use placeholders like For adding a cluster to a GitOps repo, it takes a kustomization like this one and that customization has to be registered in some list. Adding these pieces is currently handled by the command Another GitOps use case will be creating a cluster via a UI (Backstage scaffolder). In that case, no CLI interaction would be required. The user should either interactively fill in config details via the hierarchical form (based on schema), or -- as a shortcut -- paste/upload config YAML stored locally / obtained from somewhere. This, too, is outside the scope of this RFC. |
to adapt to the Release concept, which is similar to the vintage one
|
As discussed in KaaS sync, my understanding is that users can specify the release version (if they don't want to use the latest, which is the default) instead of specifying a cluster app version. Hence I just replaced the |
|
With the latest commit 0afc0f1 I've changed the proposal to provide a |
|
@marians what's the status here? i'm trying to clean up RFCs in the old format |
AndiDog
left a comment
AndiDog
left a comment
There was a problem hiding this comment.
Veto against defaulting to immediate cluster creation 😉
| - `--dry-run`: only validate the configuration and optionally print the resulting manifest. | ||
| - `--output`: This flag can be used to specify the format of the output when `--dry-run` is set. See the "Dry run output" section below for details. | ||
| - `yaml` for a Kubernetes manifest that includes an App resource and one or several ConfigMap resources. | ||
| - `config-yaml` for merged configuration YAML. |
There was a problem hiding this comment.
What is the difference between an App+ConfigMap manifest vs. "merged configuration YAML"?
There was a problem hiding this comment.
The config-yaml should only include the values part (like values.yaml, not a ConfigMap), while the yaml would be several manifests for App, ConfigMap. "Merged" means that when using several sources and flags as an input, the output is only one YAML document.
There was a problem hiding this comment.
As mentioned in the SIG product call: yaml vs. config-yaml is unclear. I suggest --output {app,values}. And we should also reword the RFC here – "merged configuration YAML" should be turned into an unambiguous explanation.
|
|
||
| ## Dry run output | ||
|
|
||
| Combining `--dry-run` and `--output`, users can decide to print a representation of the desired cluster instead of creating it immediately. |
There was a problem hiding this comment.
For existing users, it's confusing that the command defaults to creating a cluster instead of printing a manifest. In the best sense of a Unix tool, it should do only one thing well, and rely on other tools (namely | kubectl apply -f -) to do out-of-scope actions. With GitOps being the overall goal for most users, it's quite surprising and unsafe to have the new command create a workload cluster immediately. CLIs should not use --something to change to a different mode – here, I'd expect --output yaml to print a manifest, which clearly is a different mode from applying a cluster manifest.
Also, we have lots of existing users of kubectl gs template cluster who already rely on certain flags. We'd need to migrate them to the new command and deprecate the command line arguments, or maintain the old command forever. Therefore, old and new command should be compatible. Same for internal users.
There was a problem hiding this comment.
For existing users, it's confusing that the command defaults to creating a cluster instead of printing a manifest.
I think this is the case when you think of template cluster as a reference.
Since this is a new command named create cluster, I hope that users understand this is about actually creating the cluster.
@AndiDog With my reply, how do you think about this? Do you still want to veto this? |
Yes. I don't think we're creating the right thing for customers here by changing the command and having two modes, where an extra flag is needed to work with the (IMO) 80%/safe case (GitOps / dry-run). |
|
Let's please drive this forward in a larger-scale synchronous meeting instead. Opinion juggling won't help here 😉. |
|
might need sig arch cc @JosephSalisbury |
|
@marians feel free to drop in sig arch agenda, but possibly more targeted meeting / andreas chatting with you / honeybadger directly to unblock is easier |
|
Since Andreas and I obviously have contradicting opinions on this, I would prefer some more people as tie breakers. I can't make it to the SIG architecture sync on Wednesday (the slot usually never suits me). So the next best thing would be SIG product next Monday. |
AndiDog
left a comment
AndiDog
left a comment
There was a problem hiding this comment.
After yesterday's SIG Product meeting, we seem to be clear that we really want to put priority on implementing this, so I'm fine to agree on either of the proposed technical implementation details (i.e. remove my veto) in order to avoid minor things to block this.
|
|
||
| The most important flags would be: | ||
|
|
||
| - `--provider`: (required) provider name. For example: capa, capvcd, capz, eks. |
There was a problem hiding this comment.
Better make it required for now and not use the MC to implicitly determine the type. I'm saying this because we may have more CAPx-WC-on-CAPy-MC mixed cases in the future, and requiring the argument leads to the least surprise.
| - `--dry-run`: only validate the configuration and optionally print the resulting manifest. | ||
| - `--output`: This flag can be used to specify the format of the output when `--dry-run` is set. See the "Dry run output" section below for details. | ||
| - `yaml` for a Kubernetes manifest that includes an App resource and one or several ConfigMap resources. | ||
| - `config-yaml` for merged configuration YAML. |
There was a problem hiding this comment.
As mentioned in the SIG product call: yaml vs. config-yaml is unclear. I suggest --output {app,values}. And we should also reword the RFC here – "merged configuration YAML" should be turned into an unambiguous explanation.
|
Do we still want to have this command? |
Yes we still want this |
9 participants
Towards giantswarm/roadmap#2115
Feedback is welcome.
Open discussions / TODOs
--providervalues valid for a given installation/management cluster?