Skip to content

2.31.0.0-b372

@rajivkumar-dev rajivkumar-dev tagged this 20 Aug 14:00
Summary:
[YBA] Fix incremental backup failure "Got empty success marker" caused by non-idempotent
success-marker download

Incremental backups first download the base backup's success marker (DSM) via
YbcManager.downloadSuccessMarker(), which submits a create-task RPC (restoreNamespace with
dsm=true) using a deterministic task id, then polls for its result. YbcClient retries
UNAVAILABLE/DEADLINE_EXCEEDED internally (ybc.client_settings.max_unavailable_retries,
default 10), so when YB-Controller is unresponsive and a reply is dropped after the task was
already registered, the retried submit gets back EXISTS. The old code treated any non-OK
status as fatal, swallowed it, and returned null, which the caller reported as:

  Failed to send download success marker request, failure status: EXISTS
  Got empty success marker for base backup with params identifier <uuid>

The DSM task was also deleted only on the success path, so once an attempt failed the task
stayed registered on YB-Controller and every later retry of the same backup hit EXISTS again,
failing indefinitely.

managed/src/main/java/com/yugabyte/yw/common/backuprestore/ybc/YbcManager.java
- downloadSuccessMarker() now accepts both OK and EXISTS on the create RPC and continues to
  polling, since EXISTS means an earlier submit of the same task id already registered the
  task and its result is still fetchable. This matches how backupNamespace in BackupTableYbc
  and restoreNamespace in RestoreBackupYbc already handle EXISTS.
- Null-check the restoreNamespace response instead of dereferencing it. YbcClient returns
  null when it exhausts its own retries, which previously produced a swallowed NPE logged as
  "Error: null".
- Make polling null-tolerant: the result supplier records the last non-null response in an
  AtomicReference and the stop predicate treats null as "not done yet", so one failed result
  RPC mid-poll keeps polling instead of throwing out of RetryTaskUntilCondition.
- Honor the previously ignored return value of retryUntilCond and fail with an explicit
  "Timed out after N seconds ... last known status: X" instead of the misleading
  "Failed to download success marker, failure status: IN_PROGRESS".
- Drop the redundant second backupServiceTaskResult call after polling, removing an extra RPC
  and a race where the task could be reaped between the two calls.
- Move deleteYbcBackupTask into a finally block so failed, timed-out and aborted attempts no
  longer leave the task registered on YB-Controller. deleteYbcBackupTask is safe here: it
  returns early on NOT_FOUND and never throws.
- Let aborts through: a CancellationException raised by RetryTaskUntilCondition on thread
  interrupt is rethrown (with a non-null message) instead of being swallowed, so a user abort
  during marker download is recorded as an abort rather than a backup failure.
- Log the real cause by passing the exception to LOG.error, and drop the misleading
  "for restore" wording since backups use this path too.
- No signature or contract change: the method still returns null on failure, so
  BackupTableYbc, RestoreBackupYbc, RestoreUniverseKeysYbc and BackupHelper.getYbcSuccessMarker
  are unaffected.

managed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/BackupTableYbc.java
- Make the cancellation handler null-safe: ce.getMessage().contains(...) becomes
  StringUtils.contains(ce.getMessage(), ...), so a CancellationException without a message
  cannot NPE inside the catch block.

managed/src/test/java/com/yugabyte/yw/common/backuprestore/ybc/YbcManagerTest.java
- Add unit coverage for downloadSuccessMarker: EXISTS on submit, null submit response, null
  result response mid-poll, poll timeout, terminal failure status, exactly one cleanup on the
  success path, and abort propagation.

Test Plan:
New unit tests in YbcManagerTest:
- testDownloadSuccessMarkerTaskAlreadyExistsOnYbc: submit returns EXISTS and the result
  returns OK, the marker is returned instead of failing. This is the reported bug.
- testDownloadSuccessMarkerNullCreateResponse: submit returns null, the call returns null with
  no NPE, never polls for a result, and still attempts cleanup.
- testDownloadSuccessMarkerNullResultResponseKeepsPolling: the first result RPC returns null
  and the second returns OK, the marker is returned.
- testDownloadSuccessMarkerTimeout: result stays IN_PROGRESS past the configured timeout, the
  call returns null and cleanup runs.
- testDownloadSuccessMarkerFailureStatus: result returns a terminal non-OK status, the call
  returns null and cleanup runs.
- testDownloadSuccessMarkerSuccessDeletesTaskOnce: success path deletes the DSM task exactly
  once, guarding against a double delete after moving it into finally.
- testDownloadSuccessMarkerAbortPropagates: interrupting the task thread during polling
  surfaces a CancellationException with a non-null message, and cleanup still runs.

Automated runs:
- sbt "testOnly com.yugabyte.yw.common.backuprestore.ybc.YbcManagerTest
  com.yugabyte.yw.commissioner.tasks.CreateBackupTest
  com.yugabyte.yw.common.backuprestore.BackupHelperTest"
  Result: Passed: Total 75, Failed 0, Errors 0, Passed 75.
  CreateBackupTest covers the BackupTableYbc subtask flow including testBackupTableYbcRetry
  and testAbortBackup; BackupHelperTest covers the restore-preflight success-marker path.
- sbt compile and sbt Test/compile succeed.
- sbt Test/javafmtCheck passes; arc lint reports no lint messages for the changed files.

Manual validation:
- Run an incremental backup on a YBC universe and confirm it succeeds and that no
  <taskid>_success_marker task remains on YB-Controller afterwards.
- Force one DSM failure (for example break the storage config credentials briefly so the
  marker download fails), restore access, then retry the backup task. Before this change the
  retry failed permanently with "failure status: EXISTS"; now it succeeds.
- Abort a backup while the marker download is in flight and confirm the task is reported as
  Aborted rather than Failed.

Reviewers: vkumar, anabaria, agunta, aaryan.chauhan, dshubin

Reviewed By: dshubin

Subscribers: svc_phabricator, agunta, yugaware

Differential Revision: https://phorge.dev.yugabyte.com/D56351
Assets 2
Loading