Repository navigation
fix(store): find replicas and snapshots in any name spelling, store layer stacks in canonical case - #217
fix(store): find replicas and snapshots in any name spelling, store layer stacks in canonical case#217Andrei Kvapil (kvaps) wants to merge 48 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 124 files, which is 24 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.
 |
| Layer / File(s) | Summary |
|---|---|
Kubernetes storage and spelling resolution pkg/store/k8s/*, pkg/store/inmemory_*, pkg/store/store.go, pkg/store/storetest/*, pkg/linstormigrate/*, docs/linstor-migration.md, site/content/en/docs/migration.md |
The Kubernetes stores resolve alternate parent-name spellings, save normalized CR names in resource and snapshot specs, and retain registered spellings for wire conversion. LINSTOR conversion and migration guidance use normalized names. |
Controller replica and node matching internal/controller/*, pkg/satellite/controllers/* |
Controllers match resources, definitions, nodes, and snapshots across accepted spellings. Satellite CR-name resolution helpers are used by replica and snapshot operations. |
Placement, REST, and dispatcher matching pkg/placer/*, pkg/rest/*, pkg/dispatcher/*, internal/cli/*, pkg/validate/*, pkg/satellite/reconciler.go, tests/integration/* |
Name comparisons and lookup keys are normalized across placement, REST, dispatcher, CLI, validation, and satellite rendering. Added tests cover spelling variants and deletion with dependents. |
Canonical Layer Stacks
| Layer / File(s) | Summary |
|---|---|
Layer-stack canonicalization and validation pkg/validate/layers.go, pkg/rest/layer_validation.go, pkg/rest/resource_definitions.go, pkg/rest/resource_groups.go, pkg/rest/autoplace.go, internal/cli/write_more.go, pkg/satellite/controllers/resource.go, pkg/rest/layer_stack_case_test.go, internal/cli/layer_list_case_test.go, pkg/satellite/controllers/rd_needs_drbd_case_test.go |
Layer-stack validation returns canonical layer names, and REST and CLI paths store canonical stacks. Resource-group rebalance checks ignore case-only differences. Tests cover canonical storage and case-insensitive DRBD detection. |
Estimated code review effort: 5 (Critical) | ~90 minutes
Change: Bug fix · Severity of issue fixed: High
Suggested reviewers: lexfrei
Merge Risk: 
67e44
-
Resolve the placement identity mismatch before merging: it can bypass provider and shared-storage restrictions. Correct the migration naming guidance and keep distinct definitions isolated in the in-memory store.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title accurately summarizes the two main changes: spelling-insensitive replica and snapshot lookup, and canonical layer-stack storage. Linked Issues check Directly linked issues: #194,#204,#212. No contextual or parent issues apply.
[#194,#212] Child-resource listings. At head,resources.ListByNoderesolves the node spellings, then queries each sp…Out of Scope Changes check Most changed files apply the same case-insensitive name handling that the three linked issues require. The changes fall into four groups:
1. Readers that compare storedspecnames exactly. The stor…Docstring Coverage Docstring coverage is 88.99% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Full details: Linked Issues check
-
Explanation
Directly linked issues:
#194,#204,#212. No contextual or parent issues apply.[
#194,#212] Child-resource listings. At head,resources.ListByNoderesolves the node spellings, then queries each spelling throughlistUnderSpellingsonspec.nodeName.ListByDefinitionuseslistByDefinitionThrough, which applies the same spelling set. The summary states thatsnapshots.ListByDefinitionresolves all accepted RD spellings, queries each one, and falls back to an exhaustive filter that matches any spelling.Createstores the CR names of the definition and node inspecand labels, and keeps the registered spellings in annotations.crdToWireResourcereturns the registered spellings to clients. The envtestTestRDDeleteInAnotherCaseSeesSnapshotsAndReplicasand the conformance subtestsChildrenOfDefinitionUnderAnySpellingandListByNodeUnderAnySpellingcover the delete gate and the node-scoped listing. The in-memory store folds comparisons to match.[
#204] Layer-stack canonicalization. The summary states thatrd create,rg create,rg modify, autoplace, resource create, and make-available store the canonical stack throughcanonicalizeLayerStackandstoredLayerStack. The satelliterdNeedsDRBDcheck usesapiv1.LayerInStack, which folds case. TestsTestRDCreateStoresLayerStackInCanonicalCase,TestRGCreateAndModifyStoreLayerStackInCanonicalCase,TestCSILayerListIsStoredInCanonicalCase, andTestRDNeedsDRBDIgnoresCasecover these paths.Residual limit, not named by the issues: rows written before this change in a fourth spelling (neither the asked, folded, nor registered spelling) are not found. The PR states this and states that no migration is included. The
#194reproduction (PVC-Xoverpvc-x) is covered for rows written by the fixed code.
Full details: Out of Scope Changes check
-
Explanation
Most changed files apply the same case-insensitive name handling that the three linked issues require. The changes fall into four groups:
-
Readers that compare stored
specnames exactly. The store now writes CR names intospec(for exampleResource.Spec.NodeNameandSpec.ResourceDefinitionName). Controllers, satellite, dispatcher, and placer must match those CR names. Without these changes, a mixed-case definition would stop matching its replicas. This is a direct consequence of the#194design. -
Node-name comparisons of the same class as
#212(SameName,isNode,sameNode,sameDefinition,nodeKey), including REST evacuation, error-report filters, pool lookups, and the physical-deviceListForNodequery. -
Layer-stack handling for
#204, including the rebalance comparison inrgNeedsRebalance. -
linstor-migrate(pkg/linstormigrate/convert.go) and its documentation. These write parent CR names so that adopted children match the store's new layout.
Tests accompany the production changes. The PR objectives name the same areas (replica-event enqueueing, snapshot and node checks, pool and peer lookups, evacuation, projection, migration). Some changes (for example
waitForPeerDeletionAcks,validate.RestoreNodesMissingSnapshot) reach beyond the three issues' named call paths. They apply the same normalization, and the PR does not contain an unrelated feature or refactor. A separate peer-deletion acknowledgment issue is explicitly left for follow-up and is not part of this PR's code.No change was demonstrated to be unrelated to the linked issues.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 88.99% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 109 functions across 50 files. (44 skipped: 2 unsupported, 42 over the file limit.)
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
6aef318 to
bf57f52
Compare
ac8fceb to
6e36a49
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. The store listings work, but the new write spelling moves replicas of a mixed-case definition out of the controller's view, and three new test files have no license header.
Business context: a definition's replicas and snapshots must be found in any spelling of its name, or rd d passes the snapshot gate on zero rows and leaves replicas with live DRBD behind; layer stacks are stored in canonical case so exact readers see DRBD.
I tested 6e36a49, which sits on current main, so the trial merge is the head itself. pkg/store/..., pkg/validate, internal/cli/... and pkg/satellite/controllers are green with envtest, and the two new conformance subtests run on both stores, not skip. TestRDDeleteInAnotherCaseSeesSnapshotsAndReplicas passes. pkg/rest fails only on server did not stop within 2s after cancel, the known harness flake. The red Integration tests check is TestGroupG/SnapCreateListDelete, which #219 and #220 hit in the same window, so it's not this diff. #200, #206 and #215 to #222 merge cleanly with this branch.
Every fix I reverted one by one turns a named test red: the spelling set, the registered spelling, the raw replica and snapshot writes, the in-memory folds, both layer-stack helpers, the rd create / rg create / rg modify write paths, the CLI parser and rdNeedsDRBD. Two reverts survive, see follow-ups 1 and 2.
I also tried to get two different definitions to share children. Name() hashes the original string when the lowercased name is not RFC 1123, so Pvc_X and pvc_x are two separate definitions. They can't merge here: the Resource and Snapshot CEL rule wants metadata.name to equal <rd>.<node> case-insensitively, so names like that never get children at all.
Blockers
B1: the controller stops seeing replicas written under the registered spelling
pkg/store/k8s/resources.go:184
The controller doesn't use the store listings. It lists every Resource and compares spec.resourceDefinitionName exactly with rd.Name, which is the folded metadata.name. Three places do this:
ensureTiebreakervialistReplicasDirect(ctx, rd.Name)(internal/controller/resourcedefinition_controller.go:221, compare at:686), so the diskful and witness count, witness create and reap, quorumstampResizePending(internal/controller/resourcedefinition_controller.go:1658)hostingNodesForRD(ctx, rd.Name)(internal/controller/resource_controller.go:1550, compare at:1764)
Before this PR createOneResource set res.Name from the URL (pkg/rest/autoplace.go:1506) and the store wrote it as is. So r c pvc-mix n1 over a definition registered as Pvc-Mix stored pvc-mix, the slug, and the controller counted it. Now the same call stores Pvc-Mix and none of the three places sees it. The tiebreaker logic counts zero replicas for that definition and resize-pending is never stamped. Reverting line 184 turns ChildrenOfDefinitionUnderAnySpelling red, so the write really lands on the registered spelling.
This path worked before. And because the registered spelling is now the stored form, every mixed-case definition's replicas end up outside these lists. Comparing with store.FoldName on both sides there would fix it, plus a controller test on a definition registered in mixed case.
The satellite has the same shape for nodes at pkg/satellite/controllers/resource.go:164. A node registered as Node-1 with a satellite running NODE_NAME=node-1 used to apply r c pvc node-1 and now skips it, because the replica is stored as Node-1. Autoplace already wrote the registered spelling there, so that setup was half broken before. I'd still fold that compare in the same change.
B2: new test files without the SPDX header
internal/cli/layer_list_case_test.go:1, pkg/rest/layer_stack_case_test.go:1, pkg/satellite/controllers/rd_needs_drbd_case_test.go:1
AGENTS.md asks for // SPDX-License-Identifier: Apache-2.0 on every blockstor-authored Go file. On main the only Go file without it is the generated zz_generated.deepcopy.go. These three start right at package.
Non-blocking follow-ups
- Nothing pins the folded spelling in
spellingSet(pkg/store/k8s/spellings.go:116). Withoutadd(store.FoldName(given))every new test still passes, the envtest one too, becauseCreatealways writes the registered spelling. It has a real writer: the autosnapshot controller creates snapshots withResourceDefinitionName: rd.Name(internal/controller/autosnapshot_controller.go:298). A conformance case that writes a child CRD directly under the slug next to a mixed-case definition would pin it. - Nothing pins the spelling filter in the exhaustive fallback either (
pkg/store/k8s/snapshots.go:562). With== rdNamethere the package stays green, because envtest always serves the field selector and the fallback never runs. rgNeedsRebalancecompares stacks withslices.Equal(pkg/rest/resource_groups.go:416). An RG stored asdrbd,storagebefore this change gets one extra rebalance of all its children on the firstrg modifythat sends the same stack, since the patch now comes in canonical. Harmless, but a case-insensitive set compare would avoid it.- Rows already stored in a fourth spelling, like a pre-PR
r c Pvc-Xoverpvc-xor a hand-applied CRD, stay invisible to the listings and so to therd dsnapshot gate. The body says so, and I read that as the accepted limit. If it ever needs closing, the CEL rule already guaranteesmetadata.nameis the folded<rd>.<node>, which a field index could use instead of the spec value. - "Stacked on #190" in the body is out of date: #190 is merged and the base is main.
| // The replica is stored under its definition's registered spelling, so | ||
| // a listing of the definition finds it whatever spelling it is asked in. | ||
| canonical := *in | ||
| canonical.Name = canonicalRDName(ctx, s.c, in.Name) |
There was a problem hiding this comment.
With this line r c pvc-mix n1 over a definition registered as Pvc-Mix stores Pvc-Mix instead of the slug. The controller's replica lists (listReplicasDirect, stampResizePending, hostingNodesForRD) compare spec.resourceDefinitionName exactly with rd.Name, so they stop seeing this replica. See B1.
| @@ -0,0 +1,22 @@ | |||
| package cli | |||
There was a problem hiding this comment.
This file needs // SPDX-License-Identifier: Apache-2.0 at the top, like every other Go file in the repo (see B2).
| @@ -0,0 +1,193 @@ | |||
| package rest | |||
There was a problem hiding this comment.
This file needs // SPDX-License-Identifier: Apache-2.0 at the top, like every other Go file in the repo (see B2).
| @@ -0,0 +1,25 @@ | |||
| package controllers | |||
There was a problem hiding this comment.
This file needs // SPDX-License-Identifier: Apache-2.0 at the top, like every other Go file in the repo (see B2).
|
Thanks, head f64cf3b. B1: every exact compare of a replica's definition name now folds both sides the way the store addresses the definition: the three you named, plus the sibling fan-out, the node-id scan, the snapshot timeout, and the replica-event mapper, which enqueued B2: the three files carry the license header. Follow-ups 1 to 3 are in: an envtest case writes a snapshot and a replica straight under the folded name next to a mixed-case definition, a fake-client case runs the scan fallback, and Locally the integration run failed only on |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The compares from B1 now fold, and the license headers and follow-ups 1 to 3 are in, each pinned by a test. But the controller and the satellite also look a replica's definition up by spec.resourceDefinitionName as stored. With the registered spelling there, a replica of a mixed-case definition is never applied, and the controller deletes it as an orphan.
I tested f64cf3b on a trial merge with main at 7bb6606, which now carries #206. pkg/store/..., pkg/validate, internal/cli, internal/controller and pkg/satellite/... are green with envtest, and pkg/rest fails only on the 2s shutdown flake. The two red checks are not this diff. The tiebreaker test TestGroupFRToggleDiskful2DisklessReapsTieBreaker failed the same way on three unrelated branches (runs 40139309268, 40136179262, 40125497272), and it uses the lowercase td-tb, where every new fold is a no-op. recovery-deleting-convert failed on #216's run 40477975069.
Last round, point by point. B1 as written (the compares) is fixed: reverting isReplicaOf, the enqueue fold or isNode turns its test red. B2 is fixed. Follow-ups 1, 2 and 3 are fixed, and each goes red with its guard reverted. 4 stays the documented limit. 5 is fixed in the body.
Blockers
B1: lookups keyed by the stored spelling miss the definition
pkg/satellite/controllers/resource.go:440, internal/controller/resource_controller.go:285
Every replica is now stored under its definition's registered spelling, Pvc-Mix, and the CR is pvc-mix. Two Gets use the spec value as the CR key:
- The satellite's
runApplydoesr.Get(ctx, client.ObjectKey{Name: res.Spec.ResourceDefinitionName}, &rd). On NotFound it returnsctrl.Result{}, nil, without a requeue, so the replica is never applied. - The controller's
handleOrphandoes the same Get, reads NotFound as "parent deleted" and callsDeleteonceresourceWasAppliedis true. The satellite makes it true by stamping its finalizer before it reachesrunApply.
I checked both with a fake client: RD pvc-mix, a replica spelled Pvc-Mix with the satellite finalizer. handleOrphan returns orphaned=true and the replica gets a deletion timestamp. The satellite's Reconcile returns an empty result at that Get, while the same replica spelled pvc-mix goes on into prepareDRBDApply. Before this PR, r c pvc-mix n1 over Pvc-Mix stored pvc-mix (res.Name = rdName from the URL) and worked. Now every replica of a mixed-case definition takes this path, autoplace included. lookupVolumeNumbers (resource.go:1541) has the same Get on the delete path.
The satellite also still compares the definition name exactly. enqueueResourcesForRD (resource.go:422) compares with rd.Name, so an RD edit such as a resize never reaches a local Pvc-Mix replica. listPeerResources (:1164), adoptAuthoritativeFlags (:1195) and enqueueLocalSiblings (:1041) split a definition whose replicas carry two spellings, such as a pre-PR pvc-mix row next to a new Pvc-Mix one.
Main now has one more of these. #206 landed after this head and adds rdHoldsData (internal/controller/resourcedefinition_layer_stack.go:228) with the same exact compare. It returns false for a Pvc-Mix replica with MetadataCreated true. Today that is hidden only because such a replica never reaches disk. Once the Get is fixed, the pin's "already holds data" check would miss it.
Names on disk would also change. Once applied, the satellite names the DRBD resource and the volumes from the spec value (pkg/dispatcher/dispatcher.go:267, then <pool>/<name>_00000). A Pvc-Mix replica would get Pvc-Mix_00000, while #205 documents lowercase names on disk and a pre-PR peer has pvc-mix_00000.
Since this many readers key on the CR name, I'd store the CR name (k8s.Name) in spec.resourceDefinitionName, which the listing already asks under, and map back to the registered spelling only for display. Folding every reader also works, but the list above is what it takes, and the b618d42 message ("Every such compare now folds both sides") has to be true afterwards. Either way I'd want a test that runs a mixed-case replica through handleOrphan and the satellite's apply entry.
Non-blocking follow-ups
- Nothing pins
sameDefinition(internal/controller/replica_of.go:38). Witha == bthereinternal/controllerstays green with envtest. It feeds the peer list inrunApply,collectTakenNodeIDs,requestsForRDandsiblingReplicasUpToDate. - Node names have the same shape on the controller side.
takenPortsOnNode(resource_controller.go:1859) and the projection'senqueueResourcesForNode(resource_state_projection_controller.go:271) compare exactly, and the projection Gets the Node byspec.nodeName(:122), so a replica stored asNode-1is never projected.r c pvc node-1overNode-1used to storenode-1. - The f64cf3b body ends with "The new test files carry the license header.", which describes a fix to earlier commits of this PR. With squash it lands in main and means nothing there.
- This conflicts with #200 in
pkg/store/k8s/snapshots.go.
| // The replica is stored under its definition's registered spelling, so | ||
| // a listing of the definition finds it whatever spelling it is asked in. | ||
| canonical := *in | ||
| canonical.Name = canonicalRDName(ctx, s.c, in.Name) |
There was a problem hiding this comment.
With this spelling in spec, the satellite's runApply Get (pkg/satellite/controllers/resource.go:440) and the controller's handleOrphan Get (internal/controller/resource_controller.go:285) both answer NotFound for a Pvc-Mix replica. The satellite never applies it, and the controller deletes it as an orphan once the finalizer is on. See B1.
| @@ -420,7 +420,7 @@ func (r *ResourceReconciler) enqueueResourcesForRD(ctx context.Context, obj clie | |||
| for i := range localList.Items { | |||
| local := &localList.Items[i] | |||
| if local.Spec.ResourceDefinitionName == rd.Name && | |||
There was a problem hiding this comment.
rd.Name is the CR name, pvc-mix, and a replica written now carries Pvc-Mix, so an RD edit never enqueues the local replica. This compare needs the same fold as the node one below it.
f64cf3b to
8423f5d
Compare
|
Thanks, head 8423f5d, rebased on main with #206 and #219. B1: went with your suggestion. Pinned by Follow-ups: 1 is
|
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. Storing the parents' CR names in spec fixes B1: the orphan check and the satellite apply now find a mixed-case definition, and the DRBD and volume names stay lowercase. But one reader outside the diff depended on the old spelling. The dispatcher finds a peer's address by the node's registered name, so a peer on a node registered as Node-2 now gets 0.0.0.0.
I tested 8423f5d. The branch already sits on main at ffd7b90, so the head is the trial merge. pkg/store/..., pkg/validate, internal/cli/..., internal/controller, pkg/satellite/..., pkg/linstormigrate and pkg/dispatcher are green with envtest, and pkg/rest fails only on the 2s shutdown flake (TestBug145ResourceCreateIsAtomic). The red E2E lane 3 is recovery-deleting-convert (exit 2). It failed the same way on run 40477975069 for fix/create-spawn-prop-fields, and the scenario uses only lowercase names, where every new fold is a no-op.
Last round, point by point. B1 is fixed for every row the store and the migrator write: putting the display spelling back into spec turns TestOrphanCheckFindsTheMixedCaseDefinition, TestTakenPortsSeeAReplicaCreatedInAnotherNodeSpelling and TestProjectionFollowsTheNodeOfAMixedCaseReplica red. The satellite's exact compares now meet the CR name they expect, and the lowercase DRBD names are pinned. Follow-up 1 is fixed: a == b in sameDefinition reddens TestSameDefinitionFoldsCase. 2 is fixed with tests, 3 went away with the rewrite, and 4 still stands: this conflicts with #200 in pkg/store/k8s/snapshots.go.
Blocker
B1: a peer on a node registered in another spelling loses its address
pkg/dispatcher/dispatcher.go:628, :644, :1055, :611
lookupPrefNic, lookupNetInterfaceAddress and lookupEndpoint match the node name from the replica's spec against k8s.OriginalName of the Node. The comment on lookupEndpoint says this is on purpose, so that non-RFC1123 names resolve. That held while spec carried the registered spelling. Now it carries the CR name, and the match fails for every node whose registered name differs from its CR name. The pool loop at :611 compares the pool's spec.nodeName exactly, so a PrefNic on the pool is missed too.
I ran BuildDesired over two replicas created through the store, with a SatelliteEndpoint on each node:
| registered node | main | this PR |
|---|---|---|
node-2 |
peer.node-2.address = 192.0.2.2 |
same |
Node-2 |
peer.Node-2.address = 192.0.2.2 |
peer.node-2.address = 0.0.0.0 |
my_node |
peer.my_node.address = 192.0.2.2 |
peer.e74a1c5e-my-node.address = 0.0.0.0 |
0.0.0.0 is the placeholder the satellite replaces only for its own address. A peer's address goes into the .res as is (pkg/satellite/reconciler.go:5610), so the connection points nowhere. Comparing k8s.Name(nodeName) with nodes[i].Name, and folding the pool compare the way isNode does, would match both old and new rows. TestMixedCaseReplicaKeepsLowercaseNamesOnDisk passes nil nodes to BuildDesired, so it can't catch this; a test with a peer on Node-2 would.
Non-blocking
rdHoldsData(internal/controller/resourcedefinition_layer_stack.go:228, from #206) is the one controller compare of a definition name that stays exact, while 8e6e4b2 folds the others for rows adopted or written before. A replica spelledPvc-MixwithMetadataCreatedtrue and noInitializedlatch makesstackToPinreturn the group's stack; spelledpvc-mix, the default. I don't see such a row coming up in practice, since an RD with adopted data is latched, butisReplicaOfthere is one line.- 8e6e4b2 says "a replica adopted from LINSTOR keeps the display spellings", and so do the comments on
isReplicaOf,isNodeandTestSameDefinitionFoldsCase. After 8423f5d that holds only for replicas adopted before this change, and both messages land in main with the squash. - Two guards are not pinned. Dropping the two new keys from
mergeUserAnnotationsIntoinUpdatestays green, because the test sends nil annotations and the merge returns early; a REST update carrying any annotation would lose the spelling without them. Droppingadd(found.display)fromspellingSetalso stays green, though a826b50 names the registered spelling as what still finds older rows. - The PR body still says the store "stores new replicas under their definition's and their node's registered spelling". The commits now do the reverse.
- A name outside RFC 1123 that
linstorNameaccepts, likemy_res, now gets the hashed slug in spec, and so in the DRBD and volume names. Such definitions were never applied before, so nothing regresses, butdocs/linstor-migration.md:48and the migration page in #205 say "lowercase". - #221 moves the
blockstor.io/keys toblockstor.org/; the two annotations added here need to go with them.
|
|
||
| crd := wireToCRDResource(&shown) | ||
| crd.Spec.ResourceDefinitionName = rd.crName | ||
| crd.Spec.NodeName = node.crName |
There was a problem hiding this comment.
This is where the node's CR name goes into spec. The dispatcher still matches a peer's Node by its registered spelling (pkg/dispatcher/dispatcher.go:628, :644, :1055), so a peer on Node-2 or my_node now renders with address 0.0.0.0. See B1.
8423f5d to
9d15b16
Compare
|
Rebased on main after #200, head 9d15b16. The only conflict was the import block in |
9d15b16 to
0158d61
Compare
|
Thanks, head 0158d61, on main after #200. B1: the dispatcher's endpoint, PrefNic and NetInterface lookups now match Non-blocking:
|
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The dispatcher now finds a peer's node under both spellings, so last round's blocker is fixed. The same change reaches four more readers, though. Each of them compares a replica's spec name, now the CR name, with a name in the registered spelling, and none of them is in the diff.
I tested 0158d61. It sits on main at 536e935, and git merge-tree with main gives the head's own tree, so the head is the trial merge and the #200 conflict is gone. internal/cli/..., internal/controller, pkg/dispatcher, pkg/linstormigrate, pkg/satellite/controllers, pkg/store/... and pkg/validate are green with envtest. pkg/rest fails only with "server did not stop within 2s after cancel". go vet is clean. In CI only Integration tests is red, on two tests. TestGroupG/SnapCreateListDelete fails the same way on fix/unclean-al-apply (run 41438685172). TestGroupKWFPoolDestroyedDropsFromPlacer fails with the same line 473 message on feat/site 2a29a55 (run 41437867460), and that branch changes no placer code.
Last round, point by point:
- B1 is fixed. Going back to an exact
OriginalNamecompare inisNodeCalledgives0.0.0.0in both new dispatcher tests, anda == binsameNodeNameturns the PrefNic test red. - Follow-up 1 is fixed. An exact compare in
rdHoldsDatamakes thePvc-Pinsubtest pin the group's stack. - Follow-ups 2, 4 and 5 are fixed.
35d21682-my-resis whatk8s.Name("my_res")returns. - Follow-up 3 is fixed. Dropping the two keys from the resource Update merge reddens
TestReplicaStoresTheCRNamesOfItsParents, dropping the RD key from the snapshot merge reddensTestSnapshotStoresTheCRNameOfItsDefinition, and droppingadd(found.display)reddensTestARowStoredUnderTheRegisteredSpellingIsFound. - Follow-up 6 is still open. #221 merges #217 as of 6e36a49, so the two new annotations are not in it yet.
Blocker
B1: four controller readers still compare the CR name with the registered spelling
The REST surface shows a replica's node in the registered spelling. Snapshot.Spec.Nodes and the wire listings carry that spelling, and so does a storage pool. A replica's spec now carries the CR name. I checked each place below with a fake client stand built by mixedCaseStand, a node registered as Node-1:
| place | what I ran | result |
|---|---|---|
snapshot_timeout.go:339 |
replica created through the store on Node-1, disk state Outdated, Spec.Nodes from the wire listing |
wire Node-1, spec node-1, siblingReplicasUpToDate returns true |
resource_controller.go:486 |
pool created through the store on Node-1, firstAvailablePool with the replica's spec node |
"" |
resource_controller.go:1859 |
older replica with nodeName: Node-1 and port 7001, as main writes it for r c pvc-old Node-1, new replica on Node-1 |
takenPortsOnNode returns [] |
node_controller.go:122 |
replicas created as node-1 and as Node-1 |
both list as Node-1, node.Name is node-1 |
The first one matters most. That gate is the one that refuses to snapshot a replica that is not UpToDate (snapshot_controller.go:305 and :459). On a node registered as Node-1 or my_node it now skips every replica, so the snapshot goes ahead. The second leaves an in-use diskless replica on such a node diskless for good. The third hands a new replica a port that an older replica on the same node already holds. TestTakenPortsSeeAReplicaCreatedInAnotherNodeSpelling seeds the older row as node-1, a spelling that already matched. The fourth makes evacuate and lost on such a node migrate nothing. On main a replica created as node-1 matched there.
Comparing k8s.Name of both sides, the way isNode does on the satellite, would cover all four, old rows included.
Non-blocking
- Nothing pins the second clause of
isNodeCalled. Without|| strings.EqualFold(k8s.OriginalName(...), name)both dispatcher tests stay green, becausek8s.Namealready maps every spelling in the table to the CR name. The clause only matters for a case variant of a name outside RFC 1123:k8s.Name("my_node")ise74a1c5e-my-node,k8s.Name("My_Node")isfefe7665-my-node. A case with a peer stored asMy_Nodeon a node registered asmy_nodewould pin it. - Outside this diff: since that hash is case-sensitive,
My_Resandmy_resbecome two CRs, while the body opens with "the store addresses one object through the folded CRD slug".
0158d61 to
bc5dc14
Compare
|
Thanks, head bc5dc14, rebased on main with #222 and #224. B1: the controller has one
Other compares I went through and left: the REST and CLI ones ( Non-blocking 1: a peer stored as Non-blocking 2: right, the hash keeps case, so The two annotations will move to |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The five readers from the last round are fixed and pinned. A sweep of the rest of the tree still finds readers that compare a node name exactly, and two of the paths this commit fixes now reach the replica but miss it one step later.
I tested bc5dc14. It sits on main at e49b274, with #222 and #224, and git merge-tree with main gives the head's own tree. internal/cli/..., internal/controller, pkg/dispatcher, pkg/linstormigrate, pkg/satellite/..., pkg/store/... and pkg/validate are green with envtest. pkg/rest fails only with "server did not stop within 2s after cancel", and go vet is clean. In CI the six E2E lanes, Lint, Unit and Contract tests are green. Integration tests is red on TestGroupFRToggleDiskful2DisklessReapsTieBreaker, "diskful replica count never reached 2" at group_f_test.go:286. That test did not fail on another branch in the last 60 runs. The same helper times out with the same message in TestGroupFRAutoPlace2ReachesUpToDate on fix/luks-passphrase-secret-only 71cc110 (run 41509123263) and in TestGroupFRAutoPlace3WithTieBreaker on feat/site 2a29a55 (run 41437867460), so I don't count it against this PR.
Last round, point by point:
- B1 is fixed, for the four places I named and the fifth you found. Reverting each call site to the exact compare reddens its new test, and dropping either half of
sameNodereddensTestSameNodeFoldsCase. The ports test now seeds the old row asNode-1. - Non-blocking 1 is fixed. Without the
EqualFoldclause ofisNodeCalledthe newMy_Nodecase fails. - Non-blocking 2 is fixed. The body now says
My_Resandmy_resare two objects.
Blocker
B1: some readers still compare a node name exactly
The setup is the one isNode describes: a node registered as Node-1 whose satellite runs as node-1. I ran each row on the head with a fake client stand, mixedCaseStand or the same objects built by hand.
| place | what I ran | result |
|---|---|---|
node_controller.go:451, resourceCRDName is rd + "." + node, used by pruneSource and replicaUpToDate |
replica created as node-1 and as Node-1, victim taken from Store.Resources().List() the way migrateResource gets it |
victim is Pvc-Mix/Node-1, the Get is NotFound, pruneSource returns nil and deletes nothing, replicaUpToDate is false |
resource_state_projection_controller.go:122, Get of the node by res.Spec.NodeName |
the old row from TestProjectionReachesAReplicaStoredInTheRegisteredSpelling, node offline, Reconcile on it |
no status patch; a row with node-1 in spec gets Unknown |
satellite snapshot.go:90 and :740, controller allTargetsMatch |
replica created through the store as node-1, ListByDefinition |
wire node is Node-1 here and node-1 on main |
pkg/rest/node_lifecycle.go:137, the evacuate in-use guard |
in-use replica created as node-1, POST /v1/nodes/{node}/evacuate |
node-1 gets 200 and EVICTED, Node-1 gets 409; main is the other way round |
pkg/store/k8s/storage_pools.go:461 and :482 |
pool created on node-1, StoragePools().Get("Node-1", "pool") |
not found |
The first row is the evacuate and lost path this commit fixes. The node controller now reaches every replica on the node. With LOST the source replica is still never deleted, and with EVICTED the replacement never counts as UpToDate, so the drain never ends. TestEvacuationReachesReplicasInEverySpellingOfTheNode stops at the no-group annotation and never gets to pruneSource. Building the name the way the store's resourceCRDName does would close it.
The second row is the same shape. The node event now enqueues the old row, and the reconcile then drops it. Its test asserts only the enqueue. k8s.Name(res.Spec.NodeName) in that Get closes it.
The third row breaks snapshots on such a node. CSI sends no nodes, so listDiskfulNodes fills Spec.Nodes from the wire listing. On main that list carried the spelling the replica was created with. Here it is always the registered one. The satellite compares it with its own NODE_NAME exactly and skips the snapshot, and the controller looks the per-node status up by the same value. Storing Spec.Nodes as CR names, like the replica spec, would fix both sides.
The last two were exact before too, and this PR only changes which spelling misses. The evacuate guard is what keeps an in-use volume from being drained, and now the Kubernetes node name is the spelling that bypasses it. Restore, autoplace and the placer pass the replica's wire node into the pool Get.
Non-blocking
sameNodeNamein the dispatcher is the same function assameNode, and nothing pins itsEqualFoldhalf. Replacing it witha == bleavespkg/dispatchergreen.- The site PR #205 conflicts with this one in step 5 of
site/content/en/docs/migration.md. It still says<definition name, lowercase>, which this PR makes untrue for names outside RFC 1123. #226 conflicts indocs/linstor-migration.md. #221 does not merge with main yet and still has the oldspellings.gowithout the two annotations. - One more exact compare I only read: the DRBD path lookup in
pkg/drbd/conffile.go:636. Path keys keep the operator's spelling and the local host is the satellite'sNODE_NAME, so it already missed on main for such a node. It needs the same fold.
What I checked and left alone. The dispatcher maps idOf, byName and peerByName, and the satellite's adoptAuthoritativeFlags and peersMissingNodeID, key and look up by spec values of one definition's replicas. The CRD admits one object per definition and node, so those agree. linstormigrate/convert.go compares rows of one LINSTOR dump with each other. handleOrphan, runApply, rdAllocMu and the other Gets by Spec.ResourceDefinitionName miss an old row stored as Pvc-Mix. On main the satellite's Get of Pvc-Mix was NotFound too, so such a replica was never applied and holds no data. The REST and CLI compares between two wire values from one listing agree with each other.
The snapshot success check and the restore pool map folded node names to lowercase. My_Node and my_node hash to two CR names and are two nodes, so a snapshot reported on one read as taken on both, and an entry reported under a node's CR name matched no registered node. They now key by the CR name, the way the store addresses a node. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…kful paths A replica names its node by the CR name or by the spelling it was registered under, while the node list shows the registered one. The disabled and known sets and the witness exclusions compared the two exactly, so a witness on an EVICTED node registered as Node-1 was read as live and never moved, and auto-diskful counted a replica on an evicted node. The direct node probe folded case instead and read My_Node and my_node as one node. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…y CR name The disabled-node set is keyed by CR name, but the additional replica count looked a replica's node up as the wire spells it. A replica on an EVICTED node registered as Node-1 was counted as live, so +1 asked for one replica too many. The error-report node filter folded case and merged My_Node and my_node; it now compares CR names too. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A replica written before the store kept CR names in spec names its definition Pvc-Mix. Now that the satellite finds such a definition, it laid the resource down under the raw spec value while snapshot requests, and every replica written since, use the CR name pvc-mix: the snapshot failed with "resource not known on this satellite". The intent, teardown, snapshot phases, orphan sweepers, the derived port and minor, the day0 generation check and a restore's source now all use the CR name. A row that already stores it is unaffected. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The kernel and the apply chain know a resource by its definition's CR name, a hash for a name outside RFC 1123, while the store names the replica's Resource CR, and a Snapshot CR, from the spellings the definition and the node were registered under. The observer, the SkipDisk clearer, the cross-node snapshot fetcher and the MetadataCreated and FilesystemFormatted stampers built the CR name from the kernel name, so for such a definition status, KernelLoaded, SkipDisk and the conditions were never written. They now map back through the definition and the node, the way the store named the CR. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
An object is stored under the CR name its LINSTOR name maps to: the lowercase name inside RFC 1123, a hash of the name as given otherwise. That function lived in the Kubernetes store, so code above the store reached for it, or folded case itself, which is not the same thing for a name outside RFC 1123. pkg/naming now holds the key and Same, and k8s.Name delegates to it. Key is also made idempotent on every key it returns: a name long enough to be cut could end on a dash, which no stored object can carry and which Key hashed a second time. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Every controller, REST handler, the satellite, the dispatcher, the placer and the CLI keyed and compared names through k8s.Name and k8s.SameName, reaching into the store backend for it. They now use naming.Key and naming.Same, the same function, so nothing changes in behaviour; the next change removes the per-site helpers and the case folding that sit beside them. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Code compared two spellings of a name as they were, or folded case where the store hashes, in more places than any single fix reached. They came from one habit: every site decided for itself how to compare a node, definition, group, pool or snapshot name. Above the store they are now compared with naming.Same and used as map keys through naming.Key, and nothing else. That removes the per-site helpers (sameNode, sameDefinition, sameNodeName, isNodeCalled, isNode, listsNode, nodeKey, nodeCRDMatchesWireName, k8s.SameName, store.FoldName) and fixes the sites that still mixed spellings, among them: - the evacuation drain target counted a replica on a LOST or EVICTED node when its row spelled the node differently from the node list; - the placer's min-rsc-count weight read a pool's node in the pool's spelling against counts keyed by the replicas' spelling; - list filters, restore markers, group and pool names folded case, which joins two definitions such as My_Res and my_res that the store keeps apart; the in-memory store did the same and now keys like the Kubernetes one. The dispatcher keys peers by node and still emits the spelling a peer's row stores, so no DRBD connection of a running resource is renamed. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A test walks the Go sources under pkg, internal and cmd and fails on case folding applied to an object name, on comparing a reference field or variable (a replica's node or definition, a definition's group, a pool, a snapshot's nodes) with another name as it is, on slices.Contains over one, and on a map keyed by one, unless naming.Key or naming.Same is on the path. Store backends, the LINSTOR dump converter and the kernel side are out of reach, each with its reason; the few sites that match and are right as they are carry theirs in the allowlist, which also fails when an entry goes stale. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Code that reads a definition or node by the name a row stores, or by the name a satellite was started with, passed that name straight to the API server as metadata.name. A row written in another spelling, or a satellite started as Node-1, then read nothing: the controller took a replica's definition as missing, and the heartbeat stamped no Node. These lookups now go through naming.Key, and the naming guard flags a reference used as the Name of an ObjectKey, NamespacedName or ObjectMeta. The satellite's own name is keyed only where it addresses its Node, so the name it renders into DRBD configuration is unchanged. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…ing key The satellite's cold start keyed the replicas it owns by the name the store shows, the registered spelling, and looked kernel resources up under their own name, the definition's key. A resource of a definition registered as PVC-X read as another DRBD user's and was left up. The CLI's pool filter compared names as given. Both go through naming now, and the naming guard reads a replica's Name as the reference it is. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The filter comments said "case-insensitive", which the filters still are for every name inside RFC 1123; outside it two case variants are two objects, and the filters now keep them apart. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The self-migration guard compared the two node arguments as given, so `r td node-1 pvc-x --migrate-from Node-1` passed it. The store resolves both to one replica, the command marks that replica as migrating from itself, and the reconcile then prunes the source: the data loss the guard exists to refuse. The ends are compared with naming.Same. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The list filter compared node names as given, and the pair key was built from the names as given, so `node-connection list node-1` missed a pair written for Node-1, and a write that spelled the node node-1 started a second pair beside it. Lookups now match a stored pair whose nodes are naming.Same in either order, which also finds pairs written before; a new pair keeps the caller's spellings, ordered by key. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The in-memory store kept definitions, groups, nodes, replicas, snapshots, pools and volume definitions under the name as given, while the Kubernetes store resolves any spelling to one CR. A test over the in-memory store could pass code that hands a non-canonical spelling to Get or Create. It now keys every map by naming.Key, and conformance cases pin the shared behaviour: an object created as pvc-x is found as Pvc-X and refuses a second create as Pvc-X, on both backends. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Such a name keyed to its hash followed by a dash, which is not a name the API server takes and which Key hashed a second time. The dash is left out when there is no slug. No stored object can carry the old form, so no existing name changes. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The placer looked an existing replica's pool up by the node as the replica shows it, in a map filled by the node as the pool listing shows it. When the two spellings differed the pool was not found, so a new replica could land on a pool sharing the existing replica's LUN, and the provider-kind constraint started from nothing. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Paths were stored under a prop named by the two nodes as the caller spelled them, so posting a path for Node-1/beta and another for node-1/beta opened two entries for one pair, and the dispatcher handed the satellite two connections between the same two hosts. A read or delete in the other spelling missed the stored paths. The pair's prop is now found by key, and a new one is ordered by key. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The wait asked for an annotation named after the removed node as the request spelled it, so an acknowledgment stamped for node-1 was never seen for Node-1 and the delete sat out the full timeout. An annotation key cannot carry every spelling, so the peer is named by its key. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Every caller passes the definition's CR name, for which this is the same comparison; it no longer depends on that. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The guard matched object names by what variables were called, so a raw comparison under any other name went through: migrate-disk's src == dst and the node-connection filter were two. It now type-checks the tree and follows a name from where it enters, the name fields of the wire and CRD types, the path parameters, query lists and CLI flags that carry names, and whatever is passed to the store as a name, through assignments, ranges, calls and returns, to the comparison, map key, search, case fold or CR address it reaches. A planted package checks every shape under arbitrary names, and the shapes it must leave alone. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The in-memory store keyed a replica or snapshot by the keys of its two names apart, while the Kubernetes store names the CR by the key of the two joined. Outside RFC 1123 those differ: the joined key hashes the whole, so a test over the in-memory store could pass code that the Kubernetes store would send to another object. The in-memory store now uses the CR name, a conformance case pins the RFC 1123 spellings on both stores, and an in-memory test pins the hashed case the CRDs keep off a cluster. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The guard saw a raw name comparison only in the shapes it knew: a key built by Sprintf or inline concatenation, a name copied through a struct field, a switch case, a closure, a call through an interface, a prefix test or a map literal went through. It now also follows names through Sprintf, Join and inline concatenation into map keys, string fields of local structs, closures bound to a variable or called in place, the function a slices.*Func call runs, methods of this module's interfaces, append, the keys of a map keyed by a name, and strings.Cut; and reports switch cases, map literal keys and prefix or substring tests between names. A parameter a helper returns stands for what each call passes, so a helper given a name taints only its own callers. Two reads of one field count as one source by where they come from, one collection or a sort's comparator, not by what the variables are called. The doc comment lists what it follows and what it cannot see. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A migration marker names its source node as given. A marker spelled Node-1 on the replica on node-1 names that replica itself, and once it reached UpToDate the controller deleted it as the source: the only copy the migration made. The controller now drops such a marker and leaves the replica, pruneSrc refuses to delete the replica it was handed as the destination, and the REST migrate-disk path refuses one node in two spellings before stamping anything. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Snapshot status can hold entries for one node under two spellings, Node-1 and node-1, written before node names were compared by key. The satellite updated the first of them while the controller, which reads one entry per node, kept the last, so a suspend acknowledgment the satellite wrote was never seen and the snapshot stalled. Every status write now folds the node's entries into one, keeping what any of them recorded. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The removed-peer diff compared the peers a .res file was rendered with against the peers the new intent names, both as written. A peer spelled Node-2 in the file and node-2 in the intent read as gone, and the satellite ran del-peer against a peer that is still there. Both sides are compared by key now; the names it returns are still the ones the kernel knows. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The renderer looked a pair's paths up by comparing the connection's node names with the host names exactly. The satellite maps the connection onto the hosts' spelling before rendering, so this held only as long as that mapping did; matching by key here keeps the paths of Node-1/beta on the host node-1 without depending on it. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Whole files and directories were out of the guard's reach, so a raw comparison in the satellite reconciler, between connection names and host names from another source, went unseen. Exclusions now name the single functions that need one, with the reason, and the intent and DRBD types are sources. The guard also reports a map entry deleted by a raw name, and treats a helper returning naming.Key's result and a CR's GetName() as keys, so comparing either with a raw name is reported. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
bbc8f78 to
f10a0d5
Compare
|
Fixed. The migration controller drops a marker that names the destination's own node and never prunes the destination, and snapshot status writes fold a node's entries into one, so the satellite and the controller read the same ack. The guard now checks the reconciler and the DRBD renderer per function instead of skipping files, and catches raw |
LINSTOR names are case-insensitive, and the store keeps an object under the CR name its LINSTOR name maps to: the lowercase name inside RFC 1123, a hash of the name as given otherwise. So
PVC-Xandpvc-xare one definition, andMy_Resandmy_resare two. A replica or snapshot names its definition and node in spec, though, and a field selector compares that value exactly, so a listing asked in another spelling came back empty. Onrd d PVC-Xoverpvc-xthe snapshot gate passed over snapshots it could not see, the cascade reaped nothing, and the folded delete took the definition and left its replicas and snapshots behind, with DRBD still up under them. The node-scoped listing had the same gap for node names (#212).The fix has two halves.
The store: it lists a definition's replicas and snapshots, and a node's replicas, under the given, the CR and the registered spelling. It stores new replicas and snapshots with their parents' CR names in spec and keeps the registered spellings in annotations that only the REST surface reads, so clients see the spelling they compare. It derives those annotations from the parents it resolved and never takes them from a caller. A parent it cannot read is an error, never "not there".
linstor-migratewrites the same shape.Everything above the store: one identity per object,
naming.Key, in a package of its own (pkg/naming). Controllers, the satellite, the dispatcher, the placer, the REST handlers and the CLI compare object names only withnaming.Sameand key maps only bynaming.Key; the per-site helpers and the case folding they replaced are gone. That covers what earlier fixes reached one place at a time, and a few places they had not reached yet: the evacuation drain target, the placer's min-rsc-count weight, the controller's read of a replica's definition, the satellite heartbeat and cold-start orphan set, the list filters and restore markers, which folded case and so joined two definitions such asMy_Resandmy_res,migrate-disk, which tookNode-1andnode-1for two nodes and so could migrate a replica onto itself and prune the source, thenode-connection listfilter, resource-connection paths, which opened a second entry for one pair, the placer's lookup of an existing replica's pool, and the peer-forget acknowledgment key (the wait that reads it has no caller yet). The migration controller drops a stored migration marker that names the destination's own node instead of pruning the destination, and RESTmigrate-diskrefuses one node in two spellings. A snapshot's status entries for one node in two spellings are folded into one on every write, so the satellite and the controller read the same acknowledgment. The satellite no longer takes a peer written in another spelling in its.resfile for a removed peer, and the DRBD renderer matches a connection's paths to its hosts by key. The in-memory store resolves every name through the key and keys replicas and snapshots by the same name the Kubernetes store gives their CRs.A test in
pkg/namingkeeps the class from coming back. It type-checks the tree and follows an object name from where it enters (the name fields of the wire and CRD types, the REST path and query parameters and CLI flags that carry names, and anything handed to the store as a name) through assignments, ranges, concatenation and formatting, struct fields, calls into functions, closures and interface methods, and returns, and fails where one reaches a comparison with another name, a switch case, a prefix test, a map key, a search, a case fold or a CR address withoutnaming.Keyornaming.Sameon the way. It goes by where a value comes from rather than what it is called; its doc comment lists what it follows and what it cannot see, and a planted package checks that it catches every shape under arbitrary variable names and leaves the correct ones alone. Reviewed sites where both sides come from one listing or one intent are allowlisted with the reason.Nothing on disk is renamed. The satellite names a resource, its volumes and snapshots by the definition's CR name, which for every row written before is the name already in use; rows in a mixed-case spelling were never applied before this. The dispatcher still emits a peer under the spelling its row stores, and the satellite renders its own name as configured; only the lookups key them.
Rows written before this in a fourth spelling, neither the asked, the CR nor the registered one, stay out of reach of the scoped listings. Nothing is deployed on such data yet, so there is no migration for it.
The layer stack had the same problem (#204):
rd create,rg create,rg modifyand the CLI stored a layer stack as the caller spelled it while some readers compare exactly, so the satellite controller readdrbd,storageas a stack without DRBD. Every write path now stores the canonical spelling, the autoplace paths that give a definition its first stack included, and the satellite check folds layer names for objects written before or applied by hand.Fixes #194
Fixes #204
Fixes #212
Testing
TestRDDeleteInAnotherCaseSeesSnapshotsAndReplicas(envtest) fails with the spelling set reverted and passes with it.pkg/naming: the key is idempotent and keeps case variants outside RFC 1123 apart; the guard reports all 28 planted shapes and none of the planted correct ones, fails on any new site or stale allowlist entry, turns red when any rule is switched off, and turns red when the migrate-disk, node-connection, placer or resource-connection fix is reverted.go test ./...with envtest assets: green apart from the knownpkg/restshutdown flake (test: make the REST and e2e harnesses reliable off the project's own stands #199).linstor:TestGroupG/SnapCreateListDeletefails 1 run in 3 on this branch and on main alike;ResourceConnectionPathCreatepasses 6 of 6.