[FLINK-39953] Fix cluster deletion timeout silently swallowed during upgrade#1138
Conversation
04084ca to
d78e1fb
Compare
d78e1fb to
449690a
Compare
Dennis-Mircea
left a comment
There was a problem hiding this comment.
Thanks for opening this PR! The swallowed timeout is a real smell, but the logging is wrong: deleteBlocking logs ERROR and rethrows (double-logging upstream). In one place you are logging a WARN and in other you are logging ERROR, making it inconsistent. Also, the logging massage framing doesn't look to me either.
From a different point of view, this is addressed only for the native operator path, but not also the standalone part.
Still, before proceeding, I'd suggest confirming the diagnosis on the JIRA with a real reproduction first, then: leave logging to the callers, and audit/test the full set of deleteBlocking callers (standalone + clean-up), not just the native upgrade path.
449690a to
f8ed3c5
Compare
|
Thank your for taking the time to review my change @Dennis-Mircea ! Answering to your points: I added the minimum steps to reproduce the issue on the ticket description in JIRA: https://issues.apache.org/jira/browse/FLINK-39953 Regarding double logging: Regarding Standalone part: Happy to address any other concerns that appear. |
What is the purpose of the change
During an upgrade,
ApplicationReconciler.deploy()callsdeleteClusterDeployment()to tear down the old cluster and then immediately callssubmitApplicationCluster()to start the new one. If the old JobManager Deployment takes longer to disappear than the configured shutdown timeout,deleteBlocking()was silently catching theKubernetesClientTimeoutExceptioninside a broaderKubernetesClientExceptionhandler and returning normally, so the operator proceeded to create a new Deployment while the old one was still running. This caused port conflicts on the-restService or stale endpoint routing.This change makes the timeout propagate so the reconciler requeues instead of proceeding.
FLINK-39953
Brief change log
Adds an explicit
catch (KubernetesClientTimeoutException)toAbstractFlinkService.deleteBlocking()before the genericKubernetesClientExceptionhandler so the timeout propagates to the caller.NativeFlinkService.shutdownJobManagersBlocking(): wrap thedeleteBlocking()call for the optional scale-to-zero pre-step in its owncatch (KubernetesClientTimeoutException)since a timeout there should not block the actual Deployment deletion from proceeding.Verifying this change
This change added some unit tests ensuring the new behavior. For the
AbstractFlinkServiceTestpart:testBlockingDeploymentDeletion: The existing timeout case now assertsKubernetesClientTimeoutExceptionis thrown.testDeleteBlockingTimeoutThrows: Direct test ofdeleteBlocking()where the mock server always returns the Deployment, asserts the exception is thrown.Also added tests to
NativeFlinkServiceTestandStandaloneFlinkServiceTestensuring the expection propagates in both cases.Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: noAbstractFlinkService.deleteBlocking)Documentation