OLM integration

[slintes]

What this PR does / why we need it:
This PR adds needed manifests for integration with the Operator Lifecycle Management (OLM).

Special notes for your reviewer:

• For generating the new manifests I codified most parts of of the operator deployment and RBAC rules, which makes them easier to be reused in the CSV and prerequisites manifest.
• Since the CSV file does not support role references for defining needed permissions, the operator uses more fine grained rules now instead of just using the cluster-admin account. These rules are combined from what the operator needs itself, and what it needs indirectly for installing the (Cluster)Roles of the virt components. The latter rules are created by reusing and just concatenating the rules of the existing codified (Cluster)Roles.

See /docs/devel/olm-integration.md !

Release note:

Added manifests needed for integration of the KubeVirt operator with the Operator Lifecycle Manager and Operator Marketplace

[davidvossel]

\cc @rthallisey @djzager @lveyde

There are some parallel efforts going on here with kubevirt olm integration.

From what I understand, the kubevirt-olm project is investigating a unified olm CSV file that contains all the operators (kubevirt, cdi, etc...) that encompass openshift CNV. (which means it might make sense to call it the cnv-olm project instead)

It's unclear to me exactly how all this will shake out at the moment. What I don't want to see is the community trying to maintain two separate KubeVirt olm CSV files. We need one source of truth here for what it means to integrate KubeVirt with olm.

Since the KubeVirt CSV file is tightly coupled to the virt-operator, I think it makes sense for both of these pieces to live within the same repository (like @slintes is doing in this PR). From there I'd expect something like a CNV CSV file to either depend on the KubeVirt/CDI operator CRs, or somehow merge the KubeVirt/CDI maintained CSV files into a unified CNV CSV file.

The main point is, we want the KubeVirt CSV file bits consumed from and maintained within the KubeVirt repo.

Trying to talk about this problem set sounds like gibberish. Hopefully what I'm saying makes sense to everyone.

[davidvossel]

wow, lol. that's an interesting way for olm to retrieve the icon. We'll probably want to generate this base64 from the actual icon binary stored in the source tree.

[slintes]

good idea ;)

[davidvossel]

i'm unsure if cnv belongs in here or not.

[slintes]

Since the KubeVirt CSV file is tightly coupled to the virt-operator, I think it makes sense for both of these pieces to live within the same repository (like @slintes is doing in this PR). From there I'd expect something like a CNV CSV file to either depend on the KubeVirt/CDI operator CRs, or somehow merge the KubeVirt/CDI maintained CSV files into a unified CNV CSV file.

The main point is, we want the KubeVirt CSV file bits consumed from and maintained within the KubeVirt repo.

Exactly, and that is IIUC what @fabiand had in mind, when he asked me to go on with this topic.
I would be happy to collaborate with the operator team, to make this effort useful for them and their unified CSV and / or operator of operators.

[fabiand]

It's unclear to me exactly how all this will shake out at the moment. What I don't want to see is the community trying to maintain two separate KubeVirt olm CSV files. We need one source of truth here for what it means to integrate KubeVirt with olm.

Heya. There is some duplication atm, yes. But the duplication is not the goal :) It's a side effect of the reserach going on.
To me the goals of @slintes and @rthallisey and @lveyde are distinct:

@slintes is working on enabling to auto-generate all necessary OLM metadata for KubeVirt releases only in order to provide automated updates of KubeVirt only through OLM. This is a KubeVirt focused goal. KubeVirt is used standalone, and we want it to be easily consumable via OLM. This also implies that this metadta and update flows will be tested in our CI.

