community
community copied to clipboard
kubevirt
Add Helm charts proposal
This design proposal addresses the Helm chart installation method alternative. For reference: https://github.com/kubevirt/kubevirt/issues/8347
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
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.
Thank you for your input, @vasiliy-ul!
Regarding
approach #2I 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 https://github.com/kubevirt/kubevirt/issues/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.
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?
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 :
$ helm install kubevirt <repo-url>$ kubectl edit kv -n kubevirt kubevirt$ 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.
approach #3looks 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!
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
/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.
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.
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?
@vasiliy-ul do you have any updates on this one? Not exactly sure what the state is here?
@dhiller, please refer to the previous comment from @atanasdinov https://github.com/kubevirt/community/pull/224#issuecomment-1749101403
@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?
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.
@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.
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 https://github.com/akuity/kargo/pull/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
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
/remove-lifecycle stale @vladikr Did the above comments allay your concerns?
/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.