[FLINK-40542] Block session cluster deletion when running jobs cannot be determined - #1202
[FLINK-40542] Block session cluster deletion when running jobs cannot be determined#1202spuru9 wants to merge 1 commit into
Conversation
1a4ab84 to
46af4f9
Compare
Dennis-Mircea
left a comment
Dennis-Mircea
left a comment
There was a problem hiding this comment.
While I do agree with the idea of this PR, the fix is too narrow, as there are multiple other REST service/autoscaler methods that must benefit from the same fix that you did here.
Currently, there are the following operator-related methods that are not having the kubernetes.operator.flink.client.timeout set:
AbstractFlinkService#populateStateSizeAbstractFlinkService#fetchCheckpointStatsAbstractFlinkService#fetchCheckpointInfoAbstractFlinkService#fetchSavepointInfo
Further, there are the following autoscaler-related methods that should have the job.autoscaler.flink.rest-client.timeout set:
ScalingMetricCollector#queryAggregatedMetricNamesRestApiMetricsCollector#queryAggregatedVertexMetricsRestApiMetricsCollector#queryJmMetricsRestApiMetricsCollector#queryAggregatedTmMetrics
Should I add those to this PR? |
Yes, and update the JIRA and PR title and description. |
… be determined SessionReconciler.getNonTerminalJobs returned Set.of() on any failure to query the cluster. cleanupInternal treats an empty set as "no running jobs, safe to delete", so a transient JobManager outage during deletion of a session cluster with block-on-unmanaged-jobs enabled (the default) silently deleted the cluster while unmanaged jobs were still running - stopping them without a checkpoint, with only a WARN. Return Optional<Set<JobID>> so an indeterminate result (empty Optional) is distinguishable from a determined "no jobs" (present, empty set). cleanupInternal now blocks deletion and emits a CleanupFailed event on the indeterminate case, mirroring the block-on-session-jobs branch. Also close the RestClusterClient via try-with-resources and bound the jobs request with the Flink client timeout instead of an unbounded get(). The fail-closed change unmasked a deletion deadlock: a session cluster that never successfully deployed also has an unreachable JobManager, so it would block deletion forever instead of being torn down. cleanupInternal now checks isBeforeFirstDeployment() first and deletes immediately in that case, mirroring ApplicationReconciler and SessionJobReconciler. Also guard against a null jobs list, restore the interrupt flag on InterruptedException, and log CleanupFailed warnings consistently with the existing session-jobs branch. Per review feedback, apply the same bounded-timeout treatment to the other blocking REST calls that shared the unbounded get() pattern: AbstractFlinkService#fetchSavepointInfo, #fetchCheckpointInfo, #fetchCheckpointStats, #populateStateSize (bounded with the operator's kubernetes.operator.flink.client.timeout), and the autoscaler's ScalingMetricCollector#queryAggregatedMetricNames, RestApiMetricsCollector#queryAggregatedVertexMetrics, #queryJmMetrics, #queryAggregatedTmMetrics (bounded with job.autoscaler.flink.rest-client.timeout). Generated-by: Claude Code (Claude Opus 4.8)
46af4f9 to
94bc18c
Compare
|
@Dennis-Mircea Have updated the PR and JIRA. |
Dennis-Mircea
left a comment
Dennis-Mircea
left a comment
There was a problem hiding this comment.
After taking a deeper look at this one, it feels to me that we are touching a too sensitive place in order to achieve something that was never promised. The main question that I have here is: Why do we want to complicate ourselves on this existent stable flow just to turn the unmanaged session jobs check into a hard guarantee? Why would someone deploy an unmanaged session job with the expectation for having it "managed"? The operator does not give that expectation anywhere else, because an upgrade-type spec change already deletes and recreates the session cluster without checking unmanaged jobs at all. The check was introduced in FLINK-28648 as best-effort, and returning an empty set on failure was consistent with that.
So, here we are trying to resolve partially, not even fully, something that was never guaranteed by the operator. In order to see if this makes sense to be adopted fully within the operator I'd recommend opening a discussion thread where we can brainstorm this.
Speaking about the rest of the changes, I'd resume this PR/changes only for timeout resolution for REST API calls.
| if (e instanceof InterruptedException) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
I took a closer look on this one, and here by returning the Optional.empty() we cannot differentiate between a broken session JobManager and an unreachable session JobManager. Every failure lands in the same catch, so further, cleanupInternal has no way to tell a JobManager that is mid-restart from one that is crashlooping, in ImagePullBackOff, misconfigured, or whose Deployment was removed by hand. If we have a broken session JobManager (with or without unmanaged Flink session jobs), we end up in an endless loop with no cleanup, where the finalizer is never removed, the CR stays in Terminating, and the broken JM Deployment is never torn down.
| var deployment = ctx.getResource(); | ||
| var status = deployment.getStatus(); | ||
|
|
||
| if (status.getReconciliationStatus().isBeforeFirstDeployment()) { |
There was a problem hiding this comment.
This guard here doesn't seem to me to help and to tackle a broken session JobManager case. A broken session JobManager can most probably be deployed already (aka lastReconciledSpec != null) and this guard be bypassed entirely.
2 participants
What is the purpose of the change
SessionReconciler.getNonTerminalJobsreturnedSet.of()on any query failure, andcleanupInternaltreats an empty set as "safe to delete". So a sessionFlinkDeploymentwith an unreachable JobManager got deleted anyway during cleanup, stopping unmanaged jobs without a checkpoint.This makes the guard fail closed: an indeterminate result blocks deletion instead of assuming the cluster is empty. Per review feedback, it also bounds this and several other unbounded blocking REST calls in the operator and autoscaler with their configured client timeouts, so a hung JobManager can't stall them indefinitely.
Brief change log
getNonTerminalJobsnow returnsOptional<Set<JobID>>; empty means "could not determine", distinct from a present empty set ("no jobs").cleanupInternalblocks deletion and emitsCleanupFailedon the indeterminate case, rescheduling until the JobManager is reachable — mirroring the block-on-session-jobs branch. Also closes theRestClusterClientvia try-with-resources and bounds the query with the Flink client timeout.ApplicationReconciler/SessionJobReconciler. Also null-guards the jobs list and restores the interrupt flag onInterruptedException.AbstractFlinkService#fetchSavepointInfo/#fetchCheckpointInfo/#fetchCheckpointStats/#populateStateSize(kubernetes.operator.flink.client.timeout) andScalingMetricCollector#queryAggregatedMetricNames,RestApiMetricsCollector#queryAggregatedVertexMetrics/#queryJmMetrics/#queryAggregatedTmMetrics(job.autoscaler.flink.rest-client.timeout).Related: fail-open was introduced with block-on-unmanaged-jobs (FLINK-28648); the new block lifts automatically so it doesn't reintroduce the deadlock fixed in FLINK-39618. The existing opt-out (
block-on-unmanaged-jobs/block-on-session-jobs= false, FLINK-39432) still allows force-deleting a permanently unreachable cluster.Verifying this change
SessionReconcilerTest:testCleanupBlocksWhenNonTerminalJobsCannotBeDeterminedandtestCleanupDeletesImmediatelyWhenNeverDeployed.CleanupFailedevent and completes once reachable.AbstractFlinkServiceTest,ScalingMetricCollectorTest,RestApiMetricsCollectorTest,JobAutoScalerImplTestsuites.Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Claude Opus 4.8)