@rthallisey is working on enabling the orchestrated deployment of KubeVirt and other components (liek CDI, multus, etc). This is a slightly different problem, as it's orchestrating deployment of multiple components with OLM. And IIUCI then it's currently not clear how this orchestration can happen (there are options, but it's not yet decided). However @rthallisey and @lveyde work goes beyond kubevirt.

To me, ideally, @rthallisey and @lveyde can consume the work that @slintes is doing in one way or the other.
After all it's just a separation of concerns.

[rthallisey]

I think the goals here have been captured well. We want to communicate how all the components are glued together in OLM. What we need is for components to have OLM manifests written and tested so we can consume them from a combined CNV perspective.

Breaking this down into items:
Kubevirt

• CSV file to launch kubevirt-operator

CNV

• Combination of component csv files

[fabiand]

CNV

* Combination of component csv files

Not sure what CNV is in this context, but I agree about the kubevir part.

[djzager]

Not sure what CNV is in this context, but I agree about the kubevir part.

Interestingly enough, as I am becoming more familiar with kubevirt, I have been looking more into what @rthallisey is saying here:

CNV

• Combination of component csv files

My idea is that a combined kubevirt CSV would really be a dummy package that would specify version 1.0.0 of the combined kubevirt requires kubevirts.kubevirt.io/v1alpha3, ..., cdis.cdi.kubevirt.io/v1alpha1. When you make a subscription to kubevirt.v1.0.0 you get the specified versions of the individual components. An example would look something like:

apiVersion: operators.coreos.com/v1alpha1
kind: ClusterServiceVersion
metadata:
  name: kubevirt.v1.0.0
  annotations:
    tectonic-visibility: ocs
spec:
...
  installModes:
  - type: OwnNamespace
    supported: true
  - type: SingleNamespace
    supported: true
  - type: MultiNamespace
    supported: false
  - type: AllNamespaces
    supported: false
  install:
    strategy: ???
    ....maybe my dummy deployment if I use the deployment strategy...
  customresourcedefinitions:
    required:
    - name: kubevirts.kubevirt.io
      version: v1alpha3
      kind: KubeVirt
      displayName: KubeVirt
      description: KubeVirt
    - name: cdis.cdi.kubevirt.io
      version: v1alpha1
      kind: CDI
      displayName: CDI Application
      description: Represents an instance of a CDI application

[djzager]

The main point is, we want the KubeVirt CSV file bits consumed from and maintained within the KubeVirt repo.

I forgot to reply to this when I got in this morning. I wholeheartedly agree with this. If it can be done though...I would prefer to not consume any bits and just mark the CRD that you own as required in the composite kubevirt CSV.

[davidvossel]

I would prefer to not consume any bits and just mark the CRD that you own as required in the composite kubevirt CSV

yep, i'd prefer this, but I'm not sure how this would work without confusing people.

What would the olm CSV you all provide be called if KubeVirt is a dependency?

[djzager]

What would the olm CSV you all provide be called if KubeVirt is a dependency?

that thought experiment I had blew up after a conversation with @rthallisey for exactly the reason that you mention. I ended up with something that looked like:

Whereas I think what we would want is to have a single package "KubeVirt" that gets you all of the components required.

Edit: Adding a screenshot of what I think we want

[slintes]

Almost there...

@fabiand I need a few things:

• a username and password for kubevirt on Quay.io (and I need to see how to use that in Travis)
• push access to docker.io/kubevirt/builder (my docker hub account is slintes)
• and we need to consider which ones of the new manifests to release on github
• and when to push new CSVs to Quay
  • only on releases?
  • or also on every PR merge from master? we could use maybe a alpha channel for this, and beta/whatever for releases

To anyone who wants to have a look at this: you need to run make builder-build before doing anything else, until I pushed a new builder image. And I think /docs/devel/olm-integration.md is helpful :)

[alexxa]

@slintes Many compliments on Readme! So well written: very clear, detailed, all acronyms are defined (!!!) And the same for the PR description!

[alexxa]

s/based in information/based on the information

[alexxa]

declares which namespaces OLM should operate on

[alexxa]

I'm not sure, but I believe it's 'the operator'

[alexxa]

Our?

[alexxa]

s/if want/

• if you want
• if there is a need to test
• if one wants

[alexxa]

so, where can it be created?

[alexxa]

"deploy to" - is a remote operation
"deploy on" - a local one
which one is the case here?

[slintes]

well, no clue... We don't know if it's a remote or local cluster.
But thanks for your review! I think I resolved all other comments.

[alexxa]

Check >**Note:** Text here syntax

[alexxa]

Change plain urls to hyperlinks, please, everywhere.

[slintes]

done

[fabiand]

• a username and password for kubevirt on Quay.io (and I need to see how to use that in Travis)

I'll catch you offline for this one.

• push access to docker.io/kubevirt/builder (my docker hub account is slintes)

@rmohr can you sort this out with @slintes (or Marc, reach out to Roman off-list)

• and we need to consider which ones of the new manifests to release on github

Can you explain this a little more

• and when to push new CSVs to Quay
  • only on releases?

For now yes.
(Once our CI is great (TM) then maybe for each merged PR into an alpha channel? :))

• or also on every PR merge from master? we could use maybe a alpha channel for this, and beta/whatever for releases

Alpha at best. And IMO only once we have confidence that our CI is catching the worst things.
@davidvossel was actually recently discussing that we need CI for merged master and stbale branch PRs.

Thus: Once we have CI running on merged PRs on master and stable branches, maybe that's a good time to start playing with alpha release channel?
Or do you feel comfortable of enabling it now? And once we have the beefed up CI, then make them beta channels?

[slintes]

• push access to docker.io/kubevirt/builder (my docker hub account is slintes)

@rmohr can you sort this out with @slintes (or Marc, reach out to Roman off-list)

already sorted out with Artyom, new builder image is pushed

