Skip to content

Update resource cache after update/create from Dependent Resource #953

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.

Sign up for GitHub

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

Closed
wants to merge 13 commits into from
Closed

Update resource cache after update/create from Dependent Resource #953

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1cb3457
feat: up to date resource from get resource
csviri Feb 16, 2022
5ecfe4b
feature: populate change to informer after latest update
csviri Feb 16, 2022
cdd91b3
fix: sample change
csviri Feb 17, 2022
b501d19
fix: integration tests
csviri Feb 17, 2022
f53b46e
fix: temporal cache for reading resources after update
csviri Feb 17, 2022
58d52cd
docs: temporal cache
csviri Feb 17, 2022
71e54df
fix: remove not used method
csviri Feb 17, 2022
d0033fa
fix: unit test for temp cache
csviri Feb 17, 2022
40e4063
fix: unit test wip
csviri Feb 17, 2022
38fbed8
fix: unit test dependent resource
csviri Feb 18, 2022
bf71615
fix: format
csviri Feb 18, 2022
2dbdbf3
Merge branch 'next' into dr-up-to-date-get-resource
csviri Feb 18, 2022
9d31856
Merge branch 'next' into dr-up-to-date-get-resource
csviri Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,10 @@ protected R create(R target, P primary, Context context) {
"{}, with id: {}", target.getClass(), ResourceID.fromResource(target));
beforeCreateOrUpdate(target, primary);
Class<R> targetClass = (Class<R>) target.getClass();
return client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
var newResource = client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
.create(target);
populateNewResourceToCache(newResource);
return newResource;
}

@SuppressWarnings("unchecked")
Expand All @@ -103,8 +105,15 @@ protected R update(R actual, R target, P primary, Context context) {
ResourceID.fromResource(target));
beforeCreateOrUpdate(target, primary);
Class<R> targetClass = (Class<R>) target.getClass();
return client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
.replace(target);
R updatedResource =
client.resources(targetClass).inNamespace(target.getMetadata().getNamespace())
.replace(target);
populateNewResourceToCache(updatedResource);
return updatedResource;
}

private void populateNewResourceToCache(R updatedResource) {
informerEventSource.populateCacheUpdatedResource(updatedResource);
}

@Override
Expand All @@ -119,12 +128,6 @@ public Optional<EventSource> eventSource(EventSourceContext<P> context) {
return Optional.of(informerEventSource);
}

public KubernetesDependentResource<R, P> setInformerEventSource(
InformerEventSource<R, P> informerEventSource) {
this.informerEventSource = informerEventSource;
return this;
}

@Override
public void delete(P primary, Context context) {
if (!addOwnerReference) {
Expand Down
5 changes: 4 additions & 1 deletion ...in/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ public T remove(ResourceID key) {
@Override
public void put(ResourceID key, T resource) {
getSource(key.getNamespace().orElse(ANY_NAMESPACE_MAP_KEY))
.ifPresent(c -> c.put(key, resource));
.ifPresentOrElse(c -> c.put(key, resource),
() -> log.warn(
"Cannot put resource in the cache. No related cache found: {}. Resource: {}",
key, resource));
}
}
5 changes: 5 additions & 0 deletions ...javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import io.fabric8.kubernetes.client.dsl.Resource;
import io.fabric8.kubernetes.client.informers.ResourceEventHandler;
import io.javaoperatorsdk.operator.api.config.ResourceConfiguration;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
import io.javaoperatorsdk.operator.processing.event.source.CachingEventSource;
import io.javaoperatorsdk.operator.processing.event.source.UpdatableCache;

Expand Down Expand Up @@ -39,4 +40,8 @@ public void stop() {
super.stop();
manager().stop();
}

public void populateCacheUpdatedResource(R resource) {
cache.put(ResourceID.fromResource(resource), resource);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on fabric8io/kubernetes-client#3654 (comment), I'm not sure we should be modifying the informer's cache :(
Another thing is that this won't be possible in 6.0 anymore so we need to think about an alternative, in particular in the light of: fabric8io/kubernetes-client#3616. So we might have to think about re-adding our own caching layer sooner than later…

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the plan is to do it with the separate cache if the informers will be modified for that. Not sure if we can get away with this until that time. It's not 100% correct, but the chance to have there a race condition is very close to zero

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative is to wait for the event that the cache received the resource with the resource version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean not the update/delete would propagate the cache but the getResource() will be kinda "synchronous"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solved it in a different way, pls see updated PR. (Will add unit tests soon)

}
}
9 changes: 0 additions & 9 deletions ...operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package io.javaoperatorsdk.operator.sample;

import java.time.Duration;
import java.util.*;

import org.apache.commons.lang3.StringUtils;
Expand All @@ -19,7 +18,6 @@

import static io.javaoperatorsdk.operator.ReconcilerUtils.loadYaml;
import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_FINALIZER;
import static org.awaitility.Awaitility.await;

@ControllerConfiguration(finalizerName = NO_FINALIZER)
public class WebPageReconciler
Expand Down Expand Up @@ -59,7 +57,6 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context context) {

WebPageStatus status = new WebPageStatus();

waitUntilConfigMapAvailable(webPage);
status.setHtmlConfigMap(configMapDR.getResource(webPage).orElseThrow().getMetadata().getName());
status.setAreWeGood("Yes!");
status.setErrorMessage(null);
Expand All @@ -68,12 +65,6 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context context) {
return UpdateControl.updateStatus(webPage);
}

// todo after implemented we can remove this method:
// https://github.com/java-operator-sdk/java-operator-sdk/issues/924
private void waitUntilConfigMapAvailable(WebPage webPage) {
await().atMost(Duration.ofSeconds(5)).until(() -> configMapDR.getResource(webPage).isPresent());
}

@Override
public Optional<WebPage> updateErrorStatus(
WebPage resource, RetryInfo retryInfo, RuntimeException e) {
Expand Down