Resilient, Resumable Image Replication for Intermittent Edge Connecti… - #634
Draft
deepshikhatutorials wants to merge 1 commit into
Draft
Resilient, Resumable Image Replication for Intermittent Edge Connecti…#634deepshikhatutorials wants to merge 1 commit into
deepshikhatutorials wants to merge 1 commit into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 34 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/satellite/state/replicator.go">
<violation number="1" location="internal/satellite/state/replicator.go:192">
P3: The `operation` parameter of `retryWithBackoff` is never used in the function body. All three callers build a formatted string that is silently discarded, and the function logs nothing with it. Either use it (e.g. log which operation is retried) or drop the parameter to avoid dead ceremony.</violation>
<violation number="2" location="internal/satellite/state/replicator.go:224">
P3: The loop in `retryWithBackoff` returns unconditionally on every path (success, non-retryable/max-attempt error, and cancellation), so the trailing `if lastErr != nil { return lastErr }` + `return nil` is dead code and `lastErr` is only ever written. Remove the variable and the unreachable block after the loop to avoid misleading readers into thinking there is a fallback path.</violation>
<violation number="3" location="internal/satellite/state/replicator.go:285">
P2: When DNS resolution temporarily fails with `no such host`, this classification treats the transport failure as a permanent missing-image error. Remove DNS errors from `isNotFoundError` and classify them as retryable network errors instead.</violation>
</file>
<file name="internal/satellite/state/replicator_test.go">
<violation number="1" location="internal/satellite/state/replicator_test.go:170">
P3: This test drives retryWithBackoff's real backoff timer: the two transient failures each sleep (attempt 1 = 250ms, attempt 2 = 500ms), adding ~750ms of wall-clock time to the suite. Since the delay in retryWithBackoff is hardcoded and not injectable, consider making the delay configurable (e.g., a no-op/short delay passed by the test) or verifying the retry count without relying on real sleep.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return strings.Contains(msg, "not found") || | ||
| strings.Contains(msg, "manifest unknown") || | ||
| strings.Contains(msg, "name invalid") || | ||
| strings.Contains(msg, "no such host") |
Contributor
There was a problem hiding this comment.
P2: When DNS resolution temporarily fails with no such host, this classification treats the transport failure as a permanent missing-image error. Remove DNS errors from isNotFoundError and classify them as retryable network errors instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/state/replicator.go, line 285:
<comment>When DNS resolution temporarily fails with `no such host`, this classification treats the transport failure as a permanent missing-image error. Remove DNS errors from `isNotFoundError` and classify them as retryable network errors instead.</comment>
<file context>
@@ -176,6 +189,102 @@ func (r *BasicReplicator) Replicate(ctx context.Context, replicationEntities []E
+ return strings.Contains(msg, "not found") ||
+ strings.Contains(msg, "manifest unknown") ||
+ strings.Contains(msg, "name invalid") ||
+ strings.Contains(msg, "no such host")
+}
+
</file context>
| return nil | ||
| } | ||
|
|
||
| if lastErr != nil { |
Contributor
There was a problem hiding this comment.
P3: The loop in retryWithBackoff returns unconditionally on every path (success, non-retryable/max-attempt error, and cancellation), so the trailing if lastErr != nil { return lastErr } + return nil is dead code and lastErr is only ever written. Remove the variable and the unreachable block after the loop to avoid misleading readers into thinking there is a fallback path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/state/replicator.go, line 224:
<comment>The loop in `retryWithBackoff` returns unconditionally on every path (success, non-retryable/max-attempt error, and cancellation), so the trailing `if lastErr != nil { return lastErr }` + `return nil` is dead code and `lastErr` is only ever written. Remove the variable and the unreachable block after the loop to avoid misleading readers into thinking there is a fallback path.</comment>
<file context>
@@ -176,6 +189,102 @@ func (r *BasicReplicator) Replicate(ctx context.Context, replicationEntities []E
+ return nil
+ }
+
+ if lastErr != nil {
+ return lastErr
+ }
</file context>
| return nil | ||
| } | ||
|
|
||
| func retryWithBackoff(ctx context.Context, operation string, fn func() error) error { |
Contributor
There was a problem hiding this comment.
P3: The operation parameter of retryWithBackoff is never used in the function body. All three callers build a formatted string that is silently discarded, and the function logs nothing with it. Either use it (e.g. log which operation is retried) or drop the parameter to avoid dead ceremony.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/state/replicator.go, line 192:
<comment>The `operation` parameter of `retryWithBackoff` is never used in the function body. All three callers build a formatted string that is silently discarded, and the function logs nothing with it. Either use it (e.g. log which operation is retried) or drop the parameter to avoid dead ceremony.</comment>
<file context>
@@ -176,6 +189,102 @@ func (r *BasicReplicator) Replicate(ctx context.Context, replicationEntities []E
return nil
}
+func retryWithBackoff(ctx context.Context, operation string, fn func() error) error {
+ const maxAttempts = 5
+ var lastErr error
</file context>
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestRetryWithBackoff_RetriesTransientFailures(t *testing.T) { |
Contributor
There was a problem hiding this comment.
P3: This test drives retryWithBackoff's real backoff timer: the two transient failures each sleep (attempt 1 = 250ms, attempt 2 = 500ms), adding ~750ms of wall-clock time to the suite. Since the delay in retryWithBackoff is hardcoded and not injectable, consider making the delay configurable (e.g., a no-op/short delay passed by the test) or verifying the retry count without relying on real sleep.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/state/replicator_test.go, line 170:
<comment>This test drives retryWithBackoff's real backoff timer: the two transient failures each sleep (attempt 1 = 250ms, attempt 2 = 500ms), adding ~750ms of wall-clock time to the suite. Since the delay in retryWithBackoff is hardcoded and not injectable, consider making the delay configurable (e.g., a no-op/short delay passed by the test) or verifying the retry count without relying on real sleep.</comment>
<file context>
@@ -166,6 +167,41 @@ func TestReplicate_EmptyEntities(t *testing.T) {
require.NoError(t, err)
}
+func TestRetryWithBackoff_RetriesTransientFailures(t *testing.T) {
+ ctx := testContext()
+ attempts := 0
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change do?
This PR adds the first resilience layer to Harbor Satellite's single-image replication flow.
Harbor Satellite is expected to operate in edge environments where registry and network connectivity can be slow, intermittent, or temporarily unavailable. At the moment, a temporary connection failure during replication can cause the entire operation to fail even when the failure could be recovered from with a retry.
This change adds retry and backoff handling around the main replication steps so that transient failures can recover automatically.
What changed?
Why does this matter?
Harbor Satellite is designed for environments where network connectivity cannot always be relied upon.
For example, a satellite may be connected through an unstable cellular connection and temporarily lose connectivity while pulling or pushing an image. Without retry handling, the replication attempt can fail immediately and require the operation to be started again.
This is especially important for large container images because a failed transfer can waste bandwidth and delay the image becoming available at the edge.
By retrying transient failures with backoff, replication can recover from short-lived network problems without treating every connection interruption as a permanent failure.
The destination digest check also helps avoid unnecessary work when the requested image is already available.
Scope
This PR intentionally focuses on the initial single-image resilience work.
The goal at this stage is to establish reliable retry behavior around the existing replication flow rather than introduce a complete resumable-transfer system.
The current scope covers:
Full restart recovery and persistent replication state are not included in this PR.
Architecture direction
This work is intended as an initial step toward the resilient replication behavior described in #631.
The replication architecture is also moving toward the ORAS-based direction described in ADR-0009. The current changes therefore keep the resilience logic focused and isolated so that the transfer/retry behavior can be further evolved as the ORAS-based replication path is introduced.
This should also provide a foundation for future satellite-to-satellite P2P replication work without expanding the scope of this PR prematurely.
What is not covered yet?
This PR does not yet provide:
These can be addressed in follow-up work once the preferred replication architecture and ORAS migration approach are confirmed.
Testing
Added tests covering:
Existing replication tests are kept unchanged to ensure the current replication behavior continues to work.
Related Issue
Closes #631