Dependent resources implementation #785

metacosm · 2022-01-03T20:18:31Z

Fixes #771

csviri · 2022-01-13T13:21:57Z

One thing that popped up is that even for kubernetes resources we will need ordering, or the "depends on" notion. Like to create resource when other is already up and running.

metacosm · 2022-01-13T13:39:40Z

This implicitly managed at the moment by the declaring order of the dependents but we should probably make it explicit, indeed. That said, one step at a time. Let's get initial support in and then see what else is needed.

csviri · 2022-01-13T14:23:49Z

This implicitly managed at the moment by the declaring order of the dependents but we should probably make it explicit, indeed. That said, one step at a time. Let's get initial support in and then see what else is needed.

yep, lets do this in interations. But this "depends-on" notions, makes a good selling point, without it it's not more powerful than a helm chart.

metacosm · 2022-01-13T14:52:03Z

This implicitly managed at the moment by the declaring order of the dependents but we should probably make it explicit, indeed. That said, one step at a time. Let's get initial support in and then see what else is needed.

yep, lets do this in interations. But this "depends-on" notions, makes a good selling point, without it it's not more powerful than a helm chart.

I beg to differ. You cannot reconcile dependent resources with a helm chart. It's a lot more complex than just installing dependent resources, it's also about managing them as they evolve at runtime.

csviri · 2022-01-17T19:47:22Z

I beg to differ. You cannot reconcile dependent resources with a helm chart. It's a lot more complex than just installing dependent resources, it's also about managing them as they evolve at runtime.

What I mean is, that helm charts handles re-apply / changes nicely. It handles when you just apply a list or resources. What does not handle is the situation when you want to "create a resource after an other resource is in a certain state". Basically this is an analogy to depends_on from terraform.

sebastienblanc · 2022-01-19T08:01:53Z

...ators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/TomcatReconciler.java

+          tomcat.getMetadata().getName(),
+          tomcat.getMetadata().getNamespace(),
+          tomcat.getStatus().getReadyReplicas());
+      return UpdateControl.updateStatus(updatedTomcat);


Is the update method from the DependentResource called at this point ? like https://github.com/java-operator-sdk/java-operator-sdk/pull/785/files#diff-06a1ae70b5e55bf828c2120ae4525d9ad22cb82b526c6db644472ad46698f6f9R47
And how does it work when we have more than one secondary resource (like having 3 deployments) ?

At the moment the dependent resources are reconciled before we reconcile the primary one so any dependent needed to be created is created before the primary reconciler is called, similarly, any updates to secondary resources will be processed before the primary resource is reconciled.

We don't currently handle the case where you have several dependents of the same type very well. The previous version of the implementation allowed you to add a qualifier to your dependent definition specifically for this case. We have a name on EventSource for that purpose but it should indeed be exposed to the annotation.

...amework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java

...r-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DependentResource.java

csviri · 2022-01-20T08:42:52Z

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

+
+import java.util.Optional;
+
+public interface AttributeHolder {


I see that this is used in the context only. WHat is the purpose here? Maybe describe it also in javadoc.

This is also used by the EventSourceContext

csviri · 2022-01-20T08:48:39Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceContextInjector.java

@@ -0,0 +1,5 @@
+package io.javaoperatorsdk.operator.api.reconciler;
+
+public interface EventSourceContextInjector {


There should be probably a generics on the EvenSourceContext not sure if also on the interface itself too.

Not sure I understand here…

IntelliJ complains that EventSourceContext is without a generics

csviri · 2022-01-20T09:07:28Z

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

+  }
+
+  @SuppressWarnings("unchecked")
+  protected Persister<R, P> initPersister(DependentResource<R, P> delegate) {


Not sure about this part, if it needs to implement persister why it can be configured separatelly. Also why is not directly on DependentResource

I'm not sure either anymore, tbh… 😅

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java

.../src/main/java/io/javaoperatorsdk/operator/processing/event/source/DependentEventSource.java

csviri · 2022-01-20T10:00:55Z

...java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerConfiguration.java

+    extends DefaultResourceConfiguration<R> {
+
+  private final PrimaryResourcesRetriever<R> secondaryToPrimaryResourcesIdSet;
+  private final AssociatedSecondaryResourceIdentifier<P> associatedWith;


This should not abbe associatedDependentResource?

Not sure what you mean…

It's about naming, but I;m probably wrong. So I guess it's already this way to some extent, but:

The resources that are managed by dependent resources we can call "dependent resources"

And the secondary resource that is the resource managed by te controller by not the dependent resources mecahnism directly?

Or should not be rather managed dependent resource and dependent resources?
So sometimes the naming is confusing for me.

The naming is indeed not great… we should probably settle on using dependent or secondary everywhere.

csviri · 2022-01-20T10:02:22Z

.../main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java

+import io.javaoperatorsdk.operator.processing.event.source.ResourceCache;
+import io.javaoperatorsdk.operator.processing.event.source.UpdatableCache;
+
+public class InformerManager<T extends HasMetadata, C extends ResourceConfiguration<T>>


Missing Unit tests.

csviri · 2022-01-20T10:18:06Z

...java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerConfiguration.java

+  private final AssociatedSecondaryResourceIdentifier<P> associatedWith;
+  private final boolean skipUpdateEventPropagationIfNoChange;
+
+  public InformerConfiguration(ConfigurationService service, String labelSelector,


The configuration service is a little bit weird here a little bit. Especially if somebody wants to create an Informer without Depdendent resources.

csviri · 2022-01-20T10:21:32Z

It's not easy to review this big change. What I would like (maybe just temporarily) is also have there the samples without the dependent resources. Since there are some places, where it's not clear (like the informers) how that would work without dependent resources. So that would help.

Also miss unit tests in some cases.

csviri · 2022-01-20T10:26:52Z

So a goal should be also to preserve the current "low" level API clear and available. Ideally make it clear where is the difference.

csviri · 2022-01-20T10:29:32Z

Also we should add docs soons, this is clear how to use. Also update the internal docs:

https://javaoperatorsdk.io/docs/architecture-and-internals

Since this is quite massive change.

csviri · 2022-01-20T10:30:34Z

Pls add also the description of the scope this PR.

metacosm · 2022-01-20T11:30:37Z

Pls add also the description of the scope this PR.

I need to update it but the scope is defined in #771.

metacosm · 2022-01-20T11:31:51Z

So a goal should be also to preserve the current "low" level API clear and available. Ideally make it clear where is the difference.

As mentioned several times already, this is completely opt-in. If you don't want to use the dependent resources support, you can keep doing what was done before.

metacosm · 2022-01-20T11:34:21Z

It's not easy to review this big change. What I would like (maybe just temporarily) is also have there the samples without the dependent resources. Since there are some places, where it's not clear (like the informers) how that would work without dependent resources. So that would help.

Easy enough: there is no change. You can use the previous version of the examples without any issue because this is completely opt-in.

Also miss unit tests in some cases.

Definitely but that will come after the approach is validated.

csviri · 2022-01-20T11:38:33Z

It's not easy to review this big change. What I would like (maybe just temporarily) is also have there the samples without the dependent resources. Since there are some places, where it's not clear (like the informers) how that would work without dependent resources. So that would help.

Easy enough: there is no change. You can use the previous version of the examples without any issue because this is completely opt-in.

Also miss unit tests in some cases.

Definitely but that will come after the approach is validated.

I commented in one place the InformerEventSource for example seems to have completetly different API

- Kubernetes-native resources are automatically handled without users having to deal with explicit event sources - Reconcilers and event sources can exchange information via the `EventSourceContext` - Declarative management is completely opt-in and can cohabit with "traditional" handling i.e. it's possible to have only part of the dependents being declared via annotations and others explicitly handled (though from a maintenance perspective it's better to stick to one approach or the other)

sonarqubecloud · 2022-01-21T12:55:47Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
1 Security Hotspot
19 Code Smells

28.0% Coverage
0.9% Duplication

metacosm · 2022-01-21T12:56:22Z

This PR is now merged in the next branch where we will work on the next 2.1 release.

metacosm self-assigned this Jan 3, 2022

metacosm requested a review from csviri January 3, 2022 20:18

metacosm marked this pull request as draft January 3, 2022 20:18

metacosm added devex-epic eventsources-epic labels Jan 3, 2022

metacosm force-pushed the dependent-resources-2 branch from 066e0fc to 300dbb6 Compare January 5, 2022 21:24

metacosm mentioned this pull request Jan 5, 2022

Dependent resources initial support #771

Closed

metacosm force-pushed the dependent-resources-2 branch 3 times, most recently from dbab033 to 36df956 Compare January 7, 2022 17:31

metacosm mentioned this pull request Jan 8, 2022

feat: initial support for dependent resources #726

Closed

metacosm changed the title ~~WIP: new dependent resources implementation~~ Dependent resources implementation Jan 8, 2022

metacosm marked this pull request as ready for review January 8, 2022 16:12

metacosm force-pushed the dependent-resources-2 branch 4 times, most recently from d4ca4f4 to 3b7b59b Compare January 13, 2022 10:11

metacosm mentioned this pull request Jan 13, 2022

Feature: tight up the integration with extra eventSources #826

Open

csviri mentioned this pull request Jan 17, 2022

Evaluate an higher level abstraction on top of the Reconcile Loop #843

Closed

metacosm force-pushed the dependent-resources-2 branch from 17b02c5 to 0c09a5d Compare January 18, 2022 18:03

sebastienblanc reviewed Jan 19, 2022

View reviewed changes

vmuzikar mentioned this pull request Jan 19, 2022

Baseline for Keycloak deployment in operator keycloak/keycloak#9628

Merged

csviri reviewed Jan 20, 2022

View reviewed changes

metacosm force-pushed the dependent-resources-2 branch from aecc4c2 to 6e7ed41 Compare January 20, 2022 20:08

metacosm mentioned this pull request Jan 21, 2022

Expose dependent configuration for the case where we have several dependents of the same type #857

Closed

metacosm force-pushed the dependent-resources-2 branch from cadae26 to 7f1cdca Compare January 21, 2022 12:53

metacosm closed this Jan 21, 2022

metacosm deleted the dependent-resources-2 branch January 21, 2022 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependent resources implementation #785

Dependent resources implementation #785

metacosm commented Jan 3, 2022 •

edited

Loading

csviri commented Jan 13, 2022

metacosm commented Jan 13, 2022

csviri commented Jan 13, 2022

metacosm commented Jan 13, 2022

csviri commented Jan 17, 2022

sebastienblanc Jan 19, 2022

metacosm Jan 19, 2022

csviri Jan 20, 2022

metacosm Jan 20, 2022

csviri Jan 20, 2022

metacosm Jan 20, 2022

csviri Jan 20, 2022

csviri Jan 20, 2022

metacosm Jan 20, 2022

csviri Jan 20, 2022

metacosm Jan 20, 2022

csviri Jan 21, 2022 •

edited

Loading

metacosm Jan 21, 2022

csviri Jan 20, 2022

csviri Jan 20, 2022

csviri commented Jan 20, 2022 •

edited

Loading

csviri commented Jan 20, 2022

csviri commented Jan 20, 2022

csviri commented Jan 20, 2022

metacosm commented Jan 20, 2022

metacosm commented Jan 20, 2022

metacosm commented Jan 20, 2022

csviri commented Jan 20, 2022

sonarqubecloud bot commented Jan 21, 2022

metacosm commented Jan 21, 2022 •

edited

Loading


		import java.util.Optional;

		public interface AttributeHolder {

		@@ -0,0 +1,5 @@
		package io.javaoperatorsdk.operator.api.reconciler;

		public interface EventSourceContextInjector {

Dependent resources implementation #785

Dependent resources implementation #785

Conversation

metacosm commented Jan 3, 2022 • edited Loading

csviri commented Jan 13, 2022

metacosm commented Jan 13, 2022

csviri commented Jan 13, 2022

metacosm commented Jan 13, 2022

csviri commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Jan 20, 2022 • edited Loading

csviri commented Jan 20, 2022

csviri commented Jan 20, 2022

csviri commented Jan 20, 2022

metacosm commented Jan 20, 2022

metacosm commented Jan 20, 2022

metacosm commented Jan 20, 2022

csviri commented Jan 20, 2022

sonarqubecloud bot commented Jan 21, 2022

metacosm commented Jan 21, 2022 • edited Loading

metacosm commented Jan 3, 2022 •

edited

Loading

csviri Jan 21, 2022 •

edited

Loading

csviri commented Jan 20, 2022 •

edited

Loading

metacosm commented Jan 21, 2022 •

edited

Loading