Skip to content
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

fix: dependent event handling with temp cache not cleared in some corner cases #1994

Merged
merged 6 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ protected R handleUpdate(R actual, R desired, P primary, Context<P> context) {

@SuppressWarnings("unused")
public R create(R target, P primary, Context<P> context) {
if (useSSA(context)) {
// setting resource version for SSA so only created if it doesn't exist already
target.getMetadata().setResourceVersion("1");
}
final var resource = prepare(target, primary, "Creating");
return useSSA(context)
? resource
Expand All @@ -138,15 +142,23 @@ public R create(R target, P primary, Context<P> context) {
}

public R update(R actual, R target, P primary, Context<P> context) {
if (log.isDebugEnabled()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why check if debug login is enabled here and not elsewhere?

Copy link
Collaborator Author

@csviri csviri Aug 7, 2023

Choose a reason for hiding this comment

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

Since creating a new object in that case: ResourceID.fromResource(actual). Want to do that only if really needed

log.debug("Updating actual resource: {} version: {}", ResourceID.fromResource(actual),
actual.getMetadata().getResourceVersion());
}
R updatedResource;
if (useSSA(context)) {
target.getMetadata().setResourceVersion(actual.getMetadata().getResourceVersion());
return prepare(target, primary, "Updating")
updatedResource = prepare(target, primary, "Updating")
.fieldManager(context.getControllerConfiguration().fieldManager())
.forceConflicts().serverSideApply();
} else {
var updatedActual = updaterMatcher.updateResource(actual, target, context);
return prepare(updatedActual, primary, "Updating").replace();
updatedResource = prepare(updatedActual, primary, "Updating").replace();
}
log.debug("Resource version after update: {}",
updatedResource.getMetadata().getResourceVersion());
return updatedResource;
}

public Result<R> match(R actualResource, P primary, Context<P> context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
* </p>
* <br>
* <p>
* 2. Additional API is provided that is ment to be used with the combination of the previous one,
* 2. Additional API is provided that is meant to be used with the combination of the previous one,
* and the goal is to filter out events that are the results of updates and creates made by the
* controller itself. For example if in reconciler a ConfigMaps is created, there should be an
* Informer in place to handle change events of that ConfigMap, but since it has bean created (or
Expand Down Expand Up @@ -113,9 +113,9 @@ public InformerEventSource(InformerConfiguration<R> configuration, KubernetesCli
@Override
public void onAdd(R newResource) {
if (log.isDebugEnabled()) {
log.debug("On add event received for resource id: {} type: {}",
log.debug("On add event received for resource id: {} type: {} version: {}",
ResourceID.fromResource(newResource),
resourceType().getSimpleName());
resourceType().getSimpleName(), newResource.getMetadata().getResourceVersion());
}
primaryToSecondaryIndex.onAddOrUpdate(newResource);
onAddOrUpdate(Operation.ADD, newResource, null,
Expand All @@ -125,9 +125,12 @@ public void onAdd(R newResource) {
@Override
public void onUpdate(R oldObject, R newObject) {
if (log.isDebugEnabled()) {
log.debug("On update event received for resource id: {} type: {}",
log.debug(
"On update event received for resource id: {} type: {} version: {} old version: {} ",
ResourceID.fromResource(newObject),
resourceType().getSimpleName());
resourceType().getSimpleName(),
newObject.getMetadata().getResourceVersion(),
oldObject.getMetadata().getResourceVersion());
}
primaryToSecondaryIndex.onAddOrUpdate(newObject);
onAddOrUpdate(Operation.UPDATE, newObject, oldObject,
Expand Down Expand Up @@ -282,17 +285,26 @@ private void handleRecentResourceOperationAndStopEventRecording(Operation operat
log.debug(
"Did not found event in buffer with target version and resource id: {}", resourceID);
temporaryResourceCache.unconditionallyCacheResource(newResource);
} else if (eventRecorder.containsEventWithVersionButItsNotLastOne(
resourceID, newResource.getMetadata().getResourceVersion())) {
R lastEvent = eventRecorder.getLastEvent(resourceID);
log.debug(
"Found events in event buffer but the target event is not last for id: {}. Propagating event.",
resourceID);
if (eventAcceptedByFilter(operation, newResource, oldResource)) {
propagateEvent(lastEvent);
} else {
// if the resource is not added to the temp cache, it is cleared, since
// the cache is cleared by subsequent events after updates, but if those did not receive
// the temp cache is still filled at this point with an old resource
log.debug("Cleaning temporary cache for resource id: {}", resourceID);
temporaryResourceCache.removeResourceFromCache(newResource);
if (eventRecorder.containsEventWithVersionButItsNotLastOne(
resourceID, newResource.getMetadata().getResourceVersion())) {
R lastEvent = eventRecorder.getLastEvent(resourceID);

log.debug(
"Found events in event buffer but the target event is not last for id: {}. Propagating event.",
resourceID);
if (eventAcceptedByFilter(operation, newResource, oldResource)) {
propagateEvent(lastEvent);
}
}
}
} finally {
log.debug("Stopping event recording for: {}", resourceID);
eventRecorder.stopEventRecording(resourceID);
}
}
Expand Down
Loading