• and we need to consider which ones of the new manifests to release on github

Can you explain this a little more

we have several new manifests, some are pushed to Quay as "bundle". But I guess we also want to add some them to the release artifacts here on github?
We have (first 3 are in the bundle):

• the CSV (I would say, yes, add to artifacts)
• the KubeVirt CRD (not sure, probably not, it's part of the already existing operator manifest)
• the Package description (no)
• the preconditions manifest (no, this is mainly useful only for testing the CSV without the catalog/Marketplace operators)
• the OperatorSource (yes)
• the OperatorGroup (not sure, need to find out if this can be created in the UI)
• the Subscription (no, created in the UI)

• and when to push new CSVs to Quay

  • only on releases?

For now yes.
(Once our CI is great (TM) then maybe for each merged PR into an alpha channel? :))

• or also on every PR merge from master? we could use maybe a alpha channel for this, and beta/whatever for releases

Alpha at best. And IMO only once we have confidence that our CI is catching the worst things.
@davidvossel was actually recently discussing that we need CI for merged master and stbale branch PRs.

Thus: Once we have CI running on merged PRs on master and stable branches, maybe that's a good time to start playing with alpha release channel?
Or do you feel comfortable of enabling it now? And once we have the beefed up CI, then make them beta channels?

fair point about the CI on merged PRs, let's wait for it.
So only releases for now, going into a beta channel? So that we use alpha later on for master?

[fabiand]

KubeVirt Project

[djzager]

A couple of nits.

[djzager]

I'm not sure how important this is, but I believe that olm should be openshift-operator-lifecycle-manager

[djzager]

(and so run make manifests) -> and also run..

[slintes]

I consider this not WIP anymore, ready for review

[slintes]

well, no clue... We don't know if it's a remote or local cluster.
But thanks for your review! I think I resolved all other comments.

[slintes]

done

[djzager]

I'm curious, if you don't specify:

spec:
    targetNamespaces:
        - {{.Namespace}}

Does this make it so the KubeVirt operator has to be watching all namespaces?

[slintes]

yes, at the moment the operator watches all namespaces anyway, it does not evaluate the injected target namespaces yet.

[slintes]

ci test please

[slintes]

ci test please

[slintes]

while last week updating a CSV triggered a redeployment of at least cluster rules, today I see that the virt-operator deployment is not updated when changing the verbosity level. Looking at events and OLM code it seems that OLM misses the replaces field in the CSV manifest. So I need to fill that somehow with the last published CSV version of that same channel...

/hold

[slintes]

meh...and when you set the replaces field, the bundle uploaded to Quay also needs to contains that old CSV...

[djzager]

Not sure what impact this would have but I noticed that package refers to a beta channel but this says the CSV is alpha

[slintes]

not sure if those fields are related, will check, thx!

[djzager]

Thinking more about this over the past week or so. Not exposing the underlying CRDs that the virt-operator creates prevents other operators from relying on these APIs. Is that use-case something kubevirt wishes to support? if yes and now is not the time, is there an issue to track that case?

[slintes]

Is there a reason / usecase why other operators would depend on the underlying CRDs, and not the KubeVirt application CRD?

[djzager]

Maybe I have an operator that creates virtual machines. That seems reasonable.

[slintes]

need to test if Travis integration works as expected, but otherwise this is ready for another round of review
/hold cancel

[davidvossel]

👍 looks excellent! Great work @slintes

is there anything else we need to wait on before merging this?

[davidvossel]

imageTag and imageRegistry exist now as well

[slintes]

good catch, fixed

[slintes]

Thanks for reviewing @davidvossel !
The only thing I'd like to add is testing this stuff. But installing OLM and Marketplace manually doesn't make sense (I think the Openshift 3.11 OLM is quite old, not sure if Marketplace can be enabled), so I'd like to see if that is possible as soon as we have a OpenShift 4.x test lane.

[davidvossel]

The only thing I'd like to add is testing this stuff. But installing OLM and Marketplace manually doesn't make sense

yeah, we aren't there yet with a openshift 4.x lane. I think there's value for this to go in now so projects like the 'hyperconverged-cluster-operator' can at least begin consuming the initial work here.

Once a stable 4.x provider lands, we should investigate functional tests.

[stu-gott]

This is really excellent work @slintes! I've made a moderate pass over the PR, but owe you a deeper look. However, I'd defer to @davidvossel if he thinks this is ready.

[slintes]

I think there's value for this to go in now so projects like the 'hyperconverged-cluster-operator' can at least begin consuming the initial work here

yes, I didn't mean that I want to wait for 4.x. And this stuff is already used by the hyperconverged cluster operator from my fork, so I absolutely also want to get this. So I'm going to merge this.

Thx all for reviews!