Repository navigation
HDDS-16960. Keep secret key polling running after refresh failures - #11824
Conversation
Catch and log failures at the scheduled task boundary so subsequent polls continue. Cover recovery after consecutive failures while preserving manual refetch error propagation. Tests: 39 secret key and token unit tests passed; checkstyle and git diff --check passed.
|
The PR addresses a valid issue. There are two different retry mechanisms:
Looks good. Let's see what copilot says. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
What changed in this PR
Ensures secret-key polling continues after refresh failures so OM/datanodes can recover once SCM is healthy again, preventing long-lived usage of expired cached keys.
Changes:
- Wrap scheduled polling refresh in a try/catch to prevent the ScheduledExecutor task from terminating on exceptions/errors.
- Add a regression test that simulates multiple consecutive failures and verifies the poller eventually refreshes the cached key.
| File | Description |
|---|---|
| hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/DefaultSecretKeySignerClient.java | Prevents the scheduled poller from being permanently suppressed by exceptions by catching and logging at the task boundary. |
| hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/TestDefaultSecretKeyClient.java | Adds regression coverage proving the poller keeps retrying after consecutive failures and eventually refreshes to a new key. |
Review effort: Lite
| GenericTestUtils.waitFor(() -> newKey.equals(client.getCurrentSecretKey()), 10, 5_000); | ||
| verify(scm, times(5)).getCurrentSecretKey(); |
There was a problem hiding this comment.
The poller runs every 10ms, but it only calls SCM when the cached key is past its rotation interval. Once newKey is cached, its creation time is current and the rotation interval is one day, so subsequent polls skip the SCM call.
The expected count therefore remains exactly five: one initial fetch, three failed refreshes, and one successful refresh. times(5) should be safe here.
Add regression coverage for VirtualMachineError and ThreadDeath while preserving recovery from other refresh failures. Validation: 40 secret key and token tests passed; repository checkstyle and git diff --check passed.
|
CI in the fork is green: https://github.com/andyhuangdev/ozone/actions/runs/40795286459 |
|
Merged. Thanks @andyhuangdev |
What changes were proposed in this pull request?
A failed secret key poll currently stops all subsequent polls because the exception escapes the scheduled task. OM and datanodes continue signing tokens with the cached key, eventually causing token verification failures when that key expires, even after SCM recovers.
This change catches and logs failures at the scheduled task boundary so polling continues at the next check. Manual refetch calls continue to propagate failures to their callers.
What is the link to the Apache JIRA
HDDS-16960
How was this patch tested?
A regression test was added first and confirmed to fail against the original implementation. It verifies that polling recovers after consecutive IOException, RuntimeException, and Error failures and updates the cached key.
After the fix, all 39 selected secret key and token unit tests passed, including the existing manual refetch tests. The repository checkstyle script and git diff --check also passed.