Feature: Expose `--sync-period` to Kustomize Build

[callum-tait-pbx]

It would be good to parameterise the --sync-period flag as part of the kustomize build with the default of 10 minutes? I would quite like this as part of the repo as I am trying to not go down the route of forking the repo and having a highly customised copy of this repo for my env, it will make maintaining the solution much easier if I can just use this repo and configure parameters instead. We use helm in my env so I don't know anything about writing kustomize builds really. I can look into it however if someone else knows how to do this first that would be amazing!

Issue detailing what I want to add as a parameter to the kutomize build https://github.com/summerwind/actions-runner-controller/issues/83

[mumoshu]

@callum-tait-pbx Thanks for writing this up! I'd like to add that, but I'm not sure how. As far as I know, kustomize does not have an ability to set arbitrary variables in the resources.

[mumoshu]

Maybe the helm chart (#91 by @stackdumper) would be the way to go?

[callum-tait-pbx]

I don't think it will, https://github.com/summerwind/actions-runner-controller/pull/91 all we are doing is taking the output of kustomize and putting the output in helm, all the configuration work needs to be done in kustomize. All helm is in that PR is a wrapper for deploying via helm, there isn't actually any helm work being done and I don't see how we could do any without either moving to helm entirely in the repo or maintaining 2 copies of the solution which is a non-starter really.

It's a good solution for providing a a chart for people in envs that use helm but it means all the clever works needs to be done in kustomize.

This requirment could be met in Helm quite easily as all Helm does is take your value.yaml and pastes the values into your chart as defined in your chart. This means we can output anything really into the yaml anywhere in the yaml. Does Kustomize not do the same thing?

@mumoshu updated so it's much clearer if you haven't seen :D

[callum-tait-pbx]

@mumoshu https://blog.stack-labs.com/code/kustomize-101/ from reading that, doesn't Kustomize let you just apply arbritary yaml with arbritary values along a defined path? If so, this should be possible, if only I knew anything about Kustomize 😆

If so could we not just always deploy the argument in kustomize with the value set to 10 in the repo and then if someone wants to deploy a custom value they just change the value in Kustomize?

[callum-tait-pbx]

In the meantime I would like to add a snippet to the docs detailing the flag a bit more. Can you confirm the possible values? Does it just take an int and treats it as minutes or do you need / can provide a unit too?

        args:
        - --enable-leader-election
        - --sync-period 5

[jbuettnerbild]

i would find it also helpful to know how to change the sync period. I have the feeling that the hra does not scale very well or fast enough.

[callum-tait-pbx]

Changing the sync period is very easy however it has to be done manually to the template and so has all the negatives of manual changes. https://github.com/summerwind/actions-runner-controller/issues/83 details where you need to make the change (as an argument to the manager container), you just need to make the change, build your template and deploy. As it's not a parameter in the kustomize build it's not very intuitive to do nor is it cleanly pipeline-able. It would be great if at a minimum, we could expose it to kustomize to make configuring it much easier and less custom.

@mumoshu I guess the ideal scenario would be defining this sort of thing in a ConfigMap and the manager pulling the value from there instead. Initially at least getting it to be a definable parameter as part of the build would be a great step forward though.

@jbuettnerbild be aware the default value is 10 minutes to prevent someone rate limiting themselves

[jbuettnerbild]

ok i found it. thx @callum-tait-pbx for the hint. - --sync-period=1m0s

[mumoshu]

@callum-tait-pbx What if you manage this kustomization.yaml

bases:
- git@github.com:github.com/summerwind/actions-runner-controller.git/config/default

patchesStrategicMerge:
- |-
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: controller-manager
    namespace: system
  template:
    spec:
      containers:
      - name: manager
        args:
        - --enable-leader-election
        - --sync-period=5m

so that it's matter of running kustomize build ./ to generate the manifests with --sync-period added to the controller container args?

$ kustomize build . | grep -C 5 sync-period
template:
  spec:
    containers:
    - args:
      - --enable-leader-election
      - --sync-period=5m
      name: manager
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:

[callum-tait-pbx]

I don't quite understand how to integrate this into the existing kustomize build. The only way I seem to be able to get Kustomise to like adding an additional argument is by adding it to the existing manager_auth_proxy_patch.yaml. As far as I can tell Kutstomize doesn't like 2 patches editing the same path, it considers them conflicting? We could just add - "--sync-period=5m" to manager_auth_proxy_patch.yaml and document you manage it there, I don't know how to parameterise it still though like we have done with the namespace? It really should be parameterised so that it can be managed programatically in a pipeline easily without having to go down an awkward custom sed wrapper or having to just manually manage it. In my case, specifically being able to pipeline the solution is important as I am ultimately wrapping the output of kustomize in Helm and so the config needs to be easily editable programatically and really should be done in 1 place, via the solutions chosen build tool.

The below yaml works, I just don't know how to parameterise the --sync-period value in the default kustomization.yaml

# This patch inject a sidecar container which is a HTTP proxy for the controller manager,
# it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.
apiVersion: apps/v1
kind: Deployment
metadata:
  name: controller-manager
  namespace: system
spec:
  template:
    spec:
      containers:
      - name: kube-rbac-proxy
        image: gcr.io/kubebuilder/kube-rbac-proxy:v0.4.1
        args:
        - "--secure-listen-address=0.0.0.0:8443"
        - "--upstream=http://127.0.0.1:8080/"
        - "--logtostderr=true"
        - "--v=10"
        ports:
        - containerPort: 8443
          name: https
      - name: manager
        args:
        - "--metrics-addr=127.0.0.1:8080"
        - "--enable-leader-election"
        - "--sync-period=5m"

[mumoshu]

@callum-tait-pbx You can't parameterize (in other words use build-time variables) in kustomize, afaik. So adding sync-period to manager_auth_proxy_patch.yaml won't make a big deal. Am I correct?

Currently, the only practical way in my mind would be https://github.com/summerwind/actions-runner-controller/issues/184#issuecomment-727132673 i.e. we write docs about possible customizations and example kustomization.yaml, so that you can add minimum "patches" to "only replace actions-runner controller container args". That's far from parameterization. But I think that's how kustomize works.

Parameterizaion may generally be done better with Helm than Kustomize. So in the future I'd suggest enhancing https://github.com/summerwind/actions-runner-controller/issues/184#issuecomment-726691832 to a standard helm chart that has templates that actually reads and processes values.yaml.

WDYT?

[mumoshu]

can tell Kutstomize doesn't like 2 patches editing the same path, it considers them conflicting?

Btw - yes. I believe the upstream issue for that is https://github.com/kubernetes-sigs/kustomize/issues/642

[callum-tait-pbx]

:( that sucks. Sounds like better docs is about all we can do, I'll create the PR for that. Additionally it would be helpful from my perspective to add in the - "--sync-period=5m" argument to the manager_auth_proxy_patch.yaml patch by default. The reason for this is the workflow I will be doing for implementing this in my environment is to just output the kustomize build to the charts template folder and deploy it as 1 giant template. As a result having the flag already a part of the template would mean i can manage the value programatically at least doing something like this:

cd config
kustomize build ./default > manifest.yaml
sed -in 's/sync-period=5m/sync-period=1m/' manifest.yaml
mv manifest.yaml ../charts/action-controller/templates

I can look at creating a proper helm chart if this is something we would be interested in? The problem with doing that is that you now have basically 2 copies of the manifests to maintain which is not great obviously. The actual manifests themselves don't change much so it might not be that painful but it's far from ideal and super awkward.

thoughts on those 3 suggestions?

• Better docs (I can do)
• Implement the argument in manager_auth_proxy_patch.yaml as part of the repo set to 5 mins (I can do)
• A proper Helm chart (I can do but it will take some time, https://github.com/summerwind/actions-runner-controller/pull/91 didn't attempt to create a proper helm chart for the manager side of things for the reasons I've stated)

EDIT to further expand on suggestion 2. I would like the repo to be setup as such that I am just cloning, setting parameters (in this case via sed), kicking off a build, moving the template into the helm structure and then publishing the chart to our internal helm feed. As a result I really really don't want add or remove any actual yaml as that makes the pipeline much much more complicated, I just want to replace values basically.

[mumoshu]

I wasn't sure initially but your further explanation on the suggestion 2 did make sense! Thanks about that.

For the third, perhaps we can start by simply running helm create and adding the result to this repo for collaboration. Then I can at least try to cover testing that in #168 to make it more maintainable.

[mumoshu]

For the first and the second, PRs are very much appreciated ☺️

[mumoshu]

@callum-tait-pbx I've managed to bootstrap the helm chart in #187. It supports setting sync-period like --set syncPeriod=5m, too. PTAL on the chart https://github.com/summerwind/actions-runner-controller/tree/master/charts/actions-runner-controller and it's usage https://github.com/summerwind/actions-runner-controller/blob/ccd86dce69067b86217573d4cf1f26b0cece1610/acceptance/deploy.sh#L25-L29

[callum-tait-pbx]

@mumoshu Well that was super quick! Awesome dude! I'll start testing with deploying through the repos helm setup as soon as I can!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[callum-tait-pbx]

Everything that could be done has been done at this point, closing issue;