parent values

[kfox1111]

In kolla-kubernetes we have a common library, kolla-common. It has no templates that don't start with '_' (no non macro charts).

This code is used by parent charts to launch k8s objects. This allows us to share large swaths of code between all the various packages/objects we have.

One fairly big maintenance burden we have seen though is that because the macro is called in objects in the parent chart, the values always come from the parent chart, and any values defined in the library subchart are always ignored. Since we have a lot of values for the common code, we end up either having to copy/paste/maintain those values in each parent charts values.yaml or have to generate the values.yaml to make it easier to maintain. We are currently doing the latter option, but both options are a pain. We need a more helm native solution.

We would like to propose the following solution to the problem:

1. Provide a new file in the chart. parent_values.yaml. The file format is identical to values.yaml.
2. When a chart is being loaded, right before it loads its own values.yaml, it first loads any parent_values.yaml files from its subcharts. It then merges in its own values.yaml, and then any values from the user.

In this way, common values can be loaded into a subchart and included in all the parents that need it, reducing complexity.

[jascott1]

@kfox1111 in this case would just using existing library chart values with a 'parent' key that gets merged up suffice?

[kfox1111]

that could work too, if there wasn't charts in existence that had a 'parent' key. Don't know if those exist.

[sbezverk]

How about adding a keyword --parent-values which could be a list of yaml files to merge before using chart's own values.yaml.
In this case we we can specify arbitrary location of parent value files and it is sequence will be also an order of precedence. 1st mentioned file will be overridden by 2nd etc. Thoughts?

[kfox1111]

the trouble with that would be it would require the user to do that on every 'helm package ' command. which might be too onerous.

But the idea of a list is interesting. If you wanted to be explicit, we could add it as a flag in requirements?

Or have a child_values.yaml file in the parent that specifies what child values to pull in?

May be more trouble then its worth though. not sure. thoughts?

[sbezverk]

@kfox1111 I do not see it as a issue when variable with that list is used, like we do for common_vars, then the user just need carefully set it up once and then refer to it from all subsequent commands.

[jascott1]

@sbezverk In my mind that flag would be a further addition to the parent key/parent_values solution but I would like to see this work without CLI params if possible. To me the parent key approach seems the least invasive if it would serve our needs and not wreck existing charts.

[sbezverk]

@jascott1 I understand your point, but having keyword always helps in scripting. Hiding this in a file, does not help much for scripting purposes but makes it much more difficult.

[jascott1]

@sbezverk Ok that confused me, we are talking about merging a child's values up to parent thus allowing child library chart to be loaded with its values and used by parent. What scripting are we talking about?

[sbezverk]

@jascott1 ok I was thinking about different direction, using parent's values in child's chart and the scripting was launching child chart. I guess it is a different topic then.. apologies..

[kfox1111]

To me, it is kind of the same thing as saying, we don't need a requirements.yaml file, and a helm deps up command, as you could:
cp -a ../../microservice/foo charts/; cp -a ../../microservice/bar charts;
helm package .

Having the list be part of the chart makes it easier for users to not have to maintain that info outside of helm somehow.

Having the list as part of the chart means they don't have to maintain a list outside of helm somewhere and then always pass it to helm.

[kfox1111]

Another possibility:

Say you have a lib with a single subchart, but it contains a dozen or so macro's that the parent can use.

Perhaps the parent wants to pull in just the values associate with the macro's it chooses to use?

So, what about this:
the subchart gets a directory called parent_values/
With individual files in there, such as: foo.yaml, bar.yaml.

The parent chart then has a list of childchart/value's filenames to include?

Requirements might be a good place for that. a new field per dependency that is:
parent_values:

• foo
• bar

if they use both. These listed files are loaded with the same algorithm described above in place of the parent_values.yaml.

[technosophos]

What if we could provide a way for the top-level chart to identify portions of subchart configuration that it wants to import? I've been working on a prototype for this, though it's really early.

The idea would be that the top level chart could basically say "augment my values.yaml with the values in /charts/foo that match the pattern foo.parent.*".

I'm very reluctant to allow subcharts to modify parent charts without "explicit consent" because of the potential for collisions and unintended side effects. So I'm trying to come up with a way for a parent chart to bless the elevation of scope for certain subchart values.

[kfox1111]

@technosophos yeah, I think thats kind of the direction the comment directly above yours may be going. The parent chart in its dependencies lists which chunks of the child's values it wants to include.

I wasn't thinking wild cards, but that could work too.
I was thinking something like:
parent requirements.yaml file:

dependencies:
  - name: kolla-common
    repository: http://localhost:10191
    version: 0.4.0-1
    include-values:  #<--- new section describing which chunks to pull in
      - common
      - svc

