Add Helm charts proposal

[atanasdinov]

This design proposal addresses the Helm chart installation method alternative. For reference: kubevirt/kubevirt#8347

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign alonakaplan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubevirt-bot]

Hi @atanasdinov. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vasiliy-ul]

Hi @atanasdinov, great proposal. Thanks a lot for bringing it up here.

I had an initial look and here are my thoughts on the topic.

From the end-user perspective, I personally like the approach #1. It is simple and less error-prone (no chance for the user to uninstall the resources in wrong order or smth like that). However, the simplicity comes at the cost of additional logic within the hooks that needs to be maintained.

approach #3 looks like a good alternative which basically replicates the existing flow with the manifests. It is well-known to the users already. Also, from the implementation and maintenance point of view, it does not seem to add any complexity. And in fact I think that by using the hooks it might be possible to e.g. prevent uninstallation of the operator if the CR is still present in the cluster. Thus enforcing proper uninstallation order. The only downside I see for now with this approach is related to potential inconsistency between operator/cr charts versions. When updating KubeVirt/CDI you usually do not need to update the CR, only the operator (and CRD). Therefore, after an update, the CR chart will stay at version N while the operator will move to N+1. Maybe this is in fact not a big issue at the end. But still maybe worth thinking about it... if it may cause some problems or confusions for the users.

Regarding approach #2 I think there might be an issue if it always installs/updates the CR resource. Users may have some settings defined in the CR, and I assume those will be overwritten by an update.

Just to summarize: I am not sure about #2 but #1 and #3 look fine.

[vasiliy-ul]

I do not see it mentioned in the proposal, but Helm also supports charts deployment from OCI registries: https://helm.sh/docs/topics/registries/. I think it's also an interesting option to consider. Though, I do not know if there are limitations or potential compatibility issues from the perspective of quay.io. Quick googling suggests that it should work https://cloud.redhat.com/blog/quay-oci-artifact-support-for-helm-charts

[atanasdinov]

This is indeed a viable option. I'll include it.

[scydas]

In my helm, I use approach approach #1.
But when upgrading crd need to do it manually, when deleting use pre-delete to delete crd.
I agree that using #1 reduces the number of steps and the number of helm repos

[vasiliy-ul]

Helm charts in fact use the content of the manifests. Since this content is generated for every build, it should be relatively easy to also structure it properly for Helm and produce the related artifacts. Those can be released as *.tgz or via quay.io. IMHO, it would be a better way, comparing to manual maintenance in a separate repository.

[atanasdinov]

The Generate Helm chart(s) approach definitely has the better end result. However, most charts are highly customisable and even if we don't take such approach, I imagine we'd still template some values so the manifests will not be completely the same.

The users would be able to work with their preferred configuration without relying on kubectl patch and / or manually modifying manifests before applying them e.g. configuring custom options for the CR would be possible via something like $ helm install kubevirt-cr <repo-url> --set configuration.developerConfiguration.cpuAllocationRatio=5

This is the only thing we'd need to keep in mind.

[Infinoid]

I am not a kubevirt developer, but I had a thought.

I understand the motivation for generating one from the other. It's DRY, right? But generating a helm chart from the manifests seems backwards to me. Couldn't you generate manifests from the helm chart, instead, using helm template and default values?

Patching templates into manifests sounds like it will become tricky to keep up with changes to the manifest in future releases. Working in the other direction seems trivial to me (just apply the template).

That way, you'd get:

• a single source of truth (the helm chart)
• an easy, low-maintenance release process (just run helm template)
• guaranteed consistency between the two installation methods
• consistency of template parameters and values across versions
  • no scramble to update patches for new versions
• ability to add as much customization as you want, without exponentially increasing complexity

I may be wrong, maybe it's a terrible idea. But I was surprised to see no one mention it yet.

[vasiliy-ul]

I think this should be rather "update KubeVirt from N-1 to N release using Helm charts"

[atanasdinov]

This is definitely the better wording, will use it. Thanks!

[atanasdinov]

Thank you for your input, @vasiliy-ul!

Regarding approach #2 I think there might be an issue if it always installs/updates the CR resource. Users may have some settings defined in the CR, and I assume those will be overwritten by an update.

Custom options will not be overwritten on upgrade. However, I ran into kubevirt/kubevirt#2533 when upgrading KubeVirt from v0.58.0 to v0.58.1 with the single chart approach which requires further investigation.

I'm currently testing all the different setups and will update the proposal accordingly after I'm done.

I had missed some of the RBAC changes from v0.58.1 which were causing the upgrade to fail. The single chart approach did work properly once I added those.

[vasiliy-ul]

Custom options will not be overwritten on upgrade.

Just wanted to double-check this. So, assuming there is a valid KubeVirt installation running on my cluster and I have enabled some feature gates by editing the CR resource k edit kv -n kubevirt kubevirt:

...
spec:
  certificateRotateStrategy: {}
  configuration:
    developerConfiguration:
      featureGates:
      - WorkloadEncryptionSEV
...

The generated kubevirt-cr.yaml now looks like below:

---
apiVersion: kubevirt.io/v1
kind: KubeVirt
metadata:
  name: kubevirt
  namespace: kubevirt
spec:
  certificateRotateStrategy: {}
  configuration:
    developerConfiguration:
      featureGates: []
  customizeComponents: {}
  imagePullPolicy: IfNotPresent
  workloadUpdateStrategy: {}

I assume it will look similar when generated for the Helm chart. If I do smth like k apply -f _out/manifests/release/kubevirt-cr.yaml then definitely I will end up in overwriting the existing feature gates with featureGates: [].

But what will happen on update with Helm? Does Helm perform the same kubectl apply ... or does it merge the existing resource with the new one?

[atanasdinov]

Helm 3 will consider the previous manifest, the current live state and the new manifest.

The flow you described will keep the manually edited feature gate WorkloadEncryptionSEV even after :

1. $ helm install kubevirt <repo-url>
2. $ kubectl edit kv -n kubevirt kubevirt
3. $ helm upgrade kubevirt <repo-url> --version <version>

We can introduce a relevant functional test covering such scenarios.

More info on the topic from the official Helm documentation.

[atanasdinov]

approach #3 looks like a good alternative which basically replicates the existing flow with the manifests. It is well-known to the users already. Also, from the implementation and maintenance point of view, it does not seem to add any complexity. And in fact I think that by using the hooks it might be possible to e.g. prevent uninstallation of the operator if the CR is still present in the cluster. Thus enforcing proper uninstallation order. The only downside I see for now with this approach is related to potential inconsistency between operator/cr charts versions. When updating KubeVirt/CDI you usually do not need to update the CR, only the operator (and CRD). Therefore, after an update, the CR chart will stay at version N while the operator will move to N+1. Maybe this is in fact not a big issue at the end. But still maybe worth thinking about it... if it may cause some problems or confusions for the users.

Regarding the versioning concerns, upgrading the operator using the manifests will actually also upgrade the CR. I've confirmed that this also works for Helm upgrades with approach #1.

This means that the CR chart can stay at version N while the Operator / CRD chart is at version N + M. We'd only need to upgrade if there was a change to the CRD and / or there's a particular feature which we'd want to enable / disable in the standard CR manifest.

All this also applies to approach #2 where we keep a separate CRD chart instead. There wouldn't be a reason to upgrade unless there is a change there and as the product matures such changes will be less and less frequent.

Regenerating the same CR or CRD charts (depending on the approach) is definitely not necessary and we can opt for doing so only when something has changed. However, I agree that it may bring confusion to the users.

---

There is also the option to attempt creating subcharts (either of CR or CRD) as dependencies of the main chart and install a single chart only, however we'd need to have multiple hooks taking care of the ordering which would make it so complex that it didn't even make the exotic options list.

---

Please let me know if any of the comments have not been fully addressed, thanks!

[kubevirt-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[aburdenthehand]

/remove-lifecycle stale
@atanasdinov @vasiliy-ul - am I right to assume that we still want to pursue this? If so, I will see about getting some more eyes on this.

[atanasdinov]

Hello, @aburdenthehand! We're still interested in contributing a Helm chart and would definitely appreciate it if we are able to get more people review the proposal and decide on the design and implementation options.

In the meantime, we plan on using a manually curated one which we'll bump to the GA release soon.

Please keep in mind that (depending on the implementation approach) we might not be able to start working on it right away due to limited capacity.

[davenull]

As an outsider, but a very heavy user of this project, is there any assistance that I can offer to help speed this through review?

[dhiller]

@vasiliy-ul do you have any updates on this one? Not exactly sure what the state is here?

[vasiliy-ul]

@dhiller, please refer to the previous comment from @atanasdinov #224 (comment)

[xpivarc]

/cc @vladikr @davidvossel
/cc @mhenriks @awels

[vladikr]

@atanasdinov It's a nice proposal. My only concern is how it can be maintained and supported as part of core KubeVirt? I doubt we have any expertise around Helm...
Could this be set up as a sub-project? Simar to @kubevirt/application-aware-quota ?
Since it's part of the KubeVirt org all kubevirt related tests can run on any new PR?

[davenull]

The main issue with not having a chart or kustomize is CD, Argo and other cd systems are built around using charts, but kubevirt/cdi requires some funny dancing around to shoehorn into the workflow.

[atanasdinov]

@vladikr I believe it's easier to maintain the chart if it's in the same repository as the source code but I've also seen examples of the opposite. It's pretty much up to the preferences of the maintainers.

[jkleinlercher]

Regarding the dependency between CRDs and CRs: there are other very popular charts which deploy CRDs as templates in the same chart as the CRs, e.g.: https://github.com/akuity/kargo/tree/main/charts/kargo/templates

see also https://github.com/akuity/kargo/releases#chart-improvements
and the issue for this change akuity/kargo#1671

so from this it should work to have CRDs in the same chart and have them as templates so that CRD upgrades should also work

[kubevirt-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[aburdenthehand]

/remove-lifecycle stale
@vladikr Did the above comments allay your concerns?

[vladikr]

/remove-lifecycle stale @vladikr Did the above comments allay your concerns?

My preference would be to keep this chart outside of the kubevirt/kubevirt repo but in the kubevirt org.

[aburdenthehand]

@davidvossel @mhenriks @awels can you please take a look at this?

[Jmainguy]

As an end user, I wanted to say I really like this proposal, was searching for the helm chart today to discover one did not exist yet (managed by the project). The proposal is very well written and I can tell a good amount of work has gone into it and this ongoing discussion. I really appreciate everyone involved and look forward to when this is accepted.

[aburdenthehand]

@atanasdinov @vladikr
As I understand it, this might be the only thing actually blocking this moving forward - and I see in comments/have heard quite a bit of support for this proposal :)
Is it reasonable for the helm charts to be in their own repo, which can then be maintained outside of kubevirt and CDI repos by folks with particular experience in helm charts?