add patches to virt-operator #3612

ashleyschuett · 2020-06-19T12:21:16Z

What this PR does / why we need it:

Adds the CustomizeComponents field to the KubeVirt CR
Allows for optional Patches to be specified

This PR will allow users to customize the virt-controller, virt-handler, virt-api, and all other resources that are created by the virt-operator.

We need this to allow users to be able to customize the way kubevirt is ran in each of their environments. This gives users the power to specify where components can be scheduled, customizing flags and other use cases. If a user defines an invalid patch the operator will fail to deploy.

This current draft PR only adds this logic for the virt-controller. An example KubeVirt CR is as follows:

---
apiVersion: kubevirt.io/v1alpha3
kind: KubeVirt
metadata:
  name: kubevirt
  namespace: kubevirt
spec:
  certificateRotateStrategy: {}
  configuration: {}
  customizeComponents: 
    patches:
    - resourceType: Deployment
      resourceName: virt-controller
      patch: '[{"op": "remove", "path": "/spec/template/spec/containers/0/livenessProbe"}]'
      type: json
    - resourceType: Deployment
      resourecName: virt-controller
      patch: '{"metadata":{"annotations":{"patch": "true"}}}'
      type: strategic
  imagePullPolicy: IfNotPresent

Which issue(s) this PR fixes:
Fixes #3365

Special notes for your reviewer:

How do we feel able the name customizeComponents? I also ended up using the name ComponentsCustomization in the config struct. I think these two should be consistent, but would like opinions on what naming makes more sense?
Is there any reason to continue passing individual arguments to functions such as NewControllerDeployment instead of passing the who config object?

Release note:

Adds `customizeComponents` to the kubevirt api

openshift-ci-robot · 2020-06-19T12:21:18Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ashleyschuett · 2020-06-22T08:29:53Z

@davidvossel @rmohr does this approach look correct to implement for all components?

davidvossel

i left one comment about code structure. The other kind of high level comment I have is how do we detect when a new patch has been added which should result in rollling out a new install strategy.

So for example, if someone installs kubevirt, then mutates the kubevirt spec to include a patch, how does our controller know to roll out that change. Right now it's only keying off image name/registry. We

davidvossel · 2020-06-22T18:36:26Z

pkg/virt-operator/install-strategy/strategy.go

 	if err != nil {
 		return nil, fmt.Errorf("error generating virt-apiserver deployment %v", err)
 	}
 	strategy.deployments = append(strategy.deployments, apiDeployment)

-	controller, err := components.NewControllerDeployment(config.GetNamespace(), config.GetImageRegistry(), config.GetImagePrefix(), config.GetControllerVersion(), config.GetLauncherVersion(), config.GetImagePullPolicy(), config.GetVerbosity(), config.GetExtraEnv())
+	controller, err := components.NewControllerDeployment(config.GetNamespace(), config.GetImageRegistry(), config.GetImagePrefix(), config.GetControllerVersion(), config.GetLauncherVersion(), config.GetImagePullPolicy(), config.GetVerbosity(), config.GetExtraEnv(), config.GetPatchesForResource("Deployment", components.ControllerDeploymentName))


I wonder if instead of passing in the patches to each create function (like deployments, api, daemonset, etc...) if we could just loop over all the objects at the of this GenerateCurrentInstallStrategy instead.

Something like

// services for i, entry := range strategy.services { strategy.services[i], err = applyPatches(entry, getPatchesForResource(.....)) if err != nil { return nil, err } } // deployments for i, entry := range strategy.deployments { strategy.deployments[i], err = applyPatches(entry, getPatchesForResource(.....)) if err != nil { return nil, err } }

maybe there's even a generic function that can be used to do this so it would be as simple as

genericApplyPatches(strategy.Services, config) genericApplyPatches(strategy.Deployments, config) . . .

I'm just trying to think if there's a way to consolidate all the patching logic to this generate function rather than spreading it around.

Yeah I think that makes sense, I have updated apply patches to be more generic so it only needs to be called for each resource type that we want to allow patches on

timstoop · 2020-06-30T08:09:03Z

Don't want to creep in additional features, but I wonder if this would be applicable to virt-launchers as well? As we would like to add a sidecart with the libvirt_exporter (https://github.com/kumina/libvirt_exporter/), for example.

rmohr · 2020-06-30T08:16:39Z

Don't want to creep in additional features, but I wonder if this would be applicable to virt-launchers as well? As we would like to add a sidecart with the libvirt_exporter (https://github.com/kumina/libvirt_exporter/), for example.

Independent if that makes sense (probably does): We are exposing metrics for all VMIs through virt-handler, in case you are not aware of it. Let us know if you are missing something there. Have a look at https://kubevirt.io/user-guide/#/installation/monitoring?id=monitoring-kubevirt-components

ashleyschuett · 2020-07-13T08:47:13Z

So for example, if someone installs kubevirt, then mutates the kubevirt spec to include a patch, how does our controller know to roll out that change. Right now it's only keying off image name/registry.

It gets updated from this function since the correct install strategy will no longer exist
https://github.com/kubevirt/kubevirt/pull/3612/files#diff-bd81c399ded8070883a8f861d513f5e8R442

danielBelenky · 2020-07-26T14:55:59Z

staging/src/kubevirt.io/client-go/api/v1/utils.go

+	s := "CustomizeComponentsPatches"
+
+	for _, p := range c.Patches {
+		s += fmt.Sprintf("Resource%sName%sPatch%sType%s", p.ResourceType, p.ResourceName, p.Patch, string(p.Type))


I think that in this case using strings.StringBuilder will be much more efficient.
Strings are immutable and for every patch we actually reallocate a new memory chunk and reference s to the new location.

davidvossel

code quality so far looks great! I have a couple of top level comments.

what happens (or should happen) if someone modifies the KubeVirt CR with a patch after install? It looks like we might ignore those changes today until the KubeVirt version is updated, then the patches would kick in?
i think this logic deserves a functional test case in tests/operator_test.go

davidvossel · 2020-07-30T18:25:19Z

pkg/virt-operator/creation/components/deployments.go

@@ -335,6 +335,7 @@ func NewControllerDeployment(namespace string, repository string, imagePrefix st
 	}

 	attachCertificateSecret(pod, VirtControllerCertSecretName, "/etc/virt-controller/certificates")
+


super minor, there's a random line added here to a file that otherwise has no changes.

rmohr · 2020-07-31T07:20:13Z

what happens (or should happen) if someone modifies the KubeVirt CR with a patch after install? It looks like we might ignore those changes today until the KubeVirt version is updated, then the patches would kick in?

To make everything react fast and easy to grasp I am still for doing the patches always when we load the install strategy and always just dump the plain shipped install strategy. That makes is absolutely clear and understandable what will happen: If patches are provided in the CR they are applied, if they are not there they are not. Of course that can be done by dumping the proper install strategy over and over, but it is mentally much more difficult to grasp and no questions like this will arise: If I roll back to the previous version which had patches and I don't provide the patches, will it still have them? You simply always get what we release + your patches on the CR. No exception.

rmohr · 2020-07-31T14:49:17Z

One additional note, after we merge the subresource PR, the generation metadata field will only be bumped on the kubevirt cr, if the spec changes. Like on e.g. Deployments I would suggest to add the generation as a metadata annotation to all our artifacts. This will make it pretty easy to detect changes then.

kubevirt-commenter-bot · 2020-09-04T17:07:41Z