📺 Info Siaran: uber/kraken/pull/

Thanatos9404 · 2026-10-16T06:11:13Z

Fixes #506

The docker tag type retries manifest downloads with a backoff that is hard coded in tagtype.NewMap (the default* constants added in #502). Following the review of #502, this lets each tag_types entry set it in the build-index config.

tagtype.Config gets a backoff field of type httputil.ExponentialBackOffConfig, the type already used for sync_retry_backoff and the http backend's download_backoff:

tag_types:
  - namespace: .*
    type: docker
    backoff:
      initial_interval: 1s
      max_retries: 5

Fields left at zero keep the current values (500ms initial interval, 0.05 randomization factor, multiplier 2, 30s max interval, 3 retries), so a config without backoff behaves exactly as before. The defaults wait about 3.5s in total; keep the total under the 30s timeout tagclient uses for tag PUTs, or the caller gives up while build-index is still retrying.

Retries stay on for the docker type and enabled is ignored. A missing enabled reads as false, so honoring enabled: false would also turn retries off for every existing config without a backoff section. sync_retry_backoff instead ignores all values unless enabled: true is set; I used per-field defaults so a partial backoff section just works, but can switch to the sync_retry_backoff behavior if you prefer. As with the other users of this type, max_retries: 0 means the default, so the smallest setting is max_retries: 1.

I kept backoff on the tag type entry, next to the resolver that uses it, and reused the shared backoff type. If you'd rather have a single build-index level setting, closer to the shared retry policy suggested in #502, I can move it.

Tests

TestMapDockerBackoffConfig in build-index/tagtype/map_test.go loads tag_types from yaml and checks the backoff the docker resolver gets in four cases: no backoff, every field set, only some fields set (the rest fall back to the defaults), and enabled: false (still the defaults). On master the two override cases fail because the backoff key is ignored and the resolver gets the defaults; with this change all four pass.

Run on Linux (WSL Ubuntu) with Go 1.24.13, the version CI uses:

go test -race -timeout=30s --tags unit ./build-index/...: tagserver, tagstore and tagtype pass (14 tests in tagtype)
go vet --tags unit ./build-index/... and gofmt -l build-index: clean
golangci-lint run ./build-index/... and golangci-lint run --new-from-rev=master ./... (v2.14.0, repo .golangci.yml): 0 issues

The docker tag type retries manifest downloads with a backoff that was hard coded in tagtype.NewMap, so tuning it needed a code change. Add a backoff field to tagtype.Config so each tag_types entry can set it in yaml. It reuses httputil.ExponentialBackOffConfig, the type already behind sync_retry_backoff and download_backoff. Fields left at zero keep the current values (500ms initial interval, 0.05 randomization factor, multiplier 2, 30s max interval, 3 retries), so existing configs behave as before. Retries stay enabled for the docker type and enabled is ignored, since a missing enabled reads as false. Fixes uber#506.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

CLAassistant · 2026-10-16T06:11:30Z

All committers have signed the CLA.

thijmv

LGTM.

I will leave this open for a couple of days before merging, in case you want to address the nits.

thijmv · 2026-10-24T12:47:42Z

 }
+
+func TestMapDockerBackoffConfig(t *testing.T) {
+	defaults := httputil.ExponentialBackOffConfig{


nit: Consider using the backoff constants for this.

Thanks, done in eef5eaf: the defaults case now uses the constants.

thijmv · 2026-10-24T13:07:21Z

+			m, err := NewMap(configs, nil)
+			require.NoError(err)
+
+			r, ok := m.subResolvers[0].resolver.(*dockerResolver)


nit: Consider asserting m.subResolvers has length 1.

Added require.Len(m.subResolvers, 1) before the type assertion.

thijmv · 2026-10-24T13:09:35Z

+    initial_interval: 2s
+    max_retries: 6
+`,
+			want: partial,


nit: Can we inline partial to make it more explicit and readable?

Inlined it, so the expected config for that case is now written out in full.

thijmv · 2026-10-24T13:12:52Z

 	Type      string `yaml:"type"`
+
+	// Backoff controls how the docker type retries manifest downloads. Fields
+	// left at zero take the defaults above, and retries are always enabled.


nit: Can we be more explicit about the backoff config always being enabled?

Reworded the field comment to say that Backoff.Enabled is ignored and always set to true for the docker type, and added a short note in applyDefaults on why (a missing enabled key reads as false).

Use the default backoff constants in TestMapDockerBackoffConfig, inline the partially configured case, and check that the map holds exactly one sub resolver before the type assertion. Also say plainly in the Config and applyDefaults comments that retries are always enabled for the docker type.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

thijmv · 2026-10-25T11:37:24Z

Hi @Thanatos9404, the license CI is not passing. Can you try resigning it so we can merge this?

Thanatos9404 · 2026-10-25T11:56:12Z

Thanks! The CLA is already signed (CLA Assistant confirmed it on 16 Oct), but it didn't post a status on the latest commit. I've re-run the check from the CLA Assistant link; let me know if it still shows as missing on your side.

cm-howard · 2026-10-27T13:50:48Z

Heads up: this PR is being closed and immediately re-opened to re-trigger the CLA Assistant. The license/cla status was never reported on the latest commit, so this is to help remedy the missing CLA attribution. No action is needed from you; the CLA check should update shortly.

Copilot AI balanced review requested due to automatic review settings October 16, 2026 06:11

Copilot AI reviewed Oct 16, 2026

Thanatos9404 mentioned this pull request Oct 16, 2026

feat(build-index): count manifest downloads that succeed after a retry #723

Merged

gkeesh7 requested a review from thijmv October 22, 2026 11:15

thijmv approved these changes Oct 24, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 24, 2026 13:50

Copilot AI reviewed Oct 24, 2026

thijmv approved these changes Oct 25, 2026

View reviewed changes

cm-howard closed this Oct 27, 2026

cm-howard reopened this Oct 27, 2026

thijmv merged commit b75fbcb into uber:master Oct 27, 2026
21 checks passed

Conversation

Thanatos9404 commented Oct 16, 2026

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thijmv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

thijmv commented Oct 25, 2026

Uh oh!

Thanatos9404 commented Oct 25, 2026

Uh oh!

cm-howard commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Oct 16, 2026 •

edited

Loading