Feature: add Helm chart install mode #98

knmsk · 2022-02-02T12:18:01Z

Hi everyone,
I moved most of things out of kustomize to helm chart. I had some problems trying to set-up the MutationWebhookConfiguration and ValidatingWebhookConfiguration, if anyone can help with this i will be glad.
Also, i added some liveness and readiness probe configuration on the controller-runtime in case someone will need it too due to requirements of the company cluster.

Closes #92

AbdelrhmanHamouda · 2022-02-18T09:03:51Z

Hello @knmsk,
Any plans to have this as part of the 0.0.7 release?

knmsk · 2022-02-21T16:17:50Z

probably after the discussion of issue #92

CLAassistant · 2022-04-04T15:07:09Z

All committers have signed the CLA.

guillermotti · 2022-05-11T10:41:47Z

Hi @knmsk!!

Do you know when this PR will be merged? I created a custom Helm Chart meanwhile but would be great to use the official one.

Thanks for your work! Amazing ❤️

knmsk · 2022-05-15T15:58:55Z

Hi @knmsk!!

Do you know when this PR will be merged? I created a custom Helm Chart meanwhile but would be great to use the official one.

Thanks for your work! Amazing ❤️

@guillermotti i thought about having a helm and a operator format to use, so probably i'll undo the operator removal and ask @yorugac to review it. But at my company we already use this PR to deploy our k6-operator

yorugac · 2022-05-23T16:48:18Z

Hi @knmsk! My apologies for such a terrible delay but I'm finally getting to your PR 😅

This is just a heads-up as I've only started looking at the PR and testing: I'll go through all details and then add any further comments. This is really neat, and I'm amazed it covers everything, including CI!

The main thing I wanted to mention for now is the UX one: I think we should avoid adding Helm as default build everywhere. I.e. it should be an option, not a requirement. Meaning, Makefile should support both kustomize and Helm, and CI tests running separately for both. WDYT on changing approach to having Helm as an option?

I was also thinking to update controller-gen separately as there seems to have been issues with installation. But here this update is included as part of PR. If it is updated separately, this PR might need a rebase.

phlegx · 2022-06-28T12:20:55Z

Hi there! What is the status of this Pull Request. We would need this Helm way as well!

yorugac · 2022-07-04T13:20:59Z

Hi @phlegx, this PR is planned to be merged for the next big release, v0.0.8. The release will not be earlier than August / September, but hopefully, Helm support itself will be merged before that.

yulianovikova · 2022-09-19T12:02:36Z

