-
Notifications
You must be signed in to change notification settings - Fork 220
eventing refinements mentioned on #2012 #2034
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -364,4 +364,31 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() { | |||||
return Set.of(ConfigMap.class, Secret.class); | ||||||
} | ||||||
|
||||||
/** | ||||||
* If an annotation should be used so that the operator sdk can detect events from its own updates | ||||||
* of dependent resources and then filter them. | ||||||
* <p> | ||||||
* Disable this if you want to react to your own dependent resource updates | ||||||
* | ||||||
* @since 4.5.0 | ||||||
*/ | ||||||
default boolean previousAnnotationForDependentResources() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather see a more explicit method name that conveys better the intent, i.e. something like:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there have been multiple mechanism for this the thinking is to use specific rather than general terminology, so that if it changes again the user can be explicit about which mechanism(s) are in play. If you strongly prefer the general feature name, that's fine too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be too generic. |
||||||
return true; | ||||||
} | ||||||
|
||||||
/** | ||||||
* If the event logic should parse the resourceVersion to determine the ordering of events. This | ||||||
* is typically not needed. | ||||||
* <p> | ||||||
* Disabled by default as Kubernetes does not support, and discourages, this interpretation of | ||||||
* resourceVersions. Enable only if your api server event processing seems to lag the operator | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a more concrete example of when setting this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's basically for feature parity with the mutable cache that is in go client - I'm not aware of user request for the feature, so if you feel strongly about it, then it could be hidden or removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
* logic and you want to further minimize the the amount of work done / updates issued by the | ||||||
* operator. | ||||||
* | ||||||
* @since 4.5.0 | ||||||
*/ | ||||||
default boolean parseResourceVersions() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, a more explicit method name would be better, imo.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's decide if you even want this before making a name change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general I'm not against this. Also would stick with the original name, thus There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
return false; | ||||||
} | ||||||
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,8 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata> | |
extends ManagedInformerEventSource<R, P, InformerConfiguration<R>> | ||
implements ResourceEventHandler<R> { | ||
|
||
private static final int MAX_RESOURCE_VERSIONS = 256; | ||
|
||
public static String PREVIOUS_ANNOTATION_KEY = "javaoperatorsdk.io/previous"; | ||
|
||
private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class); | ||
|
@@ -78,14 +80,31 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata> | |
private final PrimaryToSecondaryMapper<P> primaryToSecondaryMapper; | ||
private Map<String, Function<R, List<String>>> indexerBuffer = new HashMap<>(); | ||
private final String id = UUID.randomUUID().toString(); | ||
private final Set<String> knownResourceVersions; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document what are the situations this is solving |
||
|
||
public InformerEventSource( | ||
InformerConfiguration<R> configuration, EventSourceContext<P> context) { | ||
this(configuration, context.getClient()); | ||
this(configuration, context.getClient(), | ||
context.getControllerConfiguration().getConfigurationService().parseResourceVersions()); | ||
} | ||
|
||
public InformerEventSource(InformerConfiguration<R> configuration, KubernetesClient client) { | ||
super(client.resources(configuration.getResourceClass()), configuration); | ||
this(configuration, client, false); | ||
} | ||
|
||
public InformerEventSource(InformerConfiguration<R> configuration, KubernetesClient client, | ||
boolean parseResourceVersions) { | ||
super(client.resources(configuration.getResourceClass()), configuration, parseResourceVersions); | ||
if (parseResourceVersions) { | ||
knownResourceVersions = Collections.newSetFromMap(new LinkedHashMap<String, Boolean>() { | ||
@Override | ||
protected boolean removeEldestEntry(java.util.Map.Entry<String, Boolean> eldest) { | ||
return size() >= MAX_RESOURCE_VERSIONS; | ||
} | ||
}); | ||
} else { | ||
knownResourceVersions = null; | ||
} | ||
|
||
// If there is a primary to secondary mapper there is no need for primary to secondary index. | ||
primaryToSecondaryMapper = configuration.getPrimaryToSecondaryMapper(); | ||
|
@@ -169,6 +188,10 @@ private synchronized void onAddOrUpdate(Operation operation, R newObject, R oldO | |
} | ||
|
||
private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) { | ||
if (knownResourceVersions != null | ||
&& knownResourceVersions.contains(newObject.getMetadata().getResourceVersion())) { | ||
return true; | ||
} | ||
var res = temporaryResourceCache.getResourceFromCache(resourceID); | ||
if (res.isEmpty()) { | ||
return isEventKnownFromAnnotation(newObject, oldObject); | ||
|
@@ -262,6 +285,9 @@ public synchronized void handleRecentResourceCreate(ResourceID resourceID, R res | |
|
||
private void handleRecentCreateOrUpdate(Operation operation, R newResource, R oldResource) { | ||
primaryToSecondaryIndex.onAddOrUpdate(newResource); | ||
if (knownResourceVersions != null) { | ||
knownResourceVersions.add(newResource.getMetadata().getResourceVersion()); | ||
} | ||
temporaryResourceCache.putResource(newResource, Optional.ofNullable(oldResource) | ||
.map(r -> r.getMetadata().getResourceVersion()).orElse(null)); | ||
} | ||
|
@@ -275,7 +301,6 @@ public boolean allowsNamespaceChanges() { | |
return configuration().followControllerNamespaceChanges(); | ||
} | ||
|
||
|
||
private boolean eventAcceptedByFilter(Operation operation, R newObject, R oldObject) { | ||
if (genericFilter != null && !genericFilter.accept(newObject)) { | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import org.slf4j.LoggerFactory; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; | ||
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; | ||
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; | ||
import io.javaoperatorsdk.operator.processing.event.ResourceID; | ||
|
||
|
@@ -36,13 +37,18 @@ public class TemporaryResourceCache<T extends HasMetadata> { | |
|
||
private final Map<ResourceID, T> cache = new ConcurrentHashMap<>(); | ||
private final ManagedInformerEventSource<T, ?, ?> managedInformerEventSource; | ||
private final boolean parseResourceVersions; | ||
|
||
public TemporaryResourceCache(ManagedInformerEventSource<T, ?, ?> managedInformerEventSource) { | ||
public TemporaryResourceCache(ManagedInformerEventSource<T, ?, ?> managedInformerEventSource, | ||
boolean parseResourceVersions) { | ||
this.managedInformerEventSource = managedInformerEventSource; | ||
this.parseResourceVersions = parseResourceVersions; | ||
} | ||
|
||
public synchronized Optional<T> removeResourceFromCache(T resource) { | ||
return Optional.ofNullable(cache.remove(ResourceID.fromResource(resource))); | ||
public synchronized void onEvent(T resource, boolean unknownState) { | ||
cache.computeIfPresent(ResourceID.fromResource(resource), | ||
(id, cached) -> (unknownState || !isLaterResourceVersion(id, cached, resource)) ? null | ||
: cached); | ||
} | ||
|
||
public synchronized void putAddedResource(T newResource) { | ||
|
@@ -61,18 +67,37 @@ public synchronized void putResource(T newResource, String previousResourceVersi | |
.orElse(managedInformerEventSource.get(resourceId).orElse(null)); | ||
|
||
if ((previousResourceVersion == null && cachedResource == null) | ||
|| (cachedResource != null && previousResourceVersion != null | ||
&& cachedResource.getMetadata().getResourceVersion() | ||
.equals(previousResourceVersion))) { | ||
|| (cachedResource != null | ||
&& (cachedResource.getMetadata().getResourceVersion().equals(previousResourceVersion)) | ||
|| isLaterResourceVersion(resourceId, newResource, cachedResource))) { | ||
log.debug( | ||
"Temporarily moving ahead to target version {} for resource id: {}", | ||
newResource.getMetadata().getResourceVersion(), resourceId); | ||
putToCache(newResource, resourceId); | ||
} else { | ||
if (cache.remove(resourceId) != null) { | ||
log.debug("Removed an obsolete resource from cache for id: {}", resourceId); | ||
} else if (cache.remove(resourceId) != null) { | ||
log.debug("Removed an obsolete resource from cache for id: {}", resourceId); | ||
} | ||
} | ||
|
||
/** | ||
* @return true if {@link InformerConfiguration#parseResourceVersions()} is enabled and the | ||
* resourceVersion of newResource is numerically greater than cachedResource, otherwise | ||
* false | ||
*/ | ||
private boolean isLaterResourceVersion(ResourceID resourceId, T newResource, T cachedResource) { | ||
try { | ||
if (parseResourceVersions | ||
&& Long.compare(Long.parseLong(newResource.getMetadata().getResourceVersion()), | ||
Long.parseLong(cachedResource.getMetadata().getResourceVersion())) > 0) { | ||
return true; | ||
} | ||
} catch (NumberFormatException e) { | ||
log.debug( | ||
"Could not compare resourceVersions {} and {} for {}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be this a warning? even if it could pollute cache? (no strong opinion) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't pollute the cache, it would exhibit the same behavior as it does now. It can of course be at whatever level you want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean pollute logs :) |
||
newResource.getMetadata().getResourceVersion(), | ||
cachedResource.getMetadata().getResourceVersion(), resourceId); | ||
} | ||
return false; | ||
} | ||
|
||
private void putToCache(T resource, ResourceID resourceID) { | ||
|
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.
Using an annotation is an implementation detail towards achieving filtering of own updates. If we want to explain that an annotation is used for this purpose, either we need to document in greater details how the annotation is used (so that users can actually use that information) or not mention it at all, imo.
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.
Other than specifying what the annotation name is, there isn't much greater detail that is needed here - the user is not expected to do anything with these annotations.
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.
Yes, I think it would make sense to document the annotation name so that people can look it up in case people notice it in their resources and wonder what that is.