Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--reuse-values does not work as expected with requirements conditions #2948

Closed
srossross opened this issue Sep 18, 2017 · 16 comments
Closed
Labels
bug Categorizes issue or PR as related to a bug. v3.x Issues and Pull Requests related to the major version v3

Comments

@srossross
Copy link

helm version
Client: &version.Version{SemVer:"v2.6.1", GitCommit:"bbc1f71dc03afc5f00c6ac84b9308f8ecb4f39ac", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.6.1", GitCommit:"bbc1f71dc03afc5f00c6ac84b9308f8ecb4f39ac", GitTreeState:"clean"}

What happened:

I have a simple chart with a subchart installed. In the requirements.yaml I have condition: subchart.enabled

Check out my gist of the the chart including bug-example-0-1-0-tgz:

helm install . --set subchart.enabled=true --set subchart.hello=Subchart --set hello=World 
NAME:   alternating-stingray
LAST DEPLOYED: Mon Sep 18 11:22:13 2017
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1/ConfigMap
NAME                                  DATA  AGE
subchart-config-alternating-stingray  1     0s
chart-config-alternating-stingray     1     0s

I see both the subchart and chart are installed. Now when I upgrade with --reuse-values I see that my subchart template is not included:

(srossross-mac bug-example •‿•) helm upgrade alternating-stingray . --reuse-values --debug --dry-run
[debug] Created tunnel using local port: '63488'

[debug] SERVER: "localhost:63488"

REVISION: 2
RELEASED: Mon Sep 18 11:27:28 2017
CHART: bug-example-0.1.0
USER-SUPPLIED VALUES:
{}

COMPUTED VALUES:
hello: World
subchart:
  enabled: true
  global: {}
  hello: Subchart

HOOKS:
MANIFEST:

---
# Source: bug-example/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: chart-config-alternating-stingray
data:
  hello: World
Release "alternating-stingray" has been upgraded. Happy Helming!
LAST DEPLOYED: Mon Sep 18 11:22:13 2017
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1/ConfigMap
NAME                                  DATA  AGE
subchart-config-alternating-stingray  1     5m
chart-config-alternating-stingray     1     5m

What you expected to happen:

The subchart is included in the upgrade.

@herrsebi
Copy link

I think this might be a more general issue with using --set on nested values. Setting one value appears to remove the entire subtree and only append the set node, even when using --reuse-values.

@danielkrainas
Copy link

danielkrainas commented Nov 1, 2017

Having a similar problem. We install a master chart (with many subcharts) and a large value set with -f staging.yaml and our CI does a --set subchart.image.tag=HASH --reuse-values on the master's release to update individual subcharts without needing access to staging.yaml. What happen after upgrade is the resulting helm get values RELEASE is only the values set by the CI:

subchart:
  image:
    tag: HASH

As the OP, I'd expect the entire coalesced value tree from the previous release. We're using v2.7.0

@danielkrainas
Copy link

Our problem was on our end. It was a combination of misunderstanding what helm get values RELEASE is meant to display and a misconfigured CI script; things seem to be working as expected now and --all gave us the value output we desired.

@sliverc
Copy link

sliverc commented Nov 28, 2017

I have also encountered this error with Helm v2.7.2. It seems that condition in requirements.yaml doesn't reuse values from previous install even though --reuse-values is used.

@withnale
Copy link

withnale commented Jan 4, 2018

Is there any update on this one? I'm seeing subcharts disappearing just by doing a helm upgrade --reuse-values on a parent chart with conditional subcharts

@ljnelson
Copy link

ljnelson commented Feb 1, 2018

I can't speak to Helm core, but I did port Helm to Java and the client-side processing involving requirements.yaml was very tricky and as a result I feel that I'm quite familiar with it.

Indeed, conditions and tags work at a high level by effectively causing chart surgery to be done in memory before the chart to be rendered crosses the wire to Tiller. So, by the time Tiller has the chart and its values to render, the chart it actually has is the synthetic one made by removing certain parts of the original.

@jascott1
Copy link
Contributor

jascott1 commented Feb 1, 2018

@ljnelson Pros and cons. Filtering on the client side means you can have much larger trees of charts that don’t blow the size limits by sending charts that will never be used. openstack/kolla-kubernetes is three levels deep and has ~twenty 2nd level charts and each of those contain 3rd level charts. We have about two hundred 3rd level charts.
https://github.com/openstack/kolla-kubernetes/tree/master/helm/microservice

I am convinced this is a bug and will check into it and see if I can come up with a solution.

@withnale
Copy link

withnale commented Feb 4, 2018

@jascott1 - looking at the code in client.go relating to this issue and my PR associated with issue #3242 I think this bug also affects import/export values.

@tsloughter
Copy link

It adds to the frustration that this isn't documented. --reuse-values acts like it can be used to keep the values you set with --set when you installed the chart. In my case this ends up with a dependencies being dropped and another, that shouldn't have been there, being deployed when I did an upgrade of a chart.

At the very least it should be made clear that you can not use --reuse-values for --set tags.<name>. Though ideally tags would even be set with a different argument than --set, unless reuse values starts to work with all --set arguments.

@jascott1
Copy link
Contributor

jascott1 commented Feb 7, 2018

@tsloughter Sorry you are having trouble. It is a bug, thats why the behavior is not documented.

@tsloughter
Copy link

2.10 was released and based on the changelog this is still a bug? I really think the docs should mention it, as a bug, no reason someone reading the docs should get the impression this works and have to find out the hard way it doesn't.

@bacongobbler
Copy link
Member

@tsloughter if you'd like to take a crack at a PR to mention this, we'd really appreciate it. We have very limited resources so we'd really appreciate the help in taking the time to make a PR. :)

@victornoel
Copy link

Any plan to get this fixed or at least better documented? This is hell to use :)
Why can't helm just take the previous values, and use them to evaluate if the conditions of a requirement holds or not… this seems a minimum that should be expected.

@bacongobbler @jascott1

@victorboissiere
Copy link

victorboissiere commented Nov 27, 2018

Same issue here. It is very frustrating.
@BenjD90

@jascott1
Copy link
Contributor

jascott1 commented Dec 7, 2018

Im labeling this v3 to ensure its looked at for the new version but it does not preclude a 2x fix.

@bacongobbler
Copy link
Member

fixed in Helm 3 via #6556.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

No branches or pull requests