internal/olm/operator/internal: operator-registry wrappers by estroz · Pull Request #2313 · operator-framework/operator-sdk

estroz · 2019-12-10T19:36:34Z

Description of the change: Methods on a RegistryResources allow creation and deletion operations as well as staleness detection on an operator-registry registry-server.

See the proposal doc for details on how the registry is managed.

Motivation for the change: Broken out of #1912.

/ping @ecordell @njhale @kevinrizza

… registry

camilamacedo86 · 2019-12-12T01:00:03Z

+
+package olm
+
+import (


Why the path is internal/olm/operator/internal again?
Could not it be just internal/olm/registry since what all it is doing is creating the Registry?
Also, it if is a public API then, I understand that, should be in the pkg instead of internals.

It could be internal/olm/registry, but this package is only intended to be used by code in internal/olm/operator, hence its current path. If that changes in the future we can move it. I recommend going through #1912 to get a handle on why this code is set up as-is.

OK. But it shows strange internal/.../internal again.
WDYT about internal/olm/operator/registry

It isn't a common use case but it is a completely viable use of internal.

camilamacedo86 · 2019-12-12T01:07:21Z

+		withRegistryGRPCContainer(pkgName),
+		withVolumeConfigMap(volName, cm.GetName()),
+		withContainerVolumeMounts(volName, []string{containerManifestsDir}),
+	)


The above impl is strange because the with would make sense it was a composition as:
newRegistryDeployment().withA()..withB()

However, it is not. The withs func impl here just building and returning the data get the data, so wdyt about something such asbuildRegistryGRPCContainerDeployment or newRegistryGRPCContainerDeployment instead of for example? Also, could not their logic be part of the newRegistryDeployment impl?

What are the scenarios that RegistryDeployment should be created with then or not? I could not find another place that is doing the same, so shows that it should always have the RegistryGRPCContainer, VolumeConfigMap and ContainerVolumeMounts. Am I right?

Going forward we may want to configure the registry Deployment differently. Using functional options allows us to configure the Deployment just by defining a new function to pass as an argument. The same applies to other object constructors.

As for naming see DynamicRESTMapperOption for an example.

I am ok with the usage of the options.
My comment is regarding the nomenclature of the funcs .

IHMO the with would make more sense if it was used as a composition myResourse().withA().withB() or buildSearch().withA().withB() which it is not the case.

The with funcs are creating/buiding new resources which are returned and used as param and can be used in any part of this code. So:

deployment:= withRegistryGRPCContainer(pkgName) (what with does?)

Would not make easier/intuitive the understatement see something as:

deployment:= newRegistryGRPContainer(pkgName) (what the new does?)

I hope that I could clarify my POV. My suggestion here would change the name only. However, it is not a blocker at all. Am ok with if the others are.

deployment:= withRegistryGRPCContainer(pkgName) will return a function with a *v1.Deployment parameter, not a v1.Deployment object. Yes I'd like to hear what others have to say as well.

AH! 💡 I see what you are doing. I too thought the withXXX was strange but you are using it as part of a variable argument list to allow options to be specified when creating the Registry. That's quite clever, I never would've done it that way. I think I would've had a Parameters or options struct that I built up setting the various options and passing that in. But I kind of like the withXXX pattern you have going here.