and the kolla-common has maybe a values.yaml like:

parent: # <--- new section where chunks named chunks are pulled out of.
  common:
     a: 1
     other:
        sometree: true
     b: 1
  svc:
     b: 2
  other:
     c: 3

so, the resulting values in the parent would be:

value_from_parent: 1
a: 1
other:
    sometree: true
b: 2

What do you think of this idea?

[sbezverk]

I do not like the idea of wildcards, as 1 it will kind of forces naming 2 if by mistake somebody uses a part of name which is in wildcards, some values could accidently being leaked into charts. Going with section name of included values sounds like a more reliable option.

[jascott1]

In this idea are the 'chunks to pull in' always root keys or we want arbitrary paths?

[kfox1111]

I think one level of grouping is probably sufficient, and paths are maybe overkill?

so, parent -> group -> stuff

where the user specifies the groups to include.

[jascott1]

Does child need to explicitly allow exports by using parent: in its own values? Why cant parent use include-values: in requirements.yaml to pick and choose any key from child? Parent could import anything from child without the need to alter the child.

[kfox1111]

yes. not all values in the child may be of interest to the parent. So tagging them explicilty into a parent: section, and making group for the parent to consume allows the child to make changes to the non parent values, or add additional groups to the parent: section without risking breaking parent charts.

[jascott1]

I get the more explicit contract idea but disagree with the assertion that it prevents parents from breaking. It would have its own section but any changes to it are likely to still break the parent.

Further would counter that child doesn't always know what is of interest to parent. Most importantly (imo) requiring parent: means that to get this out of the box, all charts maintainers have to do work to allow this behavior. In the end the chart is local and dev can modify but maintainer has no control at that point so why restrict and create more work all around? Also values would likely be duplicated unless we changed to more of an exports: and name the keys to export.

Requiring parent: would work for us because we control our many charts but I think it would help more users if it wasn't. Im not sure there is much integrity gained by requiring it.

[kfox1111]

So, are you thinking there isn't any changes required to the child. all the vars are just specified as normal,
then in the parent you do an import-values: with a list of paths to include, rooted at the path?

so, like values in child:

foo:
  thing:
    bar: baz
    wark: tros
    things:
      - 1
      - 2
      - 3
other:
  stuff: 1

with a parent deps:

include-values:
   - foo.thing

becomes:

bar: baz
wark: tros
things:
  - 1
  - 2
  - 3

in the parent?

[jascott1]

Yes. I also suspect we would need a path like in your example.

[jascott1]

Could use a dict in requirements so could be reparented in parent values but maybe thats too much complication for the simple case?

#requirements.yaml
import-values:
  foo.thing: global.foosystem

#parent values
global:
  foosystem: 
      bar: baz
      wark: tros
        things:
           - 1
           - 2
           - 3

[kfox1111]

The latter suggestion has one drawback I can see.

Some day, helm should have a way to crawl a chart and show all the default values that are there, to make it easier to tweak them.

helm show values some chart

#stuff include-value included from the child chart.
foo:
  bar: 1
#child charts settable values
childchartname:
  foo:
     bar: 1
  baz: #stuff provided by the lib chart but never used in the parent.
      wark: 2

When if done with an explicit marking with globals, it could return something like:

#just values included from child
foo:
   bar: 1

If the child chart doesn't have any values defined but a parent section, it can skip the 'childchartname' section entirely. or just include the values that aren't parent related.

If the values intended for parents are explicit in a section, then the tooling could exclude them from the list of values the child chart has.

What about a hybrid? tagging it explicitly with parent, but have path based include-values that work under the parent section?

[jascott1]

Nothing prevents someone/project following parent: convention. I just don' think its worth it to require it. To my last example though, I don't think you can have . in yaml key?

[kfox1111]

Yeah, the dict include-values idea would be by far the most flexible option. I'm unsure if that level of flexibility would ever be needed though.

Though, I think that could be added later, without changing the api, as one could be a list of strings, and the other would be a dict. So we could implement one before the other if we wanted to support a simple and and advanced version.

[jascott1]

True I was also thinking about using both and would be happy to implement :)

[kfox1111]

ah. I could go either way.

Maybe that should be asked of the other helm developers? There probably is a convention for values that aren't defined that should probably be followed.

[jascott1]

I may have set the convention in last PR :) or at least followed it unknowingly. But in tags and conditions it was to be minimally invasive so it only warns when things aren't there. We have our use case, what would you want to see if common lib didn't get loaded right?

[jascott1]

Just a thought but could always introduce yet another dimension with

