BundleDeployment creation with BundleImage #104

dtfranz · 2023-01-27T00:30:54Z

Combines @perdasilva 's latest commits with a selection from @varshaprasad96 and @ankitathomas 's controller_prototype to enable the operator-controller to create bundledeployments using a (currently hard-coded) CatalogSource. Also adds additional Makefile targets to enable simpler installation of dependencies (cert-manager and rukpak).

It should be possible to demonstrate milestone 1 by running the following two commands:

$ make run
$ kubectl apply -f config/samples/operators_v1alpha1_operator.yaml

joelanford · 2023-01-28T05:11:09Z

Makefile

+.PHONY: cert-mgr
+cert-mgr: ## Install cert-manager
+	kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/$(CERT_MGR_VERSION)/cert-manager.yaml
+	kubectl wait --for=condition=Available --namespace=cert-manager deployment/cert-manager-webhook --timeout=60s
+
+.PHONY: rukpak
+rukpak: ## Install rukpak
+	kubectl apply -f https://github.com/operator-framework/rukpak/releases/latest/download/rukpak.yaml
+	kubectl wait --for=condition=Available --namespace=rukpak-system deployment/core --timeout=60s
+


Add a TODO to swap this out with whatever we come up with for the "distribution" or install script or whatever that ends up being?

joelanford · 2023-01-28T05:15:19Z

controllers/operator_controller.go

+			Name: o.GetName(),
+		},
+		Spec: rukpakv1alpha1.BundleDeploymentSpec{
+			//TODO: Don't assume plain provisioner


+1

Long term, I envision a separate configuration API that sets up mappings from bundle properties to provisioner class names.

Mid term, perhaps we get some mileage around a property that sets the provisioner class name directly. We have to be careful with this though because I expect ProvisionerClass could end up being a new API in rukpak, which means provisionerClassName becomes a runtime thing, not something hardcoded in rukpak.

Short term, what you have here will be fine for quite awhile, I imagine.

joelanford · 2023-01-28T05:17:22Z

controllers/operator_controller.go

+				},
+				Spec: rukpakv1alpha1.BundleSpec{
+					Source: rukpakv1alpha1.BundleSource{
+						// TODO: Don't assume image type


IMO, we should make that bundle path property something a bit more definitive (e.g. bundle image reference). At that point, it would be totally safe to assume that we use the rukpak image type if we have that property. And that would then leave the door open for bundles to have different properties that map to a different source type.

Agreed here. I've created another issue here #108 to address this, since I was hoping it wouldn't be a blocker for our shorter term goals.

joelanford · 2023-01-28T05:19:52Z

controllers/operator_controller.go

-	r.resolver = resolution.NewOperatorResolver(mgr.GetClient(), resolution.HardcodedEntitySource)
-
 	err := ctrl.NewControllerManagedBy(mgr).
+		Owns(&rukpakv1alpha1.BundleDeployment{}).


Convention/nit/my opinion: Always declare For as the first watch. Its the primary CR type. Then Owns, then Watches.

Sort of ordering from most coupling (self) to least coupling (things I watch, but don't own).

Yeah, that makes sense to me too. I'll update it.

awgreene · 2023-01-30T20:54:14Z

controllers/operator_controller.go

 	Scheme *runtime.Scheme

-	resolver *resolution.OperatorResolver
+	Resolver *resolution.OperatorResolver


Does this need to be exported? Could we just create a GetEntity(id DeppyID) function for the OperatorReconciler type?

Not sure if this is answering your second question, but for the first one it was done this way to allow the resolver to be set up separately from the controller in main(), which you can see in the prototype here. This seems to make it easier to set up the catalog source cache.

That could be done with a NewOperatorReconciler(c client, s scheme, r resolver) &OperatorReconciler function, right? Point being that once the cache is set we shouldn't allow it to be manipulated directly.

Well, avoid allowing people to manipulate the resolver that is.

Yeah I see your point there, I'll update 👍

tmshort · 2023-01-30T19:48:44Z

Makefile

+.PHONY: cert-mgr
+cert-mgr: ## Install cert-manager
+	kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/$(CERT_MGR_VERSION)/cert-manager.yaml
+	kubectl wait --for=condition=Available --namespace=cert-manager deployment/cert-manager-webhook --timeout=60s


Make the timeouts a variable; "60s" is a reasonable default, but for some others?

Updated, can now be overridden with WAIT_TIMEOUT

tmshort · 2023-01-30T19:51:01Z

config/rbac/role.yaml

 kind: ClusterRole
 metadata:
  creationTimestamp: null
  name: manager-role


Not something you did (probably kubebuilder), but manager-role is just too generic a name for a ClusterRole

Yes, it is kubebuilder - and it also gets prepended with a prefix set in config/defaults/kustomization.yaml: namePrefix: operator-controller-, so it ends up as operator-controller-manager-role. Personally I also don't like this way of doing it much but it's also kind of a pain to go through and replace all the names with different values. Plus some of them don't get prepended and I've never quite figured out the rule for when that happens...

Let's avoid touching it for now, keep in mind that all kubebuilder controllers follow this convention, so there are some assumptions about the name of this role.

tmshort · 2023-01-30T19:52:15Z

controllers/operator_controller.go

 					bundlePath, err := v.BundleEntity().BundlePath()
 					if err != nil {
 						return ctrl.Result{}, err
+					} // Create bundleDeployment


Comment should really be on next line, as it does not apply to the block it follows.

awgreene · 2023-01-31T21:49:25Z

/approve

awgreene · 2023-01-31T22:11:53Z

Oh, can we fix the commit header?

@perdasilva

Combines @perdasilva 's latest commits with a selection from @varshaprasad96 and @ankitathomas 's controller_prototype to enable the operator-controller to create bundledeployments using a (currently hard-coded) CatalogSource. Also adds additional Makefile targets to enable simpler installation of dependencies (cert-manager and rukpak). Signed-off-by: dtfranz <dfranz@redhat.com>

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

dtfranz changed the title ~~BundleDeployment creation with BundleImage~~ Draft: BundleDeployment creation with BundleImage Jan 27, 2023

dtfranz marked this pull request as draft January 27, 2023 00:49

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 27, 2023

joelanford reviewed Jan 28, 2023

View reviewed changes

awgreene reviewed Jan 30, 2023

View reviewed changes

tmshort reviewed Jan 31, 2023

View reviewed changes

dtfranz force-pushed the bundle-url branch from d67e703 to 5a4fc78 Compare January 31, 2023 17:34

dtfranz marked this pull request as ready for review January 31, 2023 17:34

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2023

dtfranz changed the title ~~Draft: BundleDeployment creation with BundleImage~~ BundleDeployment creation with BundleImage Jan 31, 2023

dtfranz force-pushed the bundle-url branch from 5a4fc78 to 008f6a0 Compare January 31, 2023 20:03

dtfranz force-pushed the bundle-url branch from 008f6a0 to aea93f8 Compare January 31, 2023 21:55

awgreene added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2023

awgreene approved these changes Jan 31, 2023

View reviewed changes

dtfranz force-pushed the bundle-url branch from aea93f8 to d665309 Compare January 31, 2023 22:21

awgreene merged commit 13ea200 into operator-framework:main Jan 31, 2023

This was referenced Feb 6, 2023

Collect content for each entity ID provided in the solution set #75

Closed

The operator controller ensures that the correct bundleDeployments exist for each entity in the solution set #77

Closed

LalatenduMohanty pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Dec 19, 2024

Adds codecov integration (operator-framework#104)

b996f69

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

Uh oh!

BundleDeployment creation with BundleImage #104

BundleDeployment creation with BundleImage #104

Uh oh!

Conversation

dtfranz commented Jan 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awgreene commented Jan 31, 2023

Uh oh!

awgreene commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants