Skip to content

release-23.1: publish-artifacts: change GCS latest key prefix for Workload binary#156887

Closed
williamchoe3 wants to merge 1 commit intocockroachdb:release-23.1from
williamchoe3:backport23.1-156883
Closed

release-23.1: publish-artifacts: change GCS latest key prefix for Workload binary#156887
williamchoe3 wants to merge 1 commit intocockroachdb:release-23.1from
williamchoe3:backport23.1-156883

Conversation

@williamchoe3
Copy link
Copy Markdown
Contributor

@williamchoe3 williamchoe3 commented Nov 4, 2025

Backport 1/1 commits from #156883.

/cc @cockroachdb/release

Backport 1/1 commits from #156855.

/cc @cockroachdb/release

Backport 1/2 commits from #156092.

/cc @cockroachdb/release

Original PR that was reverted: #155789

Changes split into 2 separate commits to allow for backporting only the required changes for easier merge conflict resolution

Informs #154274

In gcs gs://cockroach-edge-artifacts-prod, change latest Workload binary key prefix from cockroach to workload
i.e.

gs://cockroach-edge-artifacts-prod/cockroach/workload.release-24.1
to
gs://cockroach-edge-artifacts-prod/workload/workload.release-24.1

This will allow latest workload binary to not be deleted after 30 days so tests can download and use workload binaries from unsupported versions (that no longer have nightlies running against them)

Notes:

  • Will need to backfill currently unsupported versions
  • ".../workload/workload...." while descriptive also sounds redundant, open to suggestions if folks don't like that prefix
  • The non latest copy of the obj i.e. gs://cockroach-edge-artifacts-prod/cockroach/workload.ee80eab77ca6434f4be7c33c0f6d29dd0b9d07b1 is still prefixed with cockroach so these objects will get deleted after 30 days with the current lifecycle policy

Edited PutNonRelease's docstring to remove "redirect" terminology, looks to be from a previous s3 implementation which is not a feature of GCS.

  • Note: I don't feel that strongly about the docstring, it just confused me a bit when reading the code and just wanted it to match what the function is actually doing

Also noticed there's no unit test coverage for non master branches, added 1 release branch test with simple coverage

Also noticed there's a slight difference between mockGCSProvider.PutObject & release.PutObject

# obj latest key names on gs://cockroach-edge-artifacts-prod
gs://cockroach-edge-artifacts-prod/cockroach/workload.release-23.2

# obj latest key names in unit tests
gs://edge-binaries-bucket/workload/workload.release-25.4/no-cache

Actual release.PutObject does not modify the key name based on the PutObjectInput.CacheControl field, but in mockGCSProvider.PutObject the "/no-cache" gets appended
I changed the behavior in the mock to match the actual function, and modified the unit tests
FWIW PutObjectInput.CacheControl is only set to False currently

  • It looks like this might be a hold over from the S3 implementation? I didn't read that diff but let me know if I should revert this change

This PR is a dependency for these roachtest changes: #155397
Also discussed other options like using a new bucket or using the release bucket here: https://cockroachlabs.slack.com/archives/C03SG8QKYRJ/p1760977289072049?thread_ts=1760976858.868889&cid=C03SG8QKYRJ
Decided on this approach for simplicity

For verification, I'm relying on the unit tests. Just assumed this wouldn't work on my local, but if there's a simple way to verify this E2E let me know and i'll do it

Will squash and write a proper commit msg after review, leaving commit separate incase i need to revert anything

Release justification: ci test artifact only changes

Release justification: ci test artifact only changes

Release justification: ci test artifact only changes

@williamchoe3
publish-artifacts: change GCS latest key prefix for Workload binary
ac2ac9d 
Change latest key prefix from 'cockroach' to 'workload' for workload binary to
allow for separate lifecycle policies for workload binary objects.

Informs cockroachdb#154274

Epic: None
Release note: None
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Nov 4, 2025 •
edited by williamchoe3
Loading

Thanks for opening a backport.

Before merging, please confirm that it falls into one of the following categories (select one):

  • Non-production code changes. Includes test-only changes, build system changes, etc.
  • Fixes for serious issues. Defined in the policy as correctness, stability, or security issues, data corruption/loss, significant performance regressions, breaking working and widely used functionality, or an inability to detect and debug production issues.
  • Other approved changes. These changes must be gated behind a disabled-by-default feature flag unless there is a strong justification not to.

Add a brief release justification to the PR description explaining your selection.

Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy.

All backports must be reviewed by the TL and EM for the owning area.

@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-testeng TestEng Team labels Nov 4, 2025
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Nov 4, 2025

This change is Reviewable

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Nov 4, 2025

✅ PR #156887 is compliant with backport policy

Confidence: high
Backward compatible: true
Explanation: The pull request is compliant with the backport policy based on the provided release justification in the PR body, which indicates that it involves changes solely related to CI test artifacts. According to the CockroachDB backport policy, changes affecting non-production code like development tools, tests, or CI configurations are exempt from the standard requirements of addressing critical bugs or being gated behind a feature flag. Both changed files pkg/cmd/publish-artifacts/main_test.go (a test file) and pkg/release/upload.go (related to release artifact uploads, impacting testing or build but not production runtime) fit this exemption as they deal with the infrastructure for building and testing, not production runtime code. Additionally, the PR modifies behavior related to CI testing processes suggesting it won't affect the actual database functionality or stability.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@williamchoe3
Copy link
Copy Markdown
Contributor Author

williamchoe3 commented Nov 4, 2025

Unrelated unit test failures

TestAuthenticationAndHBARules

Rerunning https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelEssentialCi/20694837?hideProblemsFromDependencies=false&hideTestsFromDependencies=false

@williamchoe3 williamchoe3 mentioned this pull request Nov 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

backport Label PR's that are backports to older release branches T-testeng TestEng Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@williamchoe3 @cockroach-teamcity