Dennis-Mircea commented on code in PR #1207:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1207#discussion_r3959190636
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/state/ConfigMapStore.java:
##########
@@ -93,9 +102,19 @@ public void removeInfoFromCache(ResourceID resourceID) {
cache.remove(resourceID);
}
+ private static boolean isNotFound(Exception e) {
+ return e instanceof KubernetesClientException
+ && ((KubernetesClientException) e).getCode() ==
HttpURLConnection.HTTP_NOT_FOUND;
+ }
+
private ConfigMapView getConfigMap(KubernetesJobAutoScalerContext
jobContext) {
- return cache.computeIfAbsent(
- jobContext.getJobKey(), (id) ->
getConfigMapFromKubernetes(jobContext));
+ var ownerUid = jobContext.getResource().getMetadata().getUid();
+ return cache.compute(
Review Comment:
This comment made me think one more time about the whole logic here, so I
took a step back and reassessed everything one more time.
So, the assessment of the existent code within the operator is the following:
- The resourceRetriever (which talks directly with the K8s API server) is
called on the first GET and cached afterwards, and on flush for any updated key
for a different owner reference.
- When the actual resource was recreated, it didn’t depend on whether it was
out of or mid-autoscaler reconciliation cycle, and the flush failed with an
exception, discarding all the data that was updated within that cycle, and
triggering a new recreation of the ConfigMap (based on clearing the cache) on
the next cycle. Also, if the ConfigMap takes more than 1 cycle to be entirely
deleted, it is continuously writing to a stale ConfigMap that will be deleted
anyway.
- This approach, besides the actual exception and error event emitted, is
also losing at least one autoscaler cycle of computations, which does have
importance even if we are in the autoscaler metric stabilization window.
The assessment of the previous implementation that I did was the following:
- The resourceRetriever is called on every GET, and on flush for any updated
key for a different owner reference. It is not cached because, when building
the ConfigMapView it will see it as already existent and will use the existent
ConfigMap data, which references the old owner reference, which causes no cache
hit while the ConfigMap is not garbage-collected, bringing in place the
following scenarios:
- if the previous old ConfigMap is still in place for the whole cycle
and gets recreated until the next one, we have the same data discarding/loss as
before + no error + multiple Gets.
- If the previous old ConfigMap is getting deleted mid-cycle, then we
are at risk of discarding just a portion of the data (just some key-value
pairs) while preserving the other, and this brings real inconsistency
- If the previous old ConfigMap is getting deleted instantly before the
cycle
So, even with the suggestion that you brought above, this will still cache
the old ConfigMap data with the new owner reference while the old reference
ConfigMap still being in place, which can still cause the flush at the end to
fail with a 404. This will invalidate the cache and will trigger a new
recreation of the ConfigMap (based on clearing the cache) on the next cycle.
This will keep the same data discarding logic as before. Still, this approach
is losing at least one autoscaler cycle of computations, so is not good enough.
Based on the above, I took a step further and I updated the implementation
with the following logic:
- On each GET, if the owner reference is different, then get the existing
ConfigMap (if it wasn’t garbage collected already) and take it over by updating
the owner reference and clearing the data which is no longer relevant, by doing
all this computation in memory and by updating the cache, which will no longer
trigger a miss on the owner reference.
- At the flush time, try to update the physical ConfigMap if still present
with the in-memory snapshot, if not, create it from scratch with the data
collected so far.
To sum up, with the above updated approach, we will no longer be at risk of
data inconsistency, we will have determinism, and we will not lose any
autoscaler cycle of computations. I updated the PR content + description to
reflect this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
