Repository navigation
refactor(api)!: move the API group to blockstor.org - #221
Conversation
|
Important Review skippedToo many files! This PR contains 286 files, which is 186 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
 |
4d7fdd0 to
bf67ca6
Compare
3d93cb0 to
3125e54
Compare
The project stands on its own under the domain its site is published on, and the API group is the one name that cannot change cheaply once clusters carry objects under it. Nothing runs on it yet, so it moves now, without a migration. Every kind moves to blockstor.org/v1alpha1, and the label and annotation keys from blockstor.io/ to blockstor.org/. The seed of the stable node UUIDs keeps the old string: it is a hash input, and changing it would change every node UUID. BREAKING CHANGE: objects under blockstor.cozystack.io and keys under blockstor.io/ are no longer read; a cluster on an earlier build has to be reinstalled. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
3125e54 to
050284c
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. Please put the reinstall requirement in the site's upgrade instructions before the rollout steps. The page still tells existing installations to apply the new CRDs and roll the components, but the new controller and API server cannot read their old-group resource definitions and replicas. The PR and commit explain this boundary; the published procedure does not. The rolling-upgrade test restarts the same images, so its green result does not cover this transition.
The upgrade steps told existing installations to apply the new CRDs and roll the components, but the new release reads neither the old API group nor the old keys. The page now says so before the steps, and the notes on rolling across the port and TLS change, which no longer happens, are replaced by the reinstall. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Fixed in 660ec3b. The upgrade section now says before the steps that v0.1.17 or earlier is reinstalled, and the old note about rolling across the port and TLS change is replaced by the reinstall, since that rollout no longer happens. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) The reinstall instructions address my previous comment. Please investigate the failing integration test before merging. The current run times out in TestGroupKWFHappyPath waiting for Snapshot wf-happy.snap-1. I have not established whether the API-group change causes this failure.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
A mechanical, near-symmetric rename of the API group (blockstor.cozystack.io to blockstor.org) and the label/annotation keys (blockstor.io/ to blockstor.org/), +989/-992 across 286 files. I checked it for completeness and consistency rather than reading every file, and both hold.
What I checked
The rename is complete, and the only old-group strings left are the ones the PR keeps on purpose. A tree-wide grep for the old group leaves exactly seven hits: the dated audit note docs/zfs-thick-validation-2026-05-14.md (history), site/content/en/docs/operations.md:219 (it has to name the old group to describe the migration), and pkg/api/v1/node.go:218,226, the node-UUID hash seed, which is a hash input rather than the group and now carries a comment saying exactly that. The single residual blockstor.io/ is that same operations doc. Nothing dangling is left for a reader or a writer to miss.
The one functional coupling got handled, and actually tightened. pkg/rest/internal_annotations.go is the wire scrubber that keeps controller-private annotations off the REST boundary. Before this change its canonical prefix (blockstor.io/) and its per-bug suffix (.blockstor.cozystack.io/) sat on two different domains; the rename collapses both onto blockstor.org/ and .blockstor.org/. The matching logic is unchanged (HasPrefix for the canonical namespace, Index(...) > 0 for the <ns>.blockstor.org/ scratch pattern), so producer and consumer stay paired.
The only edit that is not a string swap is the leader-election lease, a3f89427.cozystack.io becoming a3f89427.blockstor.org in cmd/controller/main.go:176, a single reference with no stale name anywhere else. The rest outside the swap is gofmt realignment from the changed key lengths plus the one added seed comment.
CI is green on the reviewed head 660ec3b: Unit, Integration, Contract, every E2E lane, piraeus interop, lint, DCO and the license checks. The Contract and interop jobs are the independent signal that the wire contract survived the rename, not just my reading of it. origin/main is still at this PR's merge-base, so the tree CI built is the head itself.
On the integration test from the earlier round: TestGroupKWFHappyPath timed out waiting for wf-happy.snap-1 on an earlier attempt of run 43129625037, and attempt 2 of that same run, on the identical head SHA 660ec3b, finished green. A rename that broke the test would fail it on every attempt, not one in two. So it is a non-deterministic timing flake in the harness, not a regression from this change. Worth tracking on its own, but it does not gate the PR.
The breaking change (no migration, reinstall required) is stated in the PR, the commit, and now the operations docs, which is where the earlier round asked for it.
Notes (non-blocking)
pkg/rest/nodes_test.go:1326assertsStableNodeUUID("n1")against a fresh call to the same function, so it never pins the literal UUID the seed produces. The seed is load-bearing and declared immutable, so a golden test fixing the exact UUID string for a known node name would turn a future accidental seed change into a red test instead of a silent identity shift. Pre-existing, not introduced here.- The flaky
TestGroupKWFHappyPathis worth its own issue, so the timeout gets chased to a root cause instead of absorbed by re-runs.
Addressed in 660ec3b: the upgrade page now says v0.1.17 or earlier is reinstalled. The reviewer confirmed it closes the comment.
Moves the API group from
blockstor.cozystack.iotoblockstor.org, and the label and annotation keys fromblockstor.io/toblockstor.org/. The project is meant to stand on its own, under the domain it publishes its site on, and the group is the one name that cannot change cheaply once clusters carry objects under it. Nothing runs on it yet, so it moves now, with no migration.What moves:
blockstor.org/v1alpha1: the CRDs (regenerated, one file per kind under the new name), the RBAC markers and roles, the samples and the stand manifests;topology.blockstor.org/labels synced from Kubernetes nodes.The LINSTOR migrator stamps
apiVersionfrom the scheme, so the objects it writes come out under the new group; its golden manifests say so.Two strings keep the old spelling on purpose: the seed of the stable node UUIDs, which is a hash input, not the group, and must not change, and the history in
CHANGELOG.mdand dated audit notes.cozystack/cozystack#2942 follows the group in its chart and tests.
Breaking change
The API group is
blockstor.org, and so are the label and annotation keys. A cluster running a build from before this has to be reinstalled: objects underblockstor.cozystack.ioand keys underblockstor.io/are not read.Testing
go test ./...with envtest assets, including the migrator's golden manifests: green.make lint: clean.