-
Notifications
You must be signed in to change notification settings - Fork 218
improve: dependent resource can be re-initialized #2026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d44a53c
improve: dependent resource can be re-initialized
csviri 6fe42ae
test for reproduce
csviri 0312a9b
Merge branch 'main' into dr-re-initialization
csviri 72bb259
fix?
csviri 6a85a83
imrpove
csviri 10b6420
remove hashmap init capacity
csviri File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
...ator-framework/src/test/java/io/javaoperatorsdk/operator/DependentReInitializationIT.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package io.javaoperatorsdk.operator; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import io.fabric8.kubernetes.client.KubernetesClientBuilder; | ||
import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; | ||
import io.javaoperatorsdk.operator.sample.dependentreinitialization.ConfigMapDependentResource; | ||
import io.javaoperatorsdk.operator.sample.dependentreinitialization.DependentReInitializationCustomResource; | ||
import io.javaoperatorsdk.operator.sample.dependentreinitialization.DependentReInitializationReconciler; | ||
|
||
class DependentReInitializationIT { | ||
|
||
/** | ||
* In case dependent resource is managed by CDI (like in Quarkus) can be handy that the instance | ||
* is reused in tests. | ||
*/ | ||
@Test | ||
void dependentCanDeReInitialized() { | ||
var client = new KubernetesClientBuilder().build(); | ||
LocallyRunOperatorExtension.applyCrd(DependentReInitializationCustomResource.class, client); | ||
|
||
var dependent = new ConfigMapDependentResource(); | ||
|
||
startEndStopOperator(client, dependent); | ||
startEndStopOperator(client, dependent); | ||
} | ||
|
||
private static void startEndStopOperator(KubernetesClient client, | ||
ConfigMapDependentResource dependent) { | ||
Operator o1 = new Operator(o -> o | ||
.withCloseClientOnStop(false) | ||
.withKubernetesClient(client)); | ||
o1.register(new DependentReInitializationReconciler(dependent, client)); | ||
o1.start(); | ||
o1.stop(); | ||
} | ||
|
||
} |
29 changes: 29 additions & 0 deletions
29
...javaoperatorsdk/operator/sample/dependentreinitialization/ConfigMapDependentResource.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package io.javaoperatorsdk.operator.sample.dependentreinitialization; | ||
|
||
import java.util.Map; | ||
|
||
import io.fabric8.kubernetes.api.model.ConfigMap; | ||
import io.fabric8.kubernetes.api.model.ConfigMapBuilder; | ||
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; | ||
import io.javaoperatorsdk.operator.api.reconciler.Context; | ||
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; | ||
|
||
public class ConfigMapDependentResource | ||
extends CRUDKubernetesDependentResource<ConfigMap, DependentReInitializationCustomResource> { | ||
|
||
public ConfigMapDependentResource() { | ||
super(ConfigMap.class); | ||
} | ||
|
||
@Override | ||
protected ConfigMap desired(DependentReInitializationCustomResource primary, | ||
Context<DependentReInitializationCustomResource> context) { | ||
return new ConfigMapBuilder() | ||
.withMetadata(new ObjectMetaBuilder() | ||
.withName(primary.getMetadata().getName()) | ||
.withNamespace(primary.getMetadata().getNamespace()) | ||
.build()) | ||
.withData(Map.of("key", "val")) | ||
.build(); | ||
} | ||
} |
14 changes: 14 additions & 0 deletions
14
...dk/operator/sample/dependentreinitialization/DependentReInitializationCustomResource.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package io.javaoperatorsdk.operator.sample.dependentreinitialization; | ||
|
||
import io.fabric8.kubernetes.api.model.Namespaced; | ||
import io.fabric8.kubernetes.client.CustomResource; | ||
import io.fabric8.kubernetes.model.annotation.Group; | ||
import io.fabric8.kubernetes.model.annotation.Version; | ||
|
||
@Group("sample.javaoperatorsdk") | ||
@Version("v1") | ||
public class DependentReInitializationCustomResource | ||
extends CustomResource<Void, Void> | ||
implements Namespaced { | ||
|
||
} |
38 changes: 38 additions & 0 deletions
38
...torsdk/operator/sample/dependentreinitialization/DependentReInitializationReconciler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package io.javaoperatorsdk.operator.sample.dependentreinitialization; | ||
|
||
import java.util.Map; | ||
|
||
import io.fabric8.kubernetes.client.KubernetesClient; | ||
import io.javaoperatorsdk.operator.api.reconciler.*; | ||
import io.javaoperatorsdk.operator.processing.event.source.EventSource; | ||
|
||
@ControllerConfiguration | ||
public class DependentReInitializationReconciler | ||
implements Reconciler<DependentReInitializationCustomResource>, | ||
EventSourceInitializer<DependentReInitializationCustomResource> { | ||
|
||
private final ConfigMapDependentResource configMapDependentResource; | ||
|
||
public DependentReInitializationReconciler(ConfigMapDependentResource dependentResource, | ||
KubernetesClient client) { | ||
this.configMapDependentResource = dependentResource; | ||
this.configMapDependentResource.setKubernetesClient(client); | ||
} | ||
|
||
@Override | ||
public UpdateControl<DependentReInitializationCustomResource> reconcile( | ||
DependentReInitializationCustomResource resource, | ||
Context<DependentReInitializationCustomResource> context) throws Exception { | ||
configMapDependentResource.reconcile(resource, context); | ||
return UpdateControl.noUpdate(); | ||
} | ||
|
||
@Override | ||
public Map<String, EventSource> prepareEventSources( | ||
EventSourceContext<DependentReInitializationCustomResource> context) { | ||
return EventSourceInitializer.nameEventSourcesFromDependentResource(context, | ||
configMapDependentResource); | ||
} | ||
|
||
|
||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that initializing the buffer to a zero size is a good idea since it will immediately need to get resized the first time something is added to it. Can't we initialize it to some default, reasonable size if we want to avoid overprovisioning for nothing? I'm not sure what's the typical use pattern here, like how many indexers might we expect to have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mostly shouldn't worry about initializing to specific sizes. The value is used lazily - so using zero or one is effectively the same. If not specified it will default to 16, but you're only holding that memory ephemerally based upon the usage pattern here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess 0 leads to the internal table being initialized to size 1… which is reasonable if we expect a low number of indexer. No idea on the usage pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually there is index just for one case (at least this is what we see now) when there is many-to-one / many-to-many relationship between resource, see: https://javaoperatorsdk.io/docs/features#managing-relation-between-primary-and-secondary-resources. But yeah, maybe not necessary to optimizations like this, can be even confusing to read. will remove this number if not objections and have the default (as when initialized).