Simple implementation of the tillerless mode #531

pvalsecc · 2019-04-02T13:10:27Z

Partial implementation of #449

mumoshu

Thanks for your efforts!
Several comments, but LGTM overall ☺️
Please notify me once this gets out of WIP, or you feel like wanting an another look.

helmexec/runner.go

helmexec/exec.go

mumoshu · 2019-04-03T13:12:08Z

helmexec/context.go

@@ -0,0 +1,18 @@
+package helmexec
+
+type HelmContext struct {


I wonder if we can safely merge this into execer. I have been long thinking that execer is the "context" and adding an another context "HelmContext" isn't a must? 🤔

No can do. execers lifecycle is not tied to the release, but is allocated once per helmfile. And it is used in // per multiple threads. That is why I ended up passing a context to most methods.

Ah that's good point. I dreamed of changing it to use a dedicated execer per release, but turned out it affects a lot of code including tests. Let's use HelmContext for now.

Thanks!

pvalsecc · 2019-04-03T13:55:46Z

@mumoshu, I just have a pain point, but I don't know if I can fix it. The tillerless mode works only with --concurrency=1 if you have more that one release per tillerNamespace. I've documented this limitation in the readme, but, I don't know if you guys can live with it...

mumoshu · 2019-04-03T14:34:02Z

@pvalsecc Why is it tied to concurrency=1? Tiller ports' collision?

pvalsecc · 2019-04-03T14:35:31Z

Why is it tied to concurrency=1? Tiller ports' collision?

Yes, I guess... it's not like there was any error message.

mumoshu · 2019-04-03T14:37:55Z

@pvalsecc Perhaps setting a dedicated HELM_TILLER_PORT value per release works? accordinvg to helm tiller --help it starts from 44134 so... do we need to create one HelmContext per concurrency = goroutine?

pvalsecc · 2019-04-03T14:39:03Z

Yes, I just came to the same conclusion. I'll try tomorrow.

mumoshu · 2019-04-03T14:39:31Z

Um.. I'm not sure if multiple tillers can safely operate on the same tiller namespace. Does tiller have a kind of exclusive lock on tiller namespace?

mumoshu · 2019-04-03T14:41:05Z

Maybe helmfile should cap concurrency per tiller-namespace to 1. I believe it's fine to run concurrent helm-tillers across different tiller namespaces, though.

pvalsecc · 2019-04-03T14:41:18Z

Tiller runs on your local machine, in this mode. All it uses are secrets and you have one set of secrets per release, named after the release. So it should be quite safe to deal with the different releases in parallel.

mumoshu · 2019-04-03T14:45:08Z

All it uses are secrets and you have one set of secrets per release, named after the release

Makes sense. Thanks for your support ☺️

pvalsecc · 2019-04-04T08:31:10Z

hummm, looks like HELM_TILLER_PORT doesn't work and nobody really cares:
rimusz/helm-tiller#30
helm/helm#3159

pvalsecc · 2019-04-04T08:55:41Z

@mumoshu, IMHO, this PR is ready for your review.

mumoshu · 2019-04-04T09:48:09Z

Makefile

@@ -42,6 +42,7 @@ clean:
 .PHONY: clean

 pristine: generate fmt
+	git diff | cat


What do we need this for?(Just curious

At work, I have an old version of go that doesn't agree with the autoformatting of the one in circleci. So I needed the actual output of the diff instead of just the list of files.

Got it. Thanks for clarifying 👍

mumoshu

Several nits but LGTM overall! I'll merge this shortly but i'd appreciate it if you could confirm if those nits are valid 😄 :smi

helmexec/context.go

mumoshu · 2019-04-05T03:19:55Z

helmexec/exec_test.go

@@ -258,7 +286,7 @@ func Test_TestRelease_Flags(t *testing.T) {
 	var buffer bytes.Buffer
 	logger := NewLogger(&buffer, "debug")
 	helm := MockExecer(logger, "dev")
-	helm.TestRelease("release", "--cleanup", "--timeout", "60")
+	helm.TestRelease(HelmContext{Tillerless: false}, "release", "--cleanup", "--timeout", "60")


Nit but can we safely omit Tillerless:false as its the default(zero) value?

What do you mean by "nit"?
OK, thanks. I never did write any go until previous PR... so I'm learning... ;-)

Thanks for taking your time :)

"Nit" stands for "nitpick" so I mean "I'm hesitant to point out such a small thing even though your PR is awesome overall, but…" ☺️

mumoshu · 2019-04-05T03:25:46Z

helmexec/exec.go

-	out, err := helm.exec(append([]string{"secrets", "dec", name})...)
+	preArgs := context.GetTillerlessArgs(helm.helmBinary)
+	env := context.getTillerlessEnv()
+	out, err := helm.exec(append(append(preArgs, "secrets", "dec", name), flags...), env)


I thought so but please let me confirm - You run helm-tiller because helm-plugin is tied to tiller, even when the actual plugin code(helm-secrets in this case, which doesnt access tiller itself) isnt?

Exactly. Looks like helm, before calling any plugin, tries to talk to its server. Sadly inefficient.

mumoshu

LGTM. Thanks a lot for your contribution @pvalsecc ☺️

pvalsecc · 2019-04-05T11:38:30Z

Thanks for the quick release, @mumoshu !

mumoshu · 2019-04-05T11:58:14Z

@pvalsecc Thanks for the great work!

Also, I've submitted a tiny PR to helm that makes the probe port configurable, which should allow us to support concurrency > 1

I'd appreciate it if you could take a look 😃

mumoshu · 2019-04-07T05:07:00Z

The PR has been merged 🎉

pvalsecc · 2019-04-08T07:07:27Z

Thanks for the merge and release.

I'd appreciate it if you could take a look

It's already merged :-)

But then there would be more work on the helm-tiller plugin to use that feature. So many layers...

pvalsecc force-pushed the tillerless branch 6 times, most recently from 2e6fea4 to fdcaeab Compare April 2, 2019 14:02

pvalsecc changed the title ~~[WIP] Simple implementation of the tillerless mode~~ Simple implementation of the tillerless mode Apr 2, 2019

pvalsecc force-pushed the tillerless branch 2 times, most recently from 2daabff to 869ed46 Compare April 3, 2019 05:36

pvalsecc changed the title ~~Simple implementation of the tillerless mode~~ [WIP] Simple implementation of the tillerless mode Apr 3, 2019

mumoshu reviewed Apr 3, 2019

View reviewed changes

pvalsecc changed the title ~~[WIP] Simple implementation of the tillerless mode~~ Simple implementation of the tillerless mode Apr 3, 2019

pvalsecc force-pushed the tillerless branch from 0049a6b to 3f8b9b3 Compare April 4, 2019 07:17

Patrick Valsecchi added 2 commits April 4, 2019 09:28

Simple implementation of the tillerless mode

979a78b

Fix tiller namespace for secrets

80170eb

pvalsecc force-pushed the tillerless branch 2 times, most recently from b587cfa to 495ad79 Compare April 4, 2019 07:39

Fix @mumoshu's comments

16f2109

pvalsecc force-pushed the tillerless branch from 495ad79 to 16f2109 Compare April 4, 2019 08:30

mumoshu reviewed Apr 4, 2019

View reviewed changes

mumoshu approved these changes Apr 5, 2019

View reviewed changes

Fix @mumoshu's comments take 2

2d1c488

mumoshu approved these changes Apr 5, 2019

View reviewed changes

mumoshu merged commit 1acd07f into roboll:master Apr 5, 2019

pvalsecc deleted the tillerless branch April 5, 2019 11:38

mumoshu mentioned this pull request Apr 5, 2019

feat: make tiller probeAddr configurable helm/helm#5557

Merged

mumoshu mentioned this pull request Apr 10, 2019

panic: runtime error: index out of range with v0.54.0 #550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple implementation of the tillerless mode #531

Simple implementation of the tillerless mode #531

pvalsecc commented Apr 2, 2019 •

edited

mumoshu left a comment

mumoshu Apr 3, 2019

pvalsecc Apr 3, 2019

mumoshu Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 4, 2019

pvalsecc commented Apr 4, 2019

mumoshu Apr 4, 2019

pvalsecc Apr 4, 2019

mumoshu Apr 5, 2019

mumoshu left a comment

mumoshu Apr 5, 2019

pvalsecc Apr 5, 2019

mumoshu Apr 5, 2019

mumoshu Apr 5, 2019

pvalsecc Apr 5, 2019

mumoshu left a comment

pvalsecc commented Apr 5, 2019

mumoshu commented Apr 5, 2019

mumoshu commented Apr 7, 2019

pvalsecc commented Apr 8, 2019

Simple implementation of the tillerless mode #531

Simple implementation of the tillerless mode #531

Conversation

pvalsecc commented Apr 2, 2019 • edited

mumoshu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 3, 2019

mumoshu commented Apr 3, 2019

pvalsecc commented Apr 4, 2019

pvalsecc commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

pvalsecc commented Apr 5, 2019

mumoshu commented Apr 5, 2019

mumoshu commented Apr 7, 2019

pvalsecc commented Apr 8, 2019

pvalsecc commented Apr 2, 2019 •

edited