ROSAENG-1089: ensure unique hash4 DNS slug per cluster name - #308
Merged
openshift-merge-bot[bot] merged 1 commit intoAug 11, 2026
Merged
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Collaborator
|
@typeid: This pull request references ROSAENG-1089 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
Merged
1 task
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCluster creation now derives DNS prefixes from UUIDs, enforces active-cluster uniqueness in PostgreSQL, retries prefix collisions up to five times, and documents and tests the behavior, including concurrent creation. ChangesCluster DNS uniqueness
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ClusterHandler
participant Database
Client->>ClusterHandler: Submit cluster creation
ClusterHandler->>ClusterHandler: Generate UUID
ClusterHandler->>Database: Create cluster
Database-->>ClusterHandler: Success or AlreadyExists
ClusterHandler->>ClusterHandler: Retry with new UUID up to five times
ClusterHandler-->>Client: Return creation result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
platform-api/pkg/handlers/cluster_test.go (1)
565-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd deterministic coverage for hash4 collisions.
Line 565 only verifies the no-collision path. The direct
uuid.New()call prevents this test from forcing a collision. Inject an ID generator intoClusterHandler. Test one collision followed by a unique prefix. Test five collisions and verifyCLUSTERS-MGMT-CREATE-007.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/pkg/handlers/cluster_test.go` around lines 565 - 593, Add an injectable ID generator to ClusterHandler and replace the direct uuid.New() call in the create flow with it. Extend TestClusterHandler_Create_Hash4NoCollision with deterministic generator values, then add coverage for one hash4 collision followed by a unique ID and for five consecutive collisions, asserting the latter returns CLUSTERS-MGMT-CREATE-007.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/pkg/handlers/cluster.go`:
- Around line 141-159: Make cluster creation enforce (name, hash4) uniqueness
atomically in the persistence operation rather than relying on the preflight
ListClustersByName check in the cluster handler; retry UUID generation only when
that atomic create operation reports a uniqueness conflict, while preserving
non-conflict errors. Update hyperfleet-operator/docs/cluster-controller.md:75 to
claim the uniqueness guarantee only after the persistence layer provides this
atomic enforcement.
- Around line 141-155: Remove the request-derived cluster_name field from both
logger.Error calls in the cluster creation flow around ListClustersByName and
the hash4 retry loop. Keep the existing error messages and other non-sensitive
fields unchanged, without introducing another request-provided identifier.
---
Nitpick comments:
In `@platform-api/pkg/handlers/cluster_test.go`:
- Around line 565-593: Add an injectable ID generator to ClusterHandler and
replace the direct uuid.New() call in the create flow with it. Extend
TestClusterHandler_Create_Hash4NoCollision with deterministic generator values,
then add coverage for one hash4 collision followed by a unique ID and for five
consecutive collisions, asserting the latter returns CLUSTERS-MGMT-CREATE-007.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 84f23557-b47f-4b05-8e98-31aa90968ce1
📒 Files selected for processing (5)
hyperfleet-operator/docs/cluster-controller.mdplatform-api/pkg/clients/hyperfleetdb/client.goplatform-api/pkg/clients/hyperfleetdb/client_test.goplatform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/cluster_test.go
typeid
force-pushed
the
avoid_dns_collision
branch
from
August 11, 2026 08:18
4ffe943 to
3e52602
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/pkg/handlers/cluster_test.go`:
- Around line 604-680: Add a coordinated concurrent-create test around
ClusterHandler.Create that submits same-name requests producing the same hash4
and verifies only one succeeds while the other receives the established conflict
response. Enforce this through an atomic shared-store reservation or datastore
unique constraint in the create path, using the persistent store as the source
of truth rather than a process-local lock; preserve the existing retry behavior
for collisions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e36e7a73-898a-4057-9764-c582a369d2e0
📒 Files selected for processing (2)
platform-api/pkg/handlers/cluster.goplatform-api/pkg/handlers/cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- platform-api/pkg/handlers/cluster.go
typeid
force-pushed
the
avoid_dns_collision
branch
from
August 11, 2026 08:40
3e52602 to
1bf4bd2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sql`:
- Around line 6-10: Update the index definition in the migration by changing
CREATE UNIQUE INDEX to CREATE UNIQUE INDEX CONCURRENTLY for
idx_cluster_name_hash4, preserving the existing table, expressions, and WHERE
conditions.
In `@platform-api/pkg/handlers/cluster_test.go`:
- Around line 593-596: Handle all serialization and deserialization errors in
the cluster handler tests: check json.Marshal at
platform-api/pkg/handlers/cluster_test.go lines 593-596, 623-626, 665-668, and
742-745, and check response decode errors at lines 638-640 and 680-683. Fail
each test immediately when an error occurs, without ignoring any error returns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ee39efc-aaff-4f9d-bc5f-4e15f633617a
📒 Files selected for processing (3)
hyperfleet-db/internal/schema/migrations/002_cluster_dns_uniqueness.sqlhyperfleet-db/internal/schema/schema.goplatform-api/pkg/handlers/cluster_test.go
typeid
force-pushed
the
avoid_dns_collision
branch
from
August 11, 2026 09:14
1bf4bd2 to
6be81a0
Compare
Add a cross-account collision check in the platform-api Create handler that queries all clusters sharing the same name and verifies no existing cluster has a matching 4-character UUID prefix. On collision, the UUID is regenerated (up to 5 attempts). UUID generation is injectable for deterministic testing of collision and retry paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
typeid
force-pushed
the
avoid_dns_collision
branch
from
August 11, 2026 09:29
6be81a0 to
c9df088
Compare
Collaborator
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, typeid The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit Aug 11, 2026
0e12a93
into
openshift-online:main
13 checks passed
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
3 participants
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.
Summary
ListClustersByNameto the platform-api's hyperfleetdb client wrapper to query clusters by name across all accounts (uses field selector pushdown to SQL)HasHash4Collisionhelper to check whether any existing cluster shares a 4-char UUID prefixTest plan
TestClient_ListClustersByName— cross-account listing by nameTestHasHash4Collision— table-driven tests for collision detection (match, no match, empty list, short/empty InternalID)TestClusterHandler_Create_Hash4NoCollision— handler-level test with pre-existing clustermetadata.namemake test-api— all tests passmake lint— 0 issues🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation