Controller name as finalizer by default #223

s-soroosh · 2020-11-16T12:32:36Z

closes #57
closes #218

…er name.

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java

s-soroosh · 2020-11-16T12:39:10Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java

@@ -28,4 +28,10 @@
     */
    UpdateControl<R> createOrUpdateResource(R resource, Context<R> context);

+    // What about we let developer to set the finalizer Name through overriding
+    // this method rather than setting it as attribute in annotation?
+    default String getDefaultFinalizerName() {


Several places in the code finalizer references are named defaultFinalzer. Any objection to renaming them all to finalizer? I suppose they are indeed referring to the finalizer and not the default one.
e.g.
Several spots in this file:
https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java#L24

I agree.. sounds like we're calling everything defaultFinalizer even though we just mean the finalizer. This is actually confusing as there a thing called the default finalizer name. @csviri can you confirm we're seeing this right?

I renamed the default finalizer to finalizer or similar everywhere in docs, logs and code for now.
LMK if you have any objection/thoughts about it.

@psycho-ir looks good, yep it jus tfinalizer in most of the cases not default finalizer.
I don't think we should use default method in the interface however. Would stick to the annotation to override the finalizer name.

Right, I have removed this method already.

s-soroosh · 2020-11-16T12:43:40Z

Some of Integration tests are failing due to the long finalized name. i.e ->

name part must be no more than 63 characters.

We can either truncate the package name or add validation on the finalizerName length and ask the developer to override the name a pick a shorter value.

adam-sandor · 2020-11-17T13:06:55Z

How about not putting the package name in the finalizer at all?

s-soroosh · 2020-11-17T13:19:58Z

How about not putting the package name in the finalizer at all?

It may result in conflict. What I also found risky about using the class name is that refactoring the code may cause unintended different finalizer name.

What about using the crdName which is less likely to be changed and if it changes then I think it's OK to have another finalizer name by default. i.e crdName.finalizer

adam-sandor · 2020-11-17T13:26:07Z

Good thinking, CRD name is better. If that changes than you really need to think about migration strategy anyway. Do we need to add .finalizer to the end?

s-soroosh · 2020-11-17T13:50:08Z

Good thinking, CRD name is better. If that changes than you really need to think about migration strategy anyway. Do we need to add .finalizer to the end?

I had a look into a few finalizers and didn't any de facto in naming conventions.
e.g. service.kubernetes.io/load-balancer-cleanup or even foregroundDeletion.
These options looks good to m e:

crdName
crdName/finalizer
finalizer.crdName

s-soroosh · 2020-11-17T15:52:10Z

Good thinking, CRD name is better. If that changes than you really need to think about migration strategy anyway. Do we need to add .finalizer to the end?

I had a look into a few finalizers and didn't any de facto in naming conventions.
e.g. service.kubernetes.io/load-balancer-cleanup or even foregroundDeletion.
These options looks good to m e:

crdName

crdName/finalizer

finalizer.crdName

I picked crdName option, easy to change though in case you prefer any other convention.

adam-sandor · 2020-11-18T10:44:48Z

I'll give this a test today and merge if it's alright

metacosm · 2020-11-18T15:55:54Z

Good thinking, CRD name is better. If that changes than you really need to think about migration strategy anyway. Do we need to add .finalizer to the end?

I had a look into a few finalizers and didn't any de facto in naming conventions.
e.g. service.kubernetes.io/load-balancer-cleanup or even foregroundDeletion.
These options looks good to m e:

crdName

crdName/finalizer

finalizer.crdName

I picked crdName option, easy to change though in case you prefer any other convention.

This is actually incorrect according to the spec: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers where a custom finalizer is supposed to be <domain name>/<finalizer name>…

Note also that #229 uses the addFinalizer method added on HasMetadata in 5.0 which validates that part (and is why the tests are currently failing on that PR).

s-soroosh · 2020-11-18T16:10:08Z

Good thinking, CRD name is better. If that changes than you really need to think about migration strategy anyway. Do we need to add .finalizer to the end?

I had a look into a few finalizers and didn't any de facto in naming conventions.
e.g. service.kubernetes.io/load-balancer-cleanup or even foregroundDeletion.
These options looks good to m e:

crdName

crdName/finalizer

finalizer.crdName

I picked crdName option, easy to change though in case you prefer any other convention.

This is actually incorrect according to the spec: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers where a custom finalizer is supposed to be <domain name>/<finalizer name>…

Note also that #229 uses the addFinalizer method added on HasMetadata in 5.0 which validates that part (and is why the tests are currently failing on that PR).

Good catch! so I'll go with crdName/finalizer option as the default name.

s-soroosh · 2020-11-18T20:44:13Z

Added the /finalizer suffix to be compliant with the specification.

kirek007 · 2020-11-19T10:07:54Z

LGTM!

metacosm · 2020-11-21T19:49:34Z

@psycho-ir do you recall which test failed if the name was too long? The problem with using the CRD name is that, while the group should usually be a valid domain name, it doesn't appear to be mandatory. This is causing issues still with the k8s client 5.0 upgrade. I've opened an issue for this: #233

Soroosh Sarabadani added 6 commits November 15, 2020 22:52

remove default finalizer name

6361e50

remove default finalizer name

efeb4b2

Add getDefaultFinalizerName method

b160d45

Update getFinalizer logic

59345b6

Unit test customer finalizer name

e3f343b

Use getDefaultFinalizerName in the code to get the controller finaliz…

40f19f5

…er name.

s-soroosh commented Nov 16, 2020

View reviewed changes

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java Outdated Show resolved Hide resolved

s-soroosh commented Nov 16, 2020

View reviewed changes

Soroosh Sarabadani added 8 commits November 17, 2020 15:03

Remove getDefaultFinalizerName

a8d8e6c

Use crdName as finalizerName

50808bf

Update finalizerName javadoc

9755f8d

rename hasDefaultFinalizer to hasGivenFinalizer

25755c5

rename defaultFinalizer to finalizer

f188c1f

rename defaultFinalizer to finalizer in logs and docs

85c09bc

fix tests

46af13e

Fix integration test

19e7117

Soroosh Sarabadani added 2 commits November 17, 2020 17:10

increase namespace deletion timeout

cf719fd

update minikube version

a893994

Soroosh Sarabadani added 3 commits November 18, 2020 21:24

Add /finalizer suffix

7259cb7

Add /finalizer to the tests

2b5ef83

extract FINALIZER_NAME

fe4ef59

s-soroosh closed this Nov 18, 2020

s-soroosh reopened this Nov 18, 2020

kirek007 merged commit bc65b1c into operator-framework:master Nov 19, 2020

Controller name as finalizer by default #223

Controller name as finalizer by default #223

Uh oh!

Conversation

s-soroosh commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

s-soroosh Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

adam-sandor Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

s-soroosh Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

csviri Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

s-soroosh Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

s-soroosh commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adam-sandor commented Nov 17, 2020

Uh oh!

s-soroosh commented Nov 17, 2020

Uh oh!

adam-sandor commented Nov 17, 2020

Uh oh!

s-soroosh commented Nov 17, 2020

Uh oh!

s-soroosh commented Nov 17, 2020

Uh oh!

adam-sandor commented Nov 18, 2020

Uh oh!

metacosm commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-soroosh commented Nov 18, 2020

Uh oh!

s-soroosh commented Nov 18, 2020

Uh oh!

kirek007 commented Nov 19, 2020

Uh oh!

metacosm commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

s-soroosh commented Nov 16, 2020 •

edited

Loading

s-soroosh commented Nov 16, 2020 •

edited

Loading

metacosm commented Nov 18, 2020 •

edited

Loading

metacosm commented Nov 21, 2020 •

edited

Loading