dep := newRegistryDeployment(pkgName, namespace, withRegistryGRPCContainer(pkgName), withVolumeConfigMap(volName, cm.GetName()), withContainerVolumeMounts(volName, []string{containerManifestsDir}),

Considering the above code from your example, I read this as create me a new RegistryDeployment using a container with GRPC, with a volume configmap with a list of volumemounts.

Again, I never would've approached it that way but it met my time criteria, that is how long does it take me to figure out what this function is doing. I'm okay with the current implementation.

@jmrodri Yeah the use of functional options is a common pattern that we've used before in the SDK, and upstream in the controller-runtime client and logger.
Though method chaining is probably easier to understand at first glance there are benefits to using functional options when returning errors.
Neither are idiomatic in Go but I prefer functional options.

@estroz I do think we can probably clean this up a bit to actually define Options structs and type aliases for the functional option types for both the RegistryConfigMap options and the RegistryDeployment options.
Take a look at how the controller-runtime's logger defines it's functional options.
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/log/zap/zap.go

We can have an Options struct for just the RegistryDeployment options and have the helpers only set the options. e.g(roughly speaking):

type Opts func(*RegistryDeploymentOptions) type RegistryDeploymentOptions struct { PkgName string ... } func withRegistryGRPCContainer(pkgName string) Opts { return func(o *RegistryDeploymentOptions) { o.PkgName = pkgName } }

And then newRegistryDeployment() just applies all the Opts passed in to a default RegistryDeploymentOptions struct and then creates the deployment based on those options. E.g how the logger constructor does it:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/log/zap/zap.go#L195-L209

camilamacedo86 · 2019-12-12T01:14:47Z

+	pkgName := m.Pkg.PackageName
+	cm := newRegistryConfigMap(pkgName, namespace)
+	dep := newRegistryDeployment(pkgName, namespace)
+	service := newRegistryService(pkgName, namespace)


Why is required re-create the full objects to delete them? Why not get the object by the name and namespace and then, delete it? Could not this impl face pitfuls scenarios where the object in the cluster was updated and is not so longer exactly equals the generation? Will not it face an error if the object for some reason was not really created?

Client.Delete() only cares about object metadata (type info, namespace, and name), which an empty object provides. Client.Get()-ing an object first would be an unnecessary network-bound operation because we know all necessary type information before calling Client.Delete().

IHMO: would be better:
// get the resource by name and namespace
// if exist remove and log
// if not exist just log

It depends on how often we need to do this and whether you ever care to keep the old ones around. I'm okay with just nuking all of the manifests and then optimizing later if speed or other factors come into play.

💡 I now see what @camilamacedo86 is saying because the method names say newXXX it looks like you are getting back a FULL XXX object. Looking at the implementation of newXXX you see it creates metadata only since there was no with arguments specified. This one does seem a bit harder to maintain in the long run. It feels too clever :) What @camilamacedo86 stated would definitely be more clear to the maintainer of what is REALLY going on.

I need to think about this one a little more.

I do not see why having an extra Get() step would be beneficial, since all Get() is doing in this context is populating metadata; this is what newXXX() does implicitly, without a request. Perhaps a comment on this bit of could will suffice? Code comments on each newXXX() method explain that only metadata is populated in the returned object.

camilamacedo86 · 2019-12-12T01:18:02Z

+// withTCPPort returns a function that appends a service port to a Service's
+// port list with name and TCP port portNum.
+func withTCPPort(name string, portNum int32) func(*corev1.Service) {
+	return func(service *corev1.Service) {


IHMO withTCPPort is not helpful for we know what it does. WDYT about something as buildServiceWithTCPPort?

The comment and return type makes its intended use obvious 😄.

Sorry, if I could not make clear my POV.

By looking a code as for example: newResource(deployment, service, withTCPPort(pkg))

I am not sure that my understanding will be that the withTCPPort will impl a new Service with a TCP port without the need to go there and check the withTCPPort code implementation.

However, if it was something as newService().withTCPPort() or newResource(deployment, service, newTCPService(pkg)) then, I think that is not required check the details of the impl to know what it does.

My suggestion would be just rename the func to make clear what it does without the need to check its implementation.

See my comment above: #2313 (comment).

The withTCPPort makes sense now that I've understood the paradigm being used here. I'm okay with it.

camilamacedo86 · 2019-12-12T01:25:55Z

+	)
+	if err = m.Client.DoCreate(ctx, cm, dep, service); err != nil {
+		return fmt.Errorf("error creating operator %q registry-server objects: %w", pkgName, err)
+	}


Wdyt about creating one and then check, create other and then check again?
Where/How it will be called/used? Could it be used in a controller/reconcile?
Should we not first check if the resource exists and then, if not create?
Could not it face a scenario where the deployment, for example, was already created?

Check out the error handling logic of DoCreate(). "already exists" errors are logged instead of being returned.

Have many errors logs in the code that are not actually errors make harder troubleshooting.
IHMO: would be better:
// check if the resource exists
// if not
// then, build a new resource
// and then create and log

@camilamacedo86 I think the check if resource exists first will get you a bunch of misses. When you are creating things you are likely to not have them there so the check is 90% of the time going to return does not exist. But if you flip the logic the way @estroz has it, you attempt the create, 90% of the time the create will happen, the other 10% it already exists you didn't really spend too much time. As long as the "already exist" error is handled I'm okay with it. I will say I would normally write it like @camilamacedo86 specified because that's just how I do things. But I'm okay with the approach in this PR.

camilamacedo86 · 2020-01-07T06:13:22Z

+
+// FormatOperatorNameDNS1123 ensures name is DNS1123 label-compliant by
+// replacing all non-compliant UTF-8 characters with "-".
+func FormatOperatorNameDNS1123(name string) string {


Have we not a func in the apimachinery that we could call directly instead of creating this one?
If not, could we add a comment to make clear that it is a copy and paste from there? WDYT?

AFAIK there is no function in apimachinery to format a string as a DNS1123 label. Nothing is copied from apimachinery either.

If there is no library to do this in the SDK or one of our main deps like controller-runtime or kubebuilder, then I'm okay with this function.

jmrodri

The withXXX paradigm is quite clever, not something I would ever have done as I don't tend to think like that. After reading all of the code I understand how it works it might be confusing to others though during maintenance of this code but I say if it becomes a maintenance burden we can refactor it at that point.

jmrodri · 2020-01-07T23:45:47Z

+	pkgName := m.Pkg.PackageName
+	cm := newRegistryConfigMap(pkgName, namespace)
+	dep := newRegistryDeployment(pkgName, namespace)
+	service := newRegistryService(pkgName, namespace)


It depends on how often we need to do this and whether you ever care to keep the old ones around. I'm okay with just nuking all of the manifests and then optimizing later if speed or other factors come into play.

jmrodri · 2020-01-08T00:00:54Z

+		withRegistryGRPCContainer(pkgName),
+		withVolumeConfigMap(volName, cm.GetName()),
+		withContainerVolumeMounts(volName, []string{containerManifestsDir}),
+	)


AH! 💡 I see what you are doing. I too thought the withXXX was strange but you are using it as part of a variable argument list to allow options to be specified when creating the Registry. That's quite clever, I never would've done it that way. I think I would've had a Parameters or options struct that I built up setting the various options and passing that in. But I kind of like the withXXX pattern you have going here.

dep := newRegistryDeployment(pkgName, namespace, withRegistryGRPCContainer(pkgName), withVolumeConfigMap(volName, cm.GetName()), withContainerVolumeMounts(volName, []string{containerManifestsDir}),

Considering the above code from your example, I read this as create me a new RegistryDeployment using a container with GRPC, with a volume configmap with a list of volumemounts.

Again, I never would've approached it that way but it met my time criteria, that is how long does it take me to figure out what this function is doing. I'm okay with the current implementation.

jmrodri · 2020-01-08T00:05:23Z

+	)
+	if err = m.Client.DoCreate(ctx, cm, dep, service); err != nil {
+		return fmt.Errorf("error creating operator %q registry-server objects: %w", pkgName, err)
+	}


@camilamacedo86 I think the check if resource exists first will get you a bunch of misses. When you are creating things you are likely to not have them there so the check is 90% of the time going to return does not exist. But if you flip the logic the way @estroz has it, you attempt the create, 90% of the time the create will happen, the other 10% it already exists you didn't really spend too much time. As long as the "already exist" error is handled I'm okay with it. I will say I would normally write it like @camilamacedo86 specified because that's just how I do things. But I'm okay with the approach in this PR.

jmrodri · 2020-01-08T00:09:59Z

+	pkgName := m.Pkg.PackageName
+	cm := newRegistryConfigMap(pkgName, namespace)
+	dep := newRegistryDeployment(pkgName, namespace)
+	service := newRegistryService(pkgName, namespace)


💡 I now see what @camilamacedo86 is saying because the method names say newXXX it looks like you are getting back a FULL XXX object. Looking at the implementation of newXXX you see it creates metadata only since there was no with arguments specified. This one does seem a bit harder to maintain in the long run. It feels too clever :) What @camilamacedo86 stated would definitely be more clear to the maintainer of what is REALLY going on.

I need to think about this one a little more.

jmrodri · 2020-01-08T00:11:38Z

+// withTCPPort returns a function that appends a service port to a Service's
+// port list with name and TCP port portNum.
+func withTCPPort(name string, portNum int32) func(*corev1.Service) {
+	return func(service *corev1.Service) {


The withTCPPort makes sense now that I've understood the paradigm being used here. I'm okay with it.

jmrodri · 2020-01-08T00:12:49Z

+
+// FormatOperatorNameDNS1123 ensures name is DNS1123 label-compliant by
+// replacing all non-compliant UTF-8 characters with "-".
+func FormatOperatorNameDNS1123(name string) string {


If there is no library to do this in the SDK or one of our main deps like controller-runtime or kubebuilder, then I'm okay with this function.

estroz · 2020-01-08T22:19:49Z

@jmrodri @hasbro17 @camilamacedo86 I'm going to follow up this PR with some of the suggested changes. These changes won't affect how this library is used very much or at all.

hasbro17 · 2020-01-08T23:41:59Z

@estroz 👍 We can do a follow up to clean up the formatting of the functional options.

internal/olm/operator/internal: tools to create and delete a manifest…

28fa846

… registry

estroz added the olm-integration Issue relates to the OLM integration label Dec 10, 2019

estroz requested review from asmacdo, camilamacedo86, hasbro17, jmccormick2001, jmrodri and joelanford December 10, 2019 19:36

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2019

estroz changed the title ~~internal/olm/operator/internal: tools to work with manifest registries~~ [WIP] internal/olm/operator/internal: tools to work with manifest registries Dec 10, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2019

use PackageManifest and Bundle types directly

68e322d

estroz changed the title ~~[WIP] internal/olm/operator/internal: tools to work with manifest registries~~ internal/olm/operator/internal: tools to work with manifest registries Dec 11, 2019

Eric Stroczynski added 3 commits December 11, 2019 08:51

test/test-framework: revendor

f049b3d

use getObjectFileName in getPackageFileName

bc08d4e

use fmt.Errorf over errors.Wrap

2d03ce3

estroz force-pushed the feature/olm-poc-registry branch from b6d4312 to 2d03ce3 Compare December 11, 2019 17:15

camilamacedo86 reviewed Dec 11, 2019

View reviewed changes

Comment thread internal/olm/operator/internal/configmap.go

estroz mentioned this pull request Dec 11, 2019

internal/olm/operator: OLM operator manager #2320

Merged

estroz added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 11, 2019

camilamacedo86 reviewed Dec 12, 2019

View reviewed changes

Comment thread internal/olm/operator/internal/registry.go

camilamacedo86 reviewed Dec 12, 2019

View reviewed changes

Comment thread internal/olm/operator/internal/registry.go

estroz mentioned this pull request Dec 12, 2019

cmd/operator-sdk/alpha: 'up/down olm' commands to run an operator #1912

Closed

5 tasks

Merge branch 'master' into feature/olm-poc-registry

c4a2df7

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019

estroz requested a review from camilamacedo86 December 12, 2019 18:48

Merge branch 'master' into feature/olm-poc-registry

64efcf9

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2020

camilamacedo86 reviewed Jan 7, 2020

View reviewed changes

Merge branch 'master' into feature/olm-poc-registry

8a282ae

jmrodri approved these changes Jan 8, 2020

View reviewed changes

remove go.sum duplicate entry

8601769

estroz changed the title ~~internal/olm/operator/internal: tools to work with manifest registries~~ internal/olm/operator/internal: operator-registry wrappers Jan 8, 2020

estroz merged commit 93f23d7 into operator-framework:master Jan 8, 2020

estroz deleted the feature/olm-poc-registry branch January 8, 2020 22:22

Conversation

estroz commented Dec 10, 2019

Uh oh!

Uh oh!

camilamacedo86 Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

camilamacedo86 Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

hasbro17 Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

camilamacedo86 Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jmrodri left a comment

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

camilamacedo86 Dec 12, 2019 •

edited

Loading

camilamacedo86 Dec 12, 2019 •

edited

Loading

camilamacedo86 Dec 12, 2019 •

edited

Loading

hasbro17 Jan 8, 2020 •

edited

Loading

camilamacedo86 Dec 12, 2019 •

edited

Loading