Add concurrency option for helmfile commands #81
Conversation
|
Hello @titirigaiulian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-10-18 14:14:36 UTC |
src/ops/cli/helmfile.py
Outdated
| cmd = ' '.join(args.extra_args + [args.subcommand]) | ||
| return "cd {} && helmfile {}".format(args.helmfile_path, cmd) | ||
| extra_args = ' '.join(args.extra_args) | ||
| helm_concurent_proc = '--concurrency={}'.format(args.concurrency) if args.concurrency else '' |
There was a problem hiding this comment.
Typo: concurent -> concurrent
src/ops/cli/helmfile.py
Outdated
| def configure(self, parser): | ||
| parser.add_argument('subcommand', help='plan | sync | apply | template', type=str) | ||
| parser.add_argument('extra_args', type=str, nargs='*', help='Extra args') | ||
| parser.add_argument('--concurrency', dest='concurrency', type=int, default=None, |
There was a problem hiding this comment.
Why not delegating the whole list of parameters to helmfile command.
I dont see the point of maintaining our parser for helmfile params/subcommand?
There was a problem hiding this comment.
This was the initial path I tried to implement, but we cannot have 2 sets of extra_args. Also adding a new arg with nargs='*' will break backwards compatibility running with -- --extra-args. Considering there are just a couple of subcommand arguments, I followed the easy way, that was already in place for terraform, playbook etc.
Another option is to keep one list extra_args with a separator and split it in 2: subcommand_extra_args and command_extra_args, but it is a bit hackish
There was a problem hiding this comment.
@amuraru, what @titirigaiulian discovered is that helmfile expects these params in a strange way. Some of them need to be specified before the subcommand (sync/apply/diff) and some of them after the subcommand. Example:
This works:
helmfile --log-level=info sync --concurrency=1
Doesn't work:
helmfile --log-level=info --concurrency=1 sync
helmfile sync --log-level=info --concurrency=1
Because of this, we need to split these args in two vars. One that goes before the subcommand (sync) and one that goes after it.
There was a problem hiding this comment.
Updated implementation to pass raw the subcommand/args
src/ops/cli/helmfile.py
Outdated
| # Run helmfile sync for a single chart | ||
| ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile sync -- --selector chart=nginx-controller | ||
| # Run helmfile sync with concurrency flag | ||
| ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile --concurrency=1 sync -- --selector chart=nginx-controller |
There was a problem hiding this comment.
Switch sync and --concurrency order here to keep it in line with what gets executed by helmfile
There was a problem hiding this comment.
In fact, it won't work running with an additional option followed by extra arguments like --concurrency=1 -- --selector chart=test
|
Would it make sense to make concurrency=1 as a default if it creates issues? |
|
@amuraru, default helmfile is with concurrency=0, used with tiller and maybe with helm 3. I wouldn't alter the default behavior of helmfile |
src/ops/cli/helmfile.py
Outdated
| ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile sync -- --selector chart=nginx-controller | ||
| ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile -- --selector chart=nginx-controller sync | ||
| # Run helmfile sync with concurrency flag | ||
| ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile -- --selector chart=nginx-controller sync --concurrency=1 |
There was a problem hiding this comment.
Better :) but can we avoid the double dash?
|
Nicey! |
|
Nice @titirigaiulian! |
4 participants
Description
Add support for helmfile execution with
--concurrencyoptionRelated Issue
Fixes #80
Motivation and Context
Running multiple helmfile release tillerless can cause concurrency issues.
roboll/helmfile#690
Usage:
ops data/env=dev/region=va6/project=ee/cluster=experiments/composition=helmfiles helmfile --concurrency=1 sync -- --selector chart=nginx-controllerTypes of changes