Replace sample inventory by auto-generated variable references from defaults #11897
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: VannTen 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 |
k8s-ci-robot
added
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
VannTen
force-pushed
the
cleanup/remove_sample_inventory
branch
5 times, most recently
from
c3aebda to
09c9998
Compare
This is an alternative to the sample inventory, which does not need manual updating.
Sometimes the change done by pre-commit are not obvious, this should help.
VannTen
force-pushed
the
cleanup/remove_sample_inventory
branch
from
09c9998 to
008c8c9
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
It makes maintenance easier (no need to sync variables back to the samples) but not for users. The sample was here to show-up the variables that users most likely want to edit (e.g cluster version, oidc...) and not all of them for an easier on boarding / maintenance. About #10645
How is it an issue to commit ~20 files ? Why is it better to commit 2? The workflow stays the same (commit/push/pull etc...) |
|
To balance both side (show users subset of things to 'edit' and easier maintenance) something like: and would create something like: With all lines 'commented' to show that those are already set in default and can be removed if needed
|
Yes, that's the point, you still should only need to add two vars, for all the rest, kubespray defaults should handle it.
There is lots of stuff in the inventory/sample, and the majority is setup specific (cloud provider / container engine / network plugin). So while it certainly started that way, IMO is does not reflect the current state.
The issue is not having 20 files, it's having more than you need and not knowing if you need it. If you copy the sample inventory, unless you're recording everything in your commit message (which most people don't, and most people don't look at git history either), 6 months after, you won't remember if the variable you have in your inventory is there because you put it deliberately, or because it was in the sample inventory.
I think this would be very hard to maintain effectively, especially |
Then I would rather go in that direction.
Then it goes in the wrong direction, as a new user you would have no idea what you need what you don't need because we won't cherry pick few of them anymore (can argue there are too many already). They would just have a list of the 7000 knobs they can tweak.
Any non-commented value is intentional.
If it's set it's intentional. Another side effect is that changing a default in a role could break clusters, by copying a subset of default in the inventory, that the user "should" have reviewed it freeze the user configuration to a certain state. Let's keep providing a curated sample inventory (generated or not), I agree the current one is not optimal and too large, it's still better than none. |
| @@ -37,5 +37,6 @@ exclude_paths: | |||
| - tests/files/custom_cni/cilium.yaml | |||
| - venv | |||
| - .github | |||
| - docs/ansible/variable_reference.yml | |||
There was a problem hiding this comment.
let's keep grouping the variables by domain in multiple files.
then let's have people the cherry pick/copy past what they need:
cp docs/ansible/variables/cloud/gcp.yml inventory/mycluster/group_vars/
cp docs/ansible/variables/network/calico.yml inventory/mycluster/group_vars/
There was a problem hiding this comment.
Like, one file per role defaults ? That's mostly redundant with the defaults file themselves, no ?
Especially if we recreate the hierarchy.
There was a problem hiding this comment.
it's already redundant with roles.
not all per role but some hierarchy especially when configuration are exclusive:
- calico vs weave vs cilium...
- crio vs docker vs ....
- gcp vs azure ...
and then maybe some hierarchy but not down to the role:
- kubernetes/vars.yml (all cluster configs)
- apps/vars.yaml (all apps)
but can comes later too. need a bit more though on it.
| @@ -12,16 +12,16 @@ Heketi provides a CLI that provides users with a means to administer the deploym | |||
|
|
|||
| ## Install | |||
|
|
|||
| Copy the inventory.yml.sample over to inventory/sample/k8s_heketi_inventory.yml and change it according to your setup. | |||
| Copy the inventory.yml.sample over to `<inventory_path>/k8s_heketi_inventory.yml` and change it according to your setup. | |||
There was a problem hiding this comment.
https://github.com/heketi/heketi is abandoned let's drop it in another PR.
|
Then it goes in the wrong direction, as a new user you would have no idea what you need what you don't need because we won't cherry pick few of them anymore (can argue there are too many already). They would just have a list of the 7000 knobs they can tweak.
But you don't need to look at the reference. I think the playbook runs without any variable defined in inventory just fine (would need to check but I think anything we define in CI is optional).
> If you copy the sample inventory, unless you're recording everything in your commit message (which most people don't, and most people don't look at git history either), 6 months after, you won't remember if the variable you have in your inventory is there because you put it deliberately, or because it was in the sample inventory.
Any non-commented value is intentional.
Unfortunately, it's not.
Users are copying this directly, just look at the questions in slack
```
$ find inventory/sample/ -type f -exec cat {} + | grep -E '^[a-z0-9_]+:' | wc -l
128
```
At a guick glance, there is not a lot there that would need to be edited on a typical setup. (maybe LB stuff, not even sure).
Besides, the commented values have a bad effect as well. If a user copied the sample inventory (with commented value and comments
explaining it) in version N of kubespray, and now they upgraded a couple of times and are in N+3, with the same inventory.
Suppose we changed/deprecated a variable `variable_a`. `variable_a` will still be in the user inventory, commented out. If they try to change it (add oidc aauth for instance), or copy the sample to a new cluster, they'll have outdated documentation, or even a variable which Kubespray won't consider at all.
Docs should be kept in sync with Kubespray itself, and we can't do that if they're in the users inventories.
Another side effect is that changing a default in a role could break clusters, by copying a subset of default in the inventory, that the user "should" have reviewed it freeze the user configuration to a certain state.
But not all do, judging from the questions we get in slack. (And I don't think a majority do either)
We can't rely on this to avoid breaking changes breaking stuff, because we can't guarantee user inventories anyway.
|
|
I've linked this PR in the slack channel in the hopes we can have some users join the discussion, so we can have more info on how the sample inventory is used. |
|
I understand that sample is complex and a pain to maintain, I'm ok with that removal for that reason. Also I use it to upgrade my clusters by comparing my inventory folder with the sample one. I don't know how new users will start using Kubespray after that removal, and I'm affraid they will probably find another solutions to deploy clusters if they don't have a tool to generate inventory files with important default values. |
|
Jumped away from the samples inventory a long time ago and retain only the basics in 1 file. My inventory looks like this : I'm parsing the default values to get most of the information for the variable names and meaning When I explain the inventory to a colleague, they always came back with the question. "Why is there a separation in k8s_cluster and all in the group_vars" so we removed everything not needed that created this confusion, resulting in configuration.yml |
|
But on my side, when I started Kubernetes 4 years ago, it helped me SO MUCH to understand Kubernetes settings and features and was a great resource to start deploying a cluster.
Could you explicit ? Did you use it as documentation, reference, copying the inventory ?
Also I use it to upgrade my clusters by comparing my inventory folder with the sample one.
Like, something like `git diff <tag> <old_tag> -- inventory/sample` ? It was my impression that given we miss updating it pretty regularly.
I don't know how new users will start using Kubespray after that removal, and I'm affraid they will probably find another solutions to deploy clusters if they don't have a tool to generate inventory files with important default values.
The sample should not be 'default values' => that's what role defaults are for. And ideally (and it's seems to work given #11898) there is not much to define unless you need to customize (at which point you need to edit your inventory anyway).
|
I used as reference and started new cluster by copying and editing the sample folder to a new one to custom my cluster.
I use a file comparaison tool (an VSCode extension) with sample folder as source and my inventory folder as destination.
Yeah sure, but I used it to get some "default keys/values" with comments, without making search on the whole codebase to find keys I need to change (I ended up to done that for specific things). Overall I'm ok with the sample folder removal, because this PR adds generated variables reference file, it will be more reliable. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The two mains reasons to do this are:
Which issue(s) this PR fixes:
Fixes #11876
Special notes for your reviewer:
/label ci-extended
@MrFreezeex
@ant31
Does this PR introduce a user-facing change?: