Skip to content

Add Helm charts proposal #224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 15, 2025

Conversation

atanasdinov
Copy link
Contributor

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

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jun 28, 2023
@kubevirt-bot kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 28, 2023
@kubevirt-bot
Copy link
Contributor

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.

Signed-off-by: Atanas Dinov <[email protected]>
@atanasdinov atanasdinov force-pushed the add-helm-chart-proposal branch from 358d304 to bdd730a Compare June 28, 2023 16:30
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jun 28, 2023
@atanasdinov atanasdinov marked this pull request as ready for review June 28, 2023 16:36
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2023
@kubevirt-bot kubevirt-bot requested a review from jobbler June 28, 2023 16:36
Copy link
Contributor

@vasiliy-ul vasiliy-ul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Implementation Phases

- Create a KubeVirt Helm chart
- Configure a Helm repository (using GitHub pages is probably the best option)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@scydas scydas Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We should be able to plug in the functionality to generate a `Chart.yaml`, `values.yaml` and possibly hooks and organize those in the necessary file structure.
The charts can then be generated, packaged and pushed to the configured Helm repository at build time.

### Approach 2: Manually curate a Helm chart
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@atanasdinov atanasdinov Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this makes perfect sense. The initial goal of the proposal was not to be invasive with regards to how the manifests are currently generated hence why I didn't include it.

@atanasdinov
Copy link
Contributor Author

atanasdinov commented Jun 30, 2023

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
Copy link
Contributor

vasiliy-ul commented Jul 3, 2023

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
Copy link
Contributor Author

atanasdinov commented Jul 3, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@kubevirt-bot kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 1, 2023
@aburdenthehand
Copy link
Member

/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.

@kubevirt-bot kubevirt-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 3, 2023
@atanasdinov
Copy link
Contributor Author

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
Copy link

davenull commented Feb 8, 2024

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?

@kubevirt-bot kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2024
@aburdenthehand
Copy link
Member

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

@kubevirt-bot kubevirt-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 8, 2024
@vladikr
Copy link
Member

vladikr commented Jul 8, 2024

/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
Copy link
Member

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

@Jmainguy
Copy link

Jmainguy commented Dec 2, 2024

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.

