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

tpl function not performant #8002

Closed
himmakam opened this issue Apr 27, 2020 · 22 comments
Closed

tpl function not performant #8002

himmakam opened this issue Apr 27, 2020 · 22 comments
Labels
bug Categorizes issue or PR as related to a bug. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. Stale

Comments

@himmakam
Copy link

I have a scenario of having one umbrella chart with many sub-charts as dependencies. When I have the templates folder in umbrella chart, helm lint takes more time like almost 30 to 40 minutes. When this folder is not there, helm lint returns very fast. Can I know the reason.running in debug mode not giving any logs. It is helm v3.

@hickeyma
Copy link
Contributor

@himmakam Thanks for raising the issue.

How many sub-charts in the umbrella chart?

I have tested with 30 sub-charts and having a templates folder in the umbrella chart. helm lint is running and returning as expected. Would you be able to test with different number of sub-charts to see when the delay starts occurring? Have you nested sub-charts within sub-charts?

@hickeyma hickeyma added the v3.x Issues and Pull Requests related to the major version v3 label Apr 27, 2020
@technosophos
Copy link
Member

Providing a reproducible case would be great. I am currently working this sprint on helm lint issues, so I can take a look if someone can point me to a reproducible case.

@technosophos
Copy link
Member

I did recently uncover an issue where if you use tpl in your templates (a notoriously poorly performing function that should be avoided whenever possible), you can drive lint times quite high. Could that be the problem here?

@himmakam
Copy link
Author

himmakam commented May 5, 2020

@himmakam Thanks for raising the issue.

How many sub-charts in the umbrella chart?

I have tested with 30 sub-charts and having a templates folder in the umbrella chart. helm lint is running and returning as expected. Would you be able to test with different number of sub-charts to see when the delay starts occurring? Have you nested sub-charts within sub-charts?

@hickeyma I have a total of 120 sub-charts and do not have any nested sub-charts.

@himmakam
Copy link
Author

himmakam commented May 5, 2020

I did recently uncover an issue where if you use tpl in your templates (a notoriously poorly performing function that should be avoided whenever possible), you can drive lint times quite high. Could that be the problem here?

Might be because I use tpl many times in these charts. Also helm install also takes almost 30 to 45 minutes to parse all the 120 sub-charts I have.

@technosophos
Copy link
Member

I did a test with a massive chart with hundreds of subcharts. I changed the tpl function to return an empty string. It cut that chart's helm lint down from 2.5 hours to 2.08 seconds. If you want to test reproducing this, all you have to do is comment out this: https://github.com/helm/helm/blob/master/pkg/engine/engine.go#L127-L152 and then recompile

@himmakam
Copy link
Author

himmakam commented May 7, 2020

So are we saying helm lint has issue with umbrella chart and having massive sub-charts whereby it is taking 2.5 hrs? Can someone fix this?

@hickeyma
Copy link
Contributor

hickeyma commented May 7, 2020

@himmakam I think @technosophos is saying that the tpl function is not very performant. This becomes an issue when using the function in an umbrella chart containing a massive number of sub-charts, and then you run helm lint on it.

helm lint does not have an issue with an umbrella chart having massive number of sub-charts.

@himmakam
Copy link
Author

ok. so can I know the alternative where helm lint works fine with umbrella and massive sub-charts having this tpl function. I see that upgrade/install/template does not take this much time. Only lint with tpl is the issue. I would like to know if there is any other alternative.

@himmakam
Copy link
Author

himmakam commented Jun 9, 2020

Is there a plan to fix the performance issue related to tpl in future helm releases? Right now it is becoming very difficult to use this as it takes lot of time. Is there any other alternative for tpl?

@technosophos
Copy link
Member

There is no plan to change the way tpl works.

@himmakam
Copy link
Author

Is there any alternative for this, please

@himmakam
Copy link
Author

@himmakam I think @technosophos is saying that the tpl function is not very performant. This becomes an issue when using the function in an umbrella chart containing a massive number of sub-charts, and then you run helm lint on it.

helm lint does not have an issue with an umbrella chart having massive number of sub-charts.

@himmakam
Copy link
Author

We tried replacing tpl function with some other logic, but we are not successful. Basically would like to know what it takes to fix the tpl function. Also would like to know if there are any other alternatives for this so that we can proceed. Please do let me know if you need my sample code to look at it.

@hickeyma
Copy link
Contributor

@himmakam Would you be able to show a snippet of how you are using tpl function?

@himmakam
Copy link
Author

Sure. Lets assume that I have this below line in my properties file which needs to be mounted to the pod as part of the configmap -
memcache.server.url=${memcache.server.url}
Basically the above is a place-holder and dynamically I would like to pass the values for the above such that the final solution in pod could be as below -

memcache.server.url="https://10.10.11.11/remote-control
So in my tpl file, I have defined a property as below where 10.10.11.11 comes from parent values.yaml.

{{- define "memcacheserverurl" -}} {{- printf "https://%s/remote-control" .Values.global.service_url -}} {{- end -}}

And I have an another function, where am parsing all the property files and search for $, replacing with actual value using below logic.

{{- $updatedline := regexReplaceAll "\$\{(.*?)\}" $line "{{ include "${1}" $}}" }}
{{ tpl $updatedline $}}

Please do let me know if you need more information.

@himmakam
Copy link
Author

Basically helm upgrade --install command takes 40 minutes to 1 hour for any update. When I comment the tpl, it just runs in 2 or 3 minutes. I would like to know if it is issue with using tpl function?

@technosophos
Copy link
Member

I have already explained that it is DEFINITELY an issue involving tpl. To function in "proper isolation", tpl has to re-render the entire template tree on each invocation, creating a sandbox for each call. That is expensive. The bigger the chart, the more expensive the re-render is. And the more calls you make to tpl, the more frequently this re-render will be performed. tpl is simply incapable of scaling and should be avoided. It's basically the eval of Helm templates.

If someone wants to take another crack at implementing tpl while still maintaining the security model, I'm fine with that. But in lieu of that, the answer is simple: Don't use tpl. Use Helmfile or some other external tool for pregenerating, or ask in the Slack channel how other people did similar things.

@himmakam
Copy link
Author

ok. thanks a lot

@hickeyma hickeyma changed the title helm lint issue with umbrella chart having templates folder tpl function not performant Jun 18, 2020
@hickeyma hickeyma added bug Categorizes issue or PR as related to a bug. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed question/support v3.x Issues and Pull Requests related to the major version v3 labels Jun 18, 2020
@hickeyma
Copy link
Contributor

Thanks for the feedback @technosophos. I have edited the issue to describe the problem better.

It is open if someone in the community would like to try and improve the function.

diafour added a commit to diafour/helm that referenced this issue Jun 29, 2020
@diafour
Copy link

diafour commented Jun 29, 2020

Hello! Here is "another crack at implementing tpl" : #8371

I think it is compliant with "still maintaining the security model" as it uses Template.Clone to run tpl in isolation.

diafour added a commit to diafour/helm that referenced this issue Jun 29, 2020
See helm#8002

Signed-off-by: Ivan Mikheykin <ivan.mikheykin@flant.com>
diafour added a commit to diafour/helm that referenced this issue Jun 29, 2020
- backport to 3.2.4

See helm#8002

Signed-off-by: Ivan Mikheykin <ivan.mikheykin@flant.com>
diafour added a commit to diafour/helm that referenced this issue Jul 7, 2020
See helm#8002

Signed-off-by: Ivan Mikheykin <ivan.mikheykin@flant.com>
@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

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. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. Stale
Projects
None yet
Development

No branches or pull requests

4 participants