Dependent resources standalone mode #914

csviri · 2022-02-07T16:11:55Z

Idea for standalon mode, it's a skeleton only, base for discussions.

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

metacosm · 2022-02-08T17:23:45Z

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

+  @Override
+  public void reconcile(P primary, Context context) {
+    var actual = getResource(primary);
+    var desired = desired(primary, context);


We should avoid creating the desired version if it's not needed because it could be costly.

As far I can see it is always used on both branches.

In this implementation, yes, that wasn't the case before :)

Would still like to see this addressed if possible… In a subsequent PR, though.

We can take a look together, not sure how you mean it.

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

.../io/javaoperatorsdk/operator/processing/dependent/KubernetesDependentResourceController.java

metacosm · 2022-02-08T17:38:23Z

.../io/javaoperatorsdk/operator/processing/dependent/KubernetesDependentResourceController.java

  @Override
-  public void createOrReplace(R dependentResource, Context context) {
-    client.resource(dependentResource).createOrReplace();
+  public Optional<EventSource> eventSource(EventSourceContext<P> context) {


Why are we returning an optional? In what situation(s) would one not want to create an event source?

For example, when the controller is just periodically checking some external resource(s). I think we should have this possibility to have it without event source, although most of the cases it will have.

But you would still need an event source in that case, though… so I guess I don't understand how that's supposed to work.

No, in that case, just the client would directly go to target API on reconcile.

Still not sure how that would work…

MySQL by the way work snow almost like this, the resource is not read from the informer but directly from server.

So basically imagine that there will be just an operator that reconciles every 5 mins, all the resources. All external, so no caches locally. This can pretty much work without an event source.

Yes but that's not the most common case and in the mean time, the most common cases is polluted by the Optional handling… Let's see if we can address this in a subsequent PR.

scrocquesel · 2022-02-08T22:59:48Z

A common use case is to provide an external identifier of the target, being a name/namespace for k8s. How will it behave if the spec change the name. Will it undo desired on the old one ans apply desired to new one ?

csviri · 2022-02-09T08:12:57Z

@scrocquesel

A common use case is to provide an external identifier of the target, being a name/namespace for k8s. How will it behave if the spec change the name. Will it undo desired on the old one ans apply desired to new one ?

The default KubernetesDependentResource is currently just applies if not changed. But of the aspects is that this whole logic can be overriden. So you can extend KubernetesDependentResource and override the reconcile method. Or just implement reconciliation from scratch.

Could you maybe create a separate issue for your case, I'm not sure if understand, and if such naming is that common. Cannot be solved with labels more elegantly?

metacosm · 2022-02-09T08:49:34Z

A common use case is to provide an external identifier of the target, being a name/namespace for k8s. How will it behave if the spec change the name. Will it undo desired on the old one ans apply desired to new one ?

I'm not sure I understand the use case here… Could you please detail it some more?

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java

...o/javaoperatorsdk/operator/sample/standalonedependent/StandaloneDependentTestReconciler.java

...ors/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/MySQLSchemaReconciler.java

scrocquesel · 2022-02-10T19:20:25Z

A common use case is to provide an external identifier of the target, being a name/namespace for k8s. How will it behave if the spec change the name. Will it undo desired on the old one ans apply desired to new one ?

I'm not sure I understand the use case here… Could you please detail it some more?

@csviri @metacosm You can look at https://github.com/redhat-developer/service-binding-operator and find some example here https://redhat-developer.github.io/service-binding-operator/userguide/creating-service-bindings/creating-service-binding.html

apiVersion: binding.operators.coreos.com/v1alpha1
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    version: v1
    group: apps
    kind: Deployment
    name: online-banking
  services: 
  - version: v1alpha1 
    group: example.com 
    kind: AccountService 
    name: prod-account-service

Basically, you're telling the operator to update the Deployment named online-banking. But, the user may change the application.name value (and even application.kind). In such a case, I expect the operator to undo (like if the CR was deleted), and then apply to the new Deployment.

This use case happens a lot with operator that patch other resources. You must tell the target to patch.

csviri · 2022-02-10T19:25:52Z

A common use case is to provide an external identifier of the target, being a name/namespace for k8s. How will it behave if the spec change the name. Will it undo desired on the old one ans apply desired to new one ?

I'm not sure I understand the use case here… Could you please detail it some more?

@csviri @metacosm You can look at https://github.com/redhat-developer/service-binding-operator and find some example here https://redhat-developer.github.io/service-binding-operator/userguide/creating-service-bindings/creating-service-binding.html
apiVersion: binding.operators.coreos.com/v1alpha1
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    version: v1
    group: apps
    kind: Deployment
    name: online-banking
  services: 
  - version: v1alpha1 
    group: example.com 
    kind: AccountService 
    name: prod-account-service
Basically, you're telling the operator to update the Deployment named online-banking. But, the user may change the application.name value (and even application.kind). In such a case, I expect the operator to undo (like if the CR was deleted), and then apply to the new Deployment.

This use case happens a lot with operator that patch other resources. You must tell the target to patch.

In this case the old target is referenced in the status? Or we are just aware of the new spec?

metacosm · 2022-02-10T20:03:37Z

Basically, you're telling the operator to update the Deployment named online-banking. But, the user may change the application.name value (and even application.kind). In such a case, I expect the operator to undo (like if the CR was deleted), and then apply to the new Deployment.

This use case happens a lot with operator that patch other resources. You must tell the target to patch.

I would argue that this use case is not covered by the dependent resource support as it is as the indirection level makes it operator-specific to reason about what needs to be done in that case. I'm not sure how the Service Binding operator does when you modify the parameters of the initial binding… Note that having easier dependent resource support doesn't mean that you cannot write operators without it anymore! 😉

metacosm

I've made some changes and there are some issues that need to be resolved but let's get this merged and address them in subsequent PRs. I've added more entries to #923 to reflect this fact.

csviri · 2022-02-10T21:15:54Z

Basically, you're telling the operator to update the Deployment named online-banking. But, the user may change the application.name value (and even application.kind). In such a case, I expect the operator to undo (like if the CR was deleted), and then apply to the new Deployment.
This use case happens a lot with operator that patch other resources. You must tell the target to patch.

I would argue that this use case is not covered by the dependent resource support as it is as the indirection level makes it operator-specific to reason about what needs to be done in that case. I'm not sure how the Service Binding operator does when you modify the parameters of the initial binding… Note that having easier dependent resource support doesn't mean that you cannot write operators without it anymore! wink

This could be covered by a custom DependentResource implementation, where the reconcile() implements this custom logic. I don't see issues with that. If this would be requested from lot's of user we can also proveide an implementation out of the box.

scrocquesel

Sorry to be late to review, I had a pretty overload week at work and not much spare time.

scrocquesel · 2022-02-10T19:53:13Z

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

+
+  @Override
+  public void reconcile(P primary, Context context) {
+    var actual = getResource(primary);


with Reconciler<T>, I guess the practice is to get the actual frrom context::getSecondaryResource. Shouldn't getResource have access to context ?
I like the way the caller of prepareEventSource actually take ownership and we don't need to keep a reference to it. It make no doubt on who should control its lifecycle (start/stop).

Actually, it's other way around, to for now the event sources are caching the resource ( this is an interesting thing we might want to re-evaluate this later, to separate these two ). But basically at the end the dependent resource should manage it itself, either having an event source (or directly calling the target API). So IMHO it should not have the context in getResource() basically forcing this pattern. (See KubernetesDependentResource)

scrocquesel · 2022-02-10T19:53:32Z

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

+
+  @Override
+  public void reconcile(P primary, Context context) {
+    var actual = getResource(primary);


I would suggest to rename getResource to be more after the version of the resource it should get like actual/desired variant.

On level above on DependentResource we don't have desired, so it's really about reading the resource and the desired here is just a way to generically implement it. Hmm will think about it, so yes, it's always actual, not sure if needs to be that explicit.

@metacosm what do you think?

actual could make sense if we had desired at the same level, indeed. Maybe something that implies that the method is responsible for retrieving the resource more strongly? The previous implementation used getFor for this purpose but it's not really better. How about fetchFrom(primary) which would convey that the responsibility is to fetch the secondary based on the information provided by the primary?

scrocquesel · 2022-02-10T21:23:16Z

...javaoperatorsdk/operator/api/reconciler/dependent/StandaloneKubernetesDependentResource.java

+
+  @Override
+  protected R desired(P primary, Context context) {
+    if (desiredSupplier != null) {


throw in ctor ?

Good point, removed!

scrocquesel · 2022-02-10T21:58:45Z

...n/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResource.java

+    this.client = client;
+  }
+
+  protected void postProcessDesired(R desired, P primary) {


this doesn't mean anything to me about when it will be called. what about beforeCreateOrReplace.
Should it be two different methods ? addOwnerReference works because it is idemPotent.
Is it really needed in update case ? Spec is already the desired state, so, IMHO, it would mean that we want to update metadata but I don't see a reason for doing this on update.

It's really about filling some common data for the desired object created. But yes that maybe is better will change it. thx!

It's needed for now, there is a plan to (discuss first) to preserve labels and annotations and mainly finalizers (will actually do soon) because otherwise the update would remove this data from the resource.

Eventually this might improve with the followup compare issue. Ideally we would do server apply just fabric8 client does not support it yet.

Yes, I'm not too happy with that part either as this feels kind of awkward since to me this should be handled by the controller rather (i.e. this shouldn't be something that implementers of dependent resources should worry about). The controller should take these automated steps based on the dependent configuration (in this case, whether the dependent is supposed to be owned or not) and deal with owner references automatically.

scrocquesel · 2022-02-10T22:23:48Z

...n/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResource.java

+  }
+
+  protected PrimaryResourcesRetriever<R> getDefaultPrimaryResourcesRetriever() {
+    return Mappers.fromOwnerReference();


What happens if owned is false ? If true, is there any other recommended way to get the primary ? I think that ownership shouldn't be exposed as owned and explicitDelete. Either :

the k8s dependent resource is owned and the primary R retriever shall be fromOwnerReference and delete is implicit (k8s garbage collector). Explicit/Implicit is left to the cluster admin with the Foreground/Background cascading deletion https://kubernetes.io/docs/concepts/architecture/garbage-collection/#cascading-deletion.

or it is not owned, and a primary R retriever must be provided and delete is de facto explicit (foreground) but can be skip to be like --cascade=orphan. IMHO, this is not a best practice.

As there is two distinct behaviors, it can be exposed through two different classes ? It will be less prone to misconfiguration. But I see there is already a discussion around hierarchy vs configuration.

Not sure if this two behaviors should be two classes, the current aim but we will refine this (just FYI, this is the first step), see issues (also pls comment in case ) for MVP: #923

Owner reference works just in same namespace, if some dependents are managed outside of namespace those needs to be explicitly created. Maybe it would make sense to subclass it yes. Definetely will make the config more explicit, this is quite vague this way, I agree.

scrocquesel · 2022-02-10T23:09:32Z

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

+    } else {
+      if (!match(actual.get(), desired, context)) {
+        update(actual.get(), desired, primary, context);
+      }


if match doesn't assert that it has been reconciled. The secondary R may exist prior to the primary one. We should be able to put in the context what has been observed to be desired, so that the reconciler can update the status if it is empty.

In the case of mysql schema operator, if the database exists when the primary is created on the cluster, the status is never updated. But, it tricky here because the user is not symetric due to password.

if we split, schema and user in two different dependent. Thus, User::match cannot be sure actual = desired because the password may not be the desired one. So if the user exist prior to the CR, either it should fail the reconcile or the password should be changed and secret updated/recreated.

if match doesn't assert that it has been reconciled. The secondary R may exist prior to the primary one. We should be able to put in the context what has been observed to be desired, so that the reconciler can update the status if it is empty.

Not sure I'm following this. We read the R, and compare it to a target state, this is how P gets reconciled (at least this is a part of it).

In MySQL there is no update now available for the password. But the whole mysql we will revisit shortly.

scrocquesel · 2022-02-10T23:28:59Z

...ain/java/io/javaoperatorsdk/operator/api/reconciler/dependent/AbstractDependentResource.java

+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+
+public abstract class AbstractDependentResource<R, P extends HasMetadata>


If it stays as a Factory (i.e. not exist = create) it should be renamed. And sharing version could be added (i.e. not exist = update status with missing target condition). The sharing version is another beast because, delete is about undoing what reconcile does not deleting. Again thinks of https://github.com/redhat-developer/service-binding-operator. When the CR is removed, the deployment is updated to suppress any env var that has been injected.
I don't if those two behaviors can be merge in one class

…operator-sdk/java-operator-sdk into dependent-resources-standalon-mode

sonarqubecloud · 2022-02-11T09:02:38Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
1 Security Hotspot
33 Code Smells

25.4% Coverage
0.7% Duplication

feat: refactor to support standalon mode

5628db5

csviri requested a review from metacosm February 7, 2022 16:11

csviri self-assigned this Feb 8, 2022

csviri added 4 commits February 8, 2022 10:06

fix: wip

0d2363c

fix: wip

6dff17b

fix: 1 IT

ba619e5

fix: format

dfa2388

metacosm requested changes Feb 8, 2022

View reviewed changes

fix: IT wip

1ad4a12

csviri added 12 commits February 9, 2022 13:02

fix: wip

4b089c1

fix: IT test standalone resource

b3edc69

fix: refactor mysql sample

7b73814

fix: wip

76a6dc6

Merge branch 'next' into dependent-resources-standalon-mode

04f76f9

fix: merged next

2d3289b

Merge branch 'next' into dependent-resources-standalon-mode

07b4834

fix: tomcat e2e tests

850a2ff

fix: format

68a2619

fix: wip

73c76dc

fix: secret handling

64b7148

fix: formaty

a3c7119

csviri marked this pull request as ready for review February 10, 2022 10:06

csviri linked an issue Feb 10, 2022 that may be closed by this pull request

Dependent Resource Standalone Building Blocks #887

Closed

csviri requested a review from metacosm February 10, 2022 10:22

metacosm reviewed Feb 10, 2022

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java Outdated Show resolved Hide resolved

metacosm requested changes Feb 10, 2022

View reviewed changes

fix: web page migrated to dependent resources

c22c295

csviri and others added 5 commits February 10, 2022 16:33

fix: comment to revisit informer setting

3251067

fix: addressed issues from PR

a3fb40f

fix: comments from PR

7843bbc

refactor: remove unneeded code

d851321

refactor: minor

816757c

refactor: better type safety, avoid duplicating configuration

5e25afa

metacosm approved these changes Feb 10, 2022

View reviewed changes

scrocquesel reviewed Feb 10, 2022

View reviewed changes

csviri added 3 commits February 11, 2022 08:24

fix: null check

da62a12

Merge branch 'dependent-resources-standalon-mode' of github.com:java-…

1dc2d88

…operator-sdk/java-operator-sdk into dependent-resources-standalon-mode

fix: naming

7840238

csviri merged commit 6a05ebc into next Feb 11, 2022

csviri deleted the dependent-resources-standalon-mode branch February 11, 2022 09:01

Dependent resources standalone mode #914

Dependent resources standalone mode #914

Uh oh!

Conversation

csviri commented Feb 7, 2022

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!

Uh oh!

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!

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!

scrocquesel commented Feb 8, 2022 via email

Uh oh!

csviri commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacosm commented Feb 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scrocquesel commented Feb 10, 2022

Uh oh!

csviri commented Feb 10, 2022

Uh oh!

metacosm commented Feb 10, 2022

Uh oh!

metacosm left a comment • 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!

csviri commented Feb 10, 2022

Uh oh!

scrocquesel 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

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

csviri commented Feb 9, 2022 •

edited

Loading

metacosm left a comment •

edited

Loading

csviri Feb 11, 2022 •

edited

Loading

metacosm Feb 11, 2022 •

edited

Loading