Hi!
Is this PR still planned for the v0.0.8 release? As far as I can see, 0.0.8rc1 does not include this feature yet :( Maybe you could also tell me when Helm support is planned to be released?
We are looking forward to this feature to be merged. Thank you!

yorugac · 2022-09-23T15:36:41Z

Hi @yulianovikova,

Unlike k6, k6-operator does not have any set schedule around releases. The reason is simple: the time resources allocated for it are limited and workload is impacted by external requests and now major issue #138. So previous estimation became invalid. I still would like to add Helm first but cannot say the dates at the moment. TBH, I'm not happy about this either. I'll try to see if anything can be improved and will update again if / when there are changes.

For now, I added next pre-release v0.0.8-rc3 (the rc2 has a CI issue), to have a new proper tagged image.

charts/templates/prometheus/serviceMonitor.yaml

StianOvrevage · 2022-12-06T21:04:08Z

I also had a look at the PR. Looks very thorough and good.

Only thing I would like to mention is the lack of the standard/recommended K8s & Helm labels: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ & https://helm.sh/docs/chart_best_practices/labels/ .

If you create a helm chart with helm create test you'll get a _helpers.tpl file with some functions that are very widely used, such as:

{{/*
Common labels
*/}}
{{- define "test.labels" -}}
helm.sh/chart: {{ include "test.chart" . }}
{{ include "test.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

And it's used in deployment.yaml like this:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "test.fullname" . }}
  labels:
    {{- include "test.labels" . | nindent 4 }}

I don't include namespace in the templates since that's only redundant when passing --namespace to helm anyway, and would only cause problems and confusion if forgotten or misconfigured in the templates. But probably more of a personal opinion than pure fact.

The scaffold helm create charts also contain a few more configurable options on a Deployment that I think would be important for some:

affinity
nodeSelector
tolerations

Dariusch · 2022-12-06T21:28:10Z

while we are at it:
Is the prometheus ServiceMontitor really necessary? Afaik prometheus metrics are pushed to the remote write gateway and not scraped (via a ServiceMonitor).

mclite

Any updates on this? I'd like to start using it my company. Also can you make sure the Helm chart gets added to AWS EKS Blueprints

knmsk · 2023-03-11T16:25:56Z

I've updated the code with the suggestions that @StianOvrevage and @Dariusch mentioned

while we are at it:
Is the prometheus ServiceMontitor really necessary? Afaik prometheus metrics are pushed to the remote write gateway and not scraped (via a ServiceMonitor).

@Dariusch I think it isn't necessary anymore, can @yorugac or someone confirm this?

mclite · 2023-03-13T13:30:13Z

Ill test this in the next few days

zalbiraw · 2023-03-31T16:39:04Z

Any updates? :)

chrisduong · 2023-04-11T09:21:35Z

I had used this chart, and haven't found any major issues. Please merge asap 🙂

danielfigueiredo · 2023-04-13T13:07:52Z

I tried installing this branch by doing:

helm package /path/to/charts/folder
helm install k6-operator-0.0.1.tgz

But got a few errors because the template output (after processing) wasn't producing valid k8s resources. Nothing big, but I made a PR against @knmsk 's branch with minor fixes after I got a successful install, followed by successful test runs using the newly installed operator.

It would be fantastic if we could get this merged! It facilitates installs on private company repositories using the apps of apps pattern, and I believe it might significantly help with adoption 😄

vazhnov · 2023-04-19T15:08:27Z

I've successfully installed the K6 operator from Helm chart with the freshest code changes.
I think it is good time to merge the changes. Any improvements can be done in new merge requests.

knmsk · 2023-04-19T23:30:08Z

Hey @danielfigueiredo can you also sign the licence/cla?
I want to get this PR ready, so when a Maintain wants to merge it, it's ok

danielfigueiredo · 2023-04-21T20:11:24Z

Hey @danielfigueiredo can you also sign the licence/cla? I want to get this PR ready, so when a Maintain wants to merge it, it's ok

Thanks for the prompt @knmsk , CLA signed!

Not sure why it hasn't updated yet, maybe there is a refresh cycle. The agreement is signed and my email used is my primary email for this github account so should all be good. I'll keep an eye but please let me know if there is anything else missing here

JeffCT0216 · 2023-05-04T01:54:46Z

Thanks @knmsk for working on this!
I'm currently waiting for this to be merge to deploy using helm, is there an estimate on when this will be merged?

@Dariusch

yorugac · 2023-08-18T14:15:07Z

It seems there was a somewhat similar case of CLA confusion over here:
grafana/mimir-prometheus#269 (comment)

Apparently, making a "Co-authored-by" commit allowed to bypass the problem. Github docs on such commits. It's still strange but perhaps that could work here as well.
cc @danielfigueiredo @knmsk

knmsk · 2023-08-18T14:37:12Z

Just to make sure that I got it, I need to make a commit with a manual co-authored-by @danielfigueiredo to bypass this problem, right?

danielfigueiredo · 2023-08-18T14:42:10Z

Okay, @knmsk I just managed to fix it!
Ended up adding the other email to my github account temporarily so that w can bypass this issue
should be unblocked now
thanks @yorugac for all the help too

knmsk · 2023-08-18T15:14:18Z

Sorry, had to do a rebase since I've got some merge problems that I usually not run into 😢

EDIT: Got it!

Added missed PodSecurityContext in K6 CRD. Tested in minikube with default e2e/test.js

undefined

author knmsk <kino.massaki@gmail.com> 1643746211 -0300 committer knmsk <kino.massaki@gmail.com> 1692377913 -0300 changed the build to helm instead of kustomize changed the build to helm instead of kustomize changed to helmlint moved the charts folder fix: helpers rendering namespace incorrectly Added chart-testing workflow fix: ct list-changed fix: chart-testing only when has differences fixed the image from the samples revert: remove the kustomize, now supports both revert: changes on go.mod fix the image build fix: makefile identation revert: controller-gen version 0.4.0 to 0.3.0 fix: makefile bundle error fix: yamllint file on github actions

Added missed PodSecurityContext in K6 CRD. Tested in minikube with default e2e/test.js

apply suggestions fix: e2e-test for kustomize fix: makefile tab

undefined

yorugac

@knmsk, this is an amazingly large work, thanks! 😄

I've gone through the main points: most of the change requests are small and should be relatively quick to fix. Quite a few of them seem to be just leftovers because of rebasing lots of commits at once.

Other than those leftovers, most noticeable change request is around names: see the comment in _helpers.tpl. As it is, the installation doesn't work for me ATM.

There are also a couple of questions I'd like to clarify with you, e.g. in values.yaml and Makefile.

Btw, thank you for including CI workflows! One of the main blockers for merging this was the additional effort required to maintain the chart: working CI would help with that 👍 I hope they'll work out-of-the-box.

.github/workflows/chart-test.yaml

.github/workflows/e2e-test-helm.yaml

.github/workflows/helmlint.yaml

Makefile

charts/values.yaml

charts/samples/customAnnotationsAndLabels.yaml

charts/templates/crds/k6.yaml

charts/values.yaml

charts/templates/_helpers.tpl

yorugac

The changes are working as far as I checked 🙌 There will probably be small adjustments we'll have to do in the future but otherwise, it looks pretty neat!

@knmsk, I'm marking this as a change request because of bundle/: I'm not clear why it was added. We might return to it if we ever get to #3 but it shouldn't be required for Helm support, AFAIS. Could you please remove it?

A note for the future: each update of CRD(s) will need to be copy-pasted to the charts/templates/crds in order to be picked up by the next chart release.

charts/values.yaml

knmsk · 2023-09-06T21:08:11Z

@yorugac yeah, its not supposed to have a bundle folder committed at least.
done making the changes! Can you take a look again please?

yorugac

LGTM! The scenarios I've tested work and the structure is straight-forward 👍

Thank you again for the work and updates and esp. patience, @knmsk and everyone who helped 😄

yorugac · 2023-09-12T12:16:45Z

charts/templates/clusterRole.yaml

+# permissions for end users to edit privateloadzones.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: privateloadzone-editor-role
+rules:
+- apiGroups:
+  - k6.io
+  resources:
+  - privateloadzones
+  verbs:
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - k6.io
+  resources:
+  - privateloadzones/status
+  verbs:
+  - get
+---
+# permissions for end users to view privateloadzones.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: privateloadzone-viewer-role
+rules:
+- apiGroups:
+  - k6.io
+  resources:
+  - privateloadzones
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - k6.io
+  resources:
+  - privateloadzones/status
+  verbs:
+  - get


These roles are not actually included by default:
https://github.com/grafana/k6-operator/blob/main/config/rbac/kustomization.yaml
The same as with k6-editor-role and k6-viewer-role.
At first glance, these don't seem to make an impact one way or another so ~ future candidates for removal.

yorugac self-requested a review February 3, 2022 11:56

Dariusch reviewed Dec 6, 2022

View reviewed changes

charts/templates/prometheus/serviceMonitor.yaml Show resolved Hide resolved

mclite reviewed Feb 16, 2023

View reviewed changes

knmsk force-pushed the feat/helm-chart branch from 3a1c3f5 to 902bd0c Compare March 11, 2023 15:35

knmsk requested review from mclite and Dariusch and removed request for yorugac and mclite March 11, 2023 15:39

knmsk changed the title ~~Feature moving to Helm from Kustomize~~ Feature: add Helm chart install mode Mar 11, 2023

changed the build to helm instead of kustomize

c1f727d

knmsk and others added 11 commits August 18, 2023 13:36

Feat: Add Helm chart install

0103db9

Added upgrade and --wait for helm installation.

9ce8196

Added missed PodSecurityContext in K6 CRD. Tested in minikube with default e2e/test.js

Remove comment for test only-render manifests.

34610cd

apply PR suggestions

954b899

fix: minor template issues preventing a clean install

750c7bb

Update Helm chart with updated CRDs and RBAC (#4)

0e04b5f

undefined

Added upgrade and --wait for helm installation.

7d8a4bc

Added missed PodSecurityContext in K6 CRD. Tested in minikube with default e2e/test.js

Remove comment for test only-render manifests.

28f2aa7

apply suggestions fix: e2e-test for kustomize fix: makefile tab

Update Helm chart with updated CRDs and RBAC (#4)

90bd0f5

undefined

add: readme e2e-helm when using the helm install

fc370da

knmsk force-pushed the feat/helm-chart branch from 1d3f445 to fc370da Compare August 18, 2023 17:26

yorugac requested changes Aug 22, 2023

View reviewed changes

yorugac mentioned this pull request Aug 22, 2023

Publish Helm chart on releases #277

Closed

ksemele mentioned this pull request Aug 22, 2023

[Improvement Request] Refactor kustomize to a helm chart template #92

Closed

knmsk added 2 commits August 24, 2023 20:51

fix: apply suggestions from yorugac

3a66cf3

fix: yamllint and add charts as exception

3f5da95

yorugac requested changes Sep 1, 2023

View reviewed changes

charts/values.yaml Outdated Show resolved Hide resolved

knmsk added 2 commits September 6, 2023 18:01

Merge branch 'main' of github.com:knmsk/k6-operator into feat/helm-chart

d4c3cf5

remove bundle folder and fixed a typo

3d2df35

knmsk requested a review from yorugac September 6, 2023 21:06

yorugac approved these changes Sep 13, 2023

View reviewed changes

yorugac merged commit 2967283 into grafana:main Sep 13, 2023
3 checks passed

yorugac added this to the 0.11 milestone Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add Helm chart install mode #98

Feature: add Helm chart install mode #98

knmsk commented Feb 2, 2022

AbdelrhmanHamouda commented Feb 18, 2022

knmsk commented Feb 21, 2022

CLAassistant commented Apr 4, 2022 •

edited

Loading

guillermotti commented May 11, 2022

knmsk commented May 15, 2022

yorugac commented May 23, 2022

phlegx commented Jun 28, 2022

yorugac commented Jul 4, 2022

yulianovikova commented Sep 19, 2022

yorugac commented Sep 23, 2022

StianOvrevage commented Dec 6, 2022

Dariusch commented Dec 6, 2022

mclite left a comment •

edited

Loading

knmsk commented Mar 11, 2023 •

edited

Loading

mclite commented Mar 13, 2023

zalbiraw commented Mar 31, 2023

chrisduong commented Apr 11, 2023 •

edited

Loading

danielfigueiredo commented Apr 13, 2023 •

edited

Loading

vazhnov commented Apr 19, 2023

knmsk commented Apr 19, 2023

danielfigueiredo commented Apr 21, 2023 •

edited

Loading

JeffCT0216 commented May 4, 2023 •

edited

Loading

yorugac commented Aug 18, 2023

knmsk commented Aug 18, 2023

danielfigueiredo commented Aug 18, 2023

knmsk commented Aug 18, 2023 •

edited

Loading

yorugac left a comment

yorugac left a comment

knmsk commented Sep 6, 2023 •

edited

Loading

yorugac left a comment

yorugac Sep 12, 2023

Feature: add Helm chart install mode #98

Feature: add Helm chart install mode #98

Conversation

knmsk commented Feb 2, 2022

AbdelrhmanHamouda commented Feb 18, 2022

knmsk commented Feb 21, 2022

CLAassistant commented Apr 4, 2022 • edited Loading

guillermotti commented May 11, 2022

knmsk commented May 15, 2022

yorugac commented May 23, 2022

phlegx commented Jun 28, 2022

yorugac commented Jul 4, 2022

yulianovikova commented Sep 19, 2022

yorugac commented Sep 23, 2022

StianOvrevage commented Dec 6, 2022

Dariusch commented Dec 6, 2022

mclite left a comment • edited Loading

Choose a reason for hiding this comment

knmsk commented Mar 11, 2023 • edited Loading

mclite commented Mar 13, 2023

zalbiraw commented Mar 31, 2023

chrisduong commented Apr 11, 2023 • edited Loading

danielfigueiredo commented Apr 13, 2023 • edited Loading

vazhnov commented Apr 19, 2023

knmsk commented Apr 19, 2023

danielfigueiredo commented Apr 21, 2023 • edited Loading

JeffCT0216 commented May 4, 2023 • edited Loading

yorugac commented Aug 18, 2023

knmsk commented Aug 18, 2023

danielfigueiredo commented Aug 18, 2023

knmsk commented Aug 18, 2023 • edited Loading

yorugac left a comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

knmsk commented Sep 6, 2023 • edited Loading

yorugac left a comment

Choose a reason for hiding this comment

yorugac Sep 12, 2023

Choose a reason for hiding this comment

CLAassistant commented Apr 4, 2022 •

edited

Loading

mclite left a comment •

edited

Loading

knmsk commented Mar 11, 2023 •

edited

Loading

chrisduong commented Apr 11, 2023 •

edited

Loading

danielfigueiredo commented Apr 13, 2023 •

edited

Loading

danielfigueiredo commented Apr 21, 2023 •

edited

Loading

JeffCT0216 commented May 4, 2023 •

edited

Loading

knmsk commented Aug 18, 2023 •

edited

Loading

knmsk commented Sep 6, 2023 •

edited

Loading