Comment on lines +30 to +31
* [kubevirt/kubevirt](https://github.com/kubevirt/kubevirt)
* [kubevirt/containerized-data-importer](https://github.com/kubevirt/containerized-data-importer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's perfectly fine. I can update the proposal to mark that manually curated Helm chart in a separate Git repository is the selected approach.

We still have to decide on the chart layout, though. I assume the safest route is with the second option. Does the community agree?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not in the same chart as mentioned in #224 (comment) ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both CRDS and CR's that use the CRD cant reliably be in the same chart. long standing helm issue. :/

As most folks want to deploy the operator, and then use it to immediately launch kubevirt, which object do you seperate? The CRDs or the CRs?

I generally like to manually apply static CRDs via kubectl, as they can be rather fiddly, then launch everything else with helm.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I see, then I would prefer also the second option as mentioned above

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both CRDS and CR's that use the CRD cant reliably be in the same chart. long standing helm issue. :/

I know that helm says it's not reliable, but kube-prometheus-stack does it - and that is a rather popular project which heavily uses CRDs.

But they can't be upgraded directly.

One can use a dedicated chart that only contains the CRDs, also like what kube-prometheus does.

There are also other, less standard options, like putting all the CRD's in the template-dir for helm, instead of in the CRD-directory, like what fluxcd-community-charts do

It would be really nice to be able to use a only helm-chart-way of managing clusters - automations with gitops and helm is very nice, and having to drop to a command-line (or write an automation that does) kubectl apply isn't great at scale, fluxcd has ways of handling this already, I assume ArgoCD has the same.

(But, I'm also a completely new person to kubevirt, so I have no idea of how kubevirt handles/changes CRDs over time, this might be a bigger issue to handle than what I know. I am subscriped to this topic, and eagerly await a helm-chart'y-way to install this project, as the current implementation comes with a bigger initial cost for me/my company, since we so heavily invested in helm and flux. I wanted to chime in as the approach of "just kubectl apply the CRD-upgrades" isn't a great scaling-option, and other options exist.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a crds chart separate from the main chart is a common solution. It still works with gitops.

sticking them in the templates directory does not work around the issue of having CRDS and CR's in the same chart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for having helm charts is a separate repo in kubevirt org

Option 2 also seems to make the most sense to me

@davenull
Copy link

davenull commented Jan 18, 2025 via email

@xpivarc
Copy link
Member

xpivarc commented Feb 27, 2025

@aburdenthehand How do we move this forward?

@scydas
Copy link

scydas commented Mar 5, 2025

Two years have passed

@aburdenthehand
Copy link
Member

@xpivarc - Unless I'm mistaken, this was waiting for the KubeVirt community to voice a preference for the chart layout. This lack of preference could be taken as agreement with the suggestion of option2. I will ping some of the ppl previously tagged in this PR and raise it again in the community meeting for input.
@atanasdinov It looks like the proposal has not been updated to specify that the charts will be maintained in a new and separate repo - is that right or am I misreading? It also looks like you need to sign one or more of your commits - if it's easiest, you can squash all commits into a single signed commit after updating it. If we're creating a new repo in the kubevirt project, it would be good to have a second person to help maintain these charts.

@atanasdinov
Copy link
Contributor Author

@aburdenthehand I'll update this with the chosen approaches (even if we hadn't had that much input from the community) in order to have this go under final review and hopefully get merged.

On a separate note, I've had all commits signed but I'll double check as soon as I update this. Thanks!

@atanasdinov atanasdinov force-pushed the add-helm-chart-proposal branch from bd74697 to 4c60784 Compare April 14, 2025 10:12
@atanasdinov
Copy link
Contributor Author

atanasdinov commented Apr 14, 2025

Updated the different sections with "Decision" subsections covering the selected approaches respectively. I intentionally did not rewrite the initial options in order to showcase those were considered and to keep the constraints clear.

We still need to decide where to publish and host the charts (i.e. HTTP(s) repository vs OCI registry). We can choose to address this in the scope of this proposal or at a later stage.

Let me know how you'd want to proceed.

CC: @aburdenthehand

@aburdenthehand
Copy link
Member

/lgtm
@atanasdinov Thank you for the update! I think that the name/details of the repo can be decided later.
I'll aim to get some more +1s here so we can merge it. In the meantime, can I ask you have a look at the community membership guidelines and apply: https://github.com/kubevirt/community/blob/main/membership_policy.md#member
You're welcome to tag me to sponsor.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2025
@vladikr
Copy link
Member

vladikr commented Apr 14, 2025

@atanasdinov Thank you. Could you please open an issue to track this proposal in kubevirt/enhancements repo?

@vladikr
Copy link
Member

vladikr commented Apr 14, 2025

@aburdenthehand @atanasdinov Thank you for this effort!
Everything looks good to me. However, something is still not clear to me. Are we going to create a new repo for this? Will you update the proposal with the name?
Beside this this, I'm +1
/approve
/hold
I'll let @lyarwood and @xpivarc have a final look.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2025
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vladikr

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2025
@atanasdinov
Copy link
Contributor Author

Are we going to create a new repo for this? Will you update the proposal with the name?

We already have it here -- kubevirt/charts seems to be a straightforward option but I'm open to discussion.

@xpivarc
Copy link
Member

xpivarc commented Apr 15, 2025

+1
@vladikr a new repository

@vladikr
Copy link
Member

vladikr commented Apr 15, 2025

Are we going to create a new repo for this? Will you update the proposal with the name?

We already have it here -- kubevirt/charts seems to be a straightforward option but I'm open to discussion.

I missed that, thank you!
@atanasdinov Please don't forget to open a tracker in the kubevirt/enhancements repo?
/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2025
@kubevirt-bot kubevirt-bot merged commit e41cd90 into kubevirt:main Apr 15, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.