-required-child:

[kfox1111]

I guess a warning is probably fine. Maybe we just go with that for now and during the code review others can poke at that. It shouldn't be hard to switch if people feel its better the other way.

[technosophos]

Man, you guys are on a roll today.

FWIW, you can include dots in YAML names if you quote the name:

"foo.bar": true

[jascott1]

Oh we were so close I thought :) If those are the only required key quotes in the system will we get hate email?

[kfox1111]

you can include dots in the key, but I don't think you can have only a dot?

".": "foo"

[technosophos]

@jascott1 I think we use them on occasion to do annotations, which often have a . in the key. But @kfox1111 may be right about only ".".

Are there cases where we'd want to work with these using --set? Because in that case, --set foo.\"bar.baz\"=zed kinds of things might get annoying.

[jascott1]

Im not sure --set would be used much here other than setting the final values in parents (not the child/parent paths from requirements). Sounds like we need to go with child/parent keys in list.
Heres an updated example based on our discussion:

Example with simple list

# parent values
mything: yay

# child values
foo:
  thing:
    bar: true
    baz: false
    other:
      - 1
      - 2
      - 3

# requirements.yaml (simple list)
...
import-values:
  - foo.thing

# result parent values
mything: yay
bar: true
baz: false
  other:
    - 1
    - 2
    - 3

Example with child/parent key

# requirements (child/parent)
...
import-values:
  - child: foo.thing
    parent: new.key

# result values
mything: yay
new:
  key:
    bar: true
    baz: false
    other:
      - 1
      - 2
      - 3

[sbezverk]

+1 child/parent key approach as it give better visibility to imported values in parent's values.

[jascott1]

@sbezverk we are discussing supporting both simple list and child/parent. Are you against including simple list?

[kfox1111]

Yeah, the dot in the key names would only be used in the dependencies file. It would not have any effect on --set or the users values files.

[sbezverk]

I do not like simple list approach, at least I would not use it as it might bring confusions.

[kfox1111]

the simple list is just a "here, just grab all the things" I think this is going to be the common use case we will hit.

with all the common stuff in kolla-common, I'd much rather have:

import-values:
  - common
  - api-python-deployment
  - pv

or something like that then:

import-values:
  - child: parent.common
    parent: .
  - child: parent.api-python-deployment
    parent: .
  - child: parent.pv
    parent: .

We'll get a lot of "whats a . do" kind of questions, and why is parent listed in child.... :/

[jascott1]

I would point out that we would still support

child: parent.api-python-deployment
parent: .

so will still get those questions :) but at least we can hide it if we choose.

[kfox1111]

right. I think that long hand would probably only be used by folks that were needing the flexible dict based approach for other things. so they already will have to have figured out how it works/accepted the complication.

[jascott1]

Cool. And as far as errors we were circling around Warning when an import path does not exist in child. Does anyone have any strong feelings about Warn vs Fail? Other than that I think if we can get a blessing I can create formal proposal and then implement.

[kfox1111]

@sbezverk with the shorthand, we can basically just take all the package specific stuff out of kolla-kubernetes all_values, and take the rest and put it in kolla-common/values.yaml under the 'parent' key.

Then in each microservices chart, we put in

include-values:
  - common
  - <list of other values that get included today with prebuild_microservices>

Should then be able to get rid of all the values logic out of prebuild entirely and make it just do localpath deps and then it becomes just like the services/computekit build scripts.

[jascott1]

Since they are both lists I think we can allow mixing of simple and child/parent formats. Thoughts?

[sbezverk]

@kfox1111 that would be great, one step towards more helm native way..

[kfox1111]

@jascott1 yeah, I guess thats true. mixing would work as one's a string and the others a dict.

[jascott1]

On more thing, if path already exists in parent who wins the merger?

[kfox1111]

in order of priorities:
first entry in include-values gets lowest priority
second entry in include-values gets next priority
.....
values from parent chart gets next priority
values specified by user gets highest priority.

highest priority wins.

[jascott1]

so parent values win over child values with --set winning overall?
if effective duplicates exist in import-values, the last one wins.

[kfox1111]

yeah. that way if the parent knows it wants to override a default that makes more sence in its particular case, it can easily do so. and the user gets total control in the end to override anything.

[jascott1]

Pretty sure we cannot order the actual requirements loading so two conflicts from different sub-charts in the same requirements would be non-deterministic.

[jascott1]

OK boiled all this down to new proposal for readability #1995

[jascott1]

@thomastaylor312 I believe we can close this as the proposal is formalized in #1995 and already implemented.

[thomastaylor312]

Ok, thank you for pointing it out @jascott1!