Repository navigation
fix(layer-stack): decide a definition's layer stack once and never format over data - #206
Andrei Kvapil (kvaps) wants to merge 22 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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. Comment |
0473bdc to
53929ba
Compare
4ff20de to
b7c304e
Compare
5a8ad9f to
55de69b
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) One commit message needs fixing before this lands, the rest are suggestions. I ran the touched packages on the delta over #190 (53929ba) and broke each new guard in turn: the pin's data checks, both satellite gates, every blkid branch, the thick wipe and rename, both stores' rule that keeps a decided stack, the restore-marker set check and the migrate stack reader. Each break turned a test red. The k8s store one goes red only under envtest, which make test provides. CI is green on 55de69b.
The thick LV commit claims more than wipefs does. c461702 says that after the rename "nothing under a volume's name ever holds another volume's leftovers", and the PR body repeats it. wipefs --all erases the signatures libblkid recognises, and its man page says it does not erase the filesystem itself or any other data from the device. The old volume's blocks stay on those extents under the new name, and a plaintext volume that gets them can read them back from the raw device. The LUKS header part of the claim holds, because the header magic is gone. docs/layer-stack.md already words it right ("never inherits a signature"). Commit bodies land in main, so please narrow the commit message and the PR body to signatures. The other way is to zero the LV, which I assume is skipped because it is slow on a large thick LV.
Suggestions:
-
A definition in a group without a stack is never pinned. It stays undecided while its volume is built and used under the default, and if the group gains a stack later, the pin decides from rdHoldsData at that moment. Between the satellite starting the build and MetadataCreated being stamped, the pin can give it the group's stack over a volume that already has DRBD metadata. The blkid guard refuses then, so nothing is lost, but the replica is stuck. The refusal's hint also suggests wipefs, which here would wipe the volume's own metadata. Pinning the default at the same point when the group sets no stack, or there is no group, closes the window and makes "decided once" literal. Upstream writes the resolved stack when the resource is created, too.
-
The unapplied-group-layer-stack annotation is never removed. I checked with a fake client: a definition pinned to DRBD,STORAGE in a DRBD,LUKS,STORAGE group keeps the annotation after the group changes to DRBD,STORAGE and the definition is reconciled again. The kubectl recipe in docs/layer-stack.md then lists a volume that is no longer short of its group. Either re-evaluate it on reconcile, or say in the docs that it records the group's stack at pin time.
55de69b to
81ec822
Compare
|
Thanks, head 81ec822. The thick LV commit and the PR body now say signatures only, and name what stays on the extents. Zeroing is left out on purpose: it means writing a large thick LV in full before first use. 2: the annotation follows the group now. Each reconcile re-checks a flagged definition, updates the stack it names, and drops it once the group no longer asks for a missing layer or is gone. 1: agreed, pinning the default when the group sets no stack makes "decided once" literal. It touches every definition outside a group with a stack, so I'd do it in its own PR. Here the refusal hint no longer suggests wipefs for DRBD metadata on a volume already in use. |
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 new annotation refresh can lose the flag on a plaintext volume in a LUKS group for good, and I'd like that fixed before this lands. The rest of my last review is done or fine to defer: the thick LV commit and the PR body now say signatures only, the default pin can go in its own PR, and the refusal hint no longer points at wipefs for a volume's own DRBD metadata.
The refresh looks only at a definition that already carries the annotation, and now it can also remove it. Once it is removed, nothing puts it back. I ran this with the fake client. A DRBD,STORAGE definition is flagged in a DRBD,LUKS,STORAGE group. The group goes to DRBD,STORAGE, and the reconcile drops the annotation. The group goes back to DRBD,LUKS,STORAGE, and the reconcile leaves the definition unflagged, with no event. At 55de69b the annotation would have stayed. The "never flagged" case in TestPinnedRDsUnappliedGroupStackFollowsTheGroup pins exactly this end state as correct. docs/layer-stack.md says the mark follows the group's current stack and offers the kubectl recipe to find every RD short of a layer its group asks for, and after a flap the recipe misses this one.
The simplest fix I see is to make the check symmetric: for every definition with a decided stack in a group, set or clear the annotation from the group's current stack. That also covers a definition whose group gains a layer after the pin, and a prepared restore target whose group asks for more than the source has. Neither gets the annotation today. The controller reads the group from the cache as wired in cmd/controller/main.go, so doing this on every reconcile costs no apiserver call. If you'd rather keep the annotation as a record of the decision at pin or copy time, then the refresh should update its value and never delete it, and the docs should say that.
A smaller one on thick LVM. A .bsnew LV left by a crash between lvcreate and lvrename is removed only by DeleteVolume, and the sweeper never sees it, as the commit says. If the resource goes away some other way, the LV stays forever. A later CreateVolume under the same name also renames the leftover without checking its size, since lvcreate is skipped whenever the temporary name exists. Not blocking.
This time I re-read the whole delta over #190 (53929ba), not only what changed since my last review. Touched packages pass go vet and go test on 81ec822. The one red in pkg/rest, TestBug180SnapshotCreateRollsBackOnRDDeletedRace on its 2s shutdown wait, fails the same way on 53929ba. Making the refresh a no-op turns its test red. I also checked that the only builder of satellite intent goes through the new pin gate, and that the pin sees data on existing DRBD volumes through MetadataCreated.
|
Thanks, head 61d6be6. The check is symmetric now. Every definition with a decided stack in a group is checked against the group's current stack on each reconcile: flagged while it lacks a layer of it, with the same warning Event when the flag goes on, and cleared once it doesn't. That covers the flap you ran, a group that gains a layer after the pin, and a copy whose group asks for more than the source had. The "never flagged" case is gone; a definition that set its own stack short of its group is flagged too. Thick LVM: a leftover |
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 flap is fixed, but now that a definition whose stack was set by hand also gets flagged, the volume-less clone path still reads the flag the old way, and a clone can lose LUKS.
The refresh now sets and clears the annotation from the group's current stack, and TestUnappliedGroupStackSurvivesAGroupFlap runs the flap. With the old if !flagged { return nil } guard put back, that test and the "group gained a layer" case go red. Without the Event, the flap test goes red. The thick LVM leftover is now removed and created again at the current size, and skipping the lvremove turns TestThickCreateVolumeReplacesATemporaryLVACrashLeft red. The orphan .bsnew the sweeper never sees is fine as a follow-up.
A clone of a volume-less definition drops the source's stack whenever the source carries the flag. cloneShellLayerStack in pkg/rest/rd_clone.go returns nil for it, because the flag used to mean "pinned over data, not a choice". Since b6f01f1 it also means "set by hand and short of the group". I ran this with the fake client: a volume-less RD with LUKS,STORAGE in a group with DRBD,STORAGE gets flagged with DRBD,STORAGE and the LayerStackNotInherited warning. A clone of it then gets an empty stack, and the controller pins that from the group to DRBD,STORAGE, so the clone has no LUKS. At 81ec822 the same clone took LUKS,STORAGE. docs/layer-stack.md still says such a clone "takes the source's stack only when the source set one by choice", and the comment on UnappliedGroupLayerStackAnnotation still lists only the pinned-over-data and copy cases.
A stack the controller pins over data is always DRBD,STORAGE. So one fix is to drop the source's stack only when it is flagged and is the default stack. A separate marker for "pinned over data" works too. Either way the docs and the annotation comment should describe the wider meaning. The Event text "migrate the data to a new volume" also reads oddly on a definition that holds nothing and picked its own stack.
Two smaller things, not blocking:
- A change to a group does not re-queue its definitions. SetupWithManager watches RDs, Resource spec changes and Node drain flags, not ResourceGroups. After a group gains or drops a layer, the annotation catches up only when the RD is reconciled for another reason, or at the manager resync, which is 10 hours by default. A watch on ResourceGroup that enqueues the RDs in it, filtered to layer stack changes, would fix that.
- I was wrong last time when I said the group is read from the cache.
readGroupgoes throughdirectOrCached(), and SetupWithManager setsAPIReader, so in production this is a direct GET on every reconcile of every RD in a group. The reconcile already lists Resources directly, so the cost is probably fine. Still, the comment onrefreshUnappliedGroupStacksays cache: fix the comment, or read the group throughr.Client.
This PR and #222 now conflict in pkg/linstormigrate/convert.go and docs/linstor-migration.md. Both edit the same LUKS passphrase warning and doc paragraph, so the conflict is textual only. I took #222's side of both hunks. The combined tree builds, and the touched packages pass go vet and go test, apart from the pkg/rest 2s server-shutdown flake that fails the same way on 53929ba.
On 61d6be6 itself, go vet and go test pass for the touched packages, with the same pkg/rest flake. Integration is red on TestGroupFRAutoPlace2ReachesUpToDate and TestGroupG/SnapCreateListDelete, both 2m30s Eventually timeouts. The same timeouts show up on unrelated branches over the last two days, including TestGroupFRAutoPlace2ReachesUpToDate itself, so I don't think this diff causes them. The piraeus interop lane was still running when I checked.
|
Thanks, head 11c1de8. The volume-less clone now drops the source's stack only when the source is flagged and its stack is the default one, which is the only shape a pin over data has. A flagged Both smaller ones are in too: a watch on groups enqueues their definitions when the layer stack changes, and the refresh comment says the group is read directly. The conflict with #222 is the same LUKS warning text on both sides; I'll merge the wordings in whichever lands second. |
A definition that leaves its layer stack empty and sits in a group that sets one was read two ways: the REST and witness gates resolved the group's stack, while the satellite built the volume from the empty one, which is the default stack. In a LUKS group that put plaintext on disk under a definition every gate called encrypted. The stack is now decided once, by the controller, and written onto the definition, as upstream decides it at resource creation. A definition with nothing on disk takes the group's stack. One that already holds data keeps the stack its storage was built with: giving it the group's now would have the satellite build a different stack over that data. It is annotated with the group's stack it did not get, and a warning event names it, so such a volume reads as what it is and can be found. The group is looked up under its folded name, since a definition keeps the group's name as the operator spelled it. Once set, the stack is never emptied again. Both stores keep a decided stack when an update carries none, as a caller writing back a copy read before the pin does, and the CRD refuses an update that empties it, so the controller never decides a second time over a volume that may by then hold data. A stack can still be replaced by another one. The spec.layerStack description says what an empty stack now means. The controller-side resolver that walked the group on read had no callers left and is removed. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The satellite read a definition's empty layer stack as the default and built the volume from it, even when the definition's group sets another stack. It now waits, on a short requeue, until the controller has written the group's stack onto the definition, and the intent builder refuses such a definition outright, so no path turns the empty stack into storage. The group is looked up under its folded name, so a definition naming it in another case waits too rather than being taken for one outside any group. A definition in no group, or in a group without a stack, keeps the default as before; a group that cannot be read applies nothing. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Format took "not a LUKS device" to mean "empty" and ran luksFormat. A volume that went to disk without LUKS and is later asked for it, by a layer stack that changed under it or a restore that brought data in, is not LUKS either, and luksFormat destroys it. Format now probes the device with `blkid -p` first and formats only one that carries no signature at all. DRBD internal metadata, a filesystem or a partition table refuses it with ErrDeviceHoldsData, naming what was found; a probe that cannot say the device is empty refuses it too. blkid exits 2 both for "nothing found" and for a device it could not open, so exit 2 counts as empty only without an "error:" line on stderr and for a path that stats as a block device; a missing path or a regular file is refused. The apply of that replica fails with the reason, and nothing is written. The satellite also records the refusal as a LUKSDeviceHoldsData warning Event on the replica, through events.k8s.io, for which its role gains the events rule. A signature left on reused extents is cleared with wipefs; the message says so. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The layer stack pin looked only at what replicas had reported. A definition can hold data before any of them has: linstor-migrate adopts volumes that are already full and latches Initialized on them, and the latch is set once the controller has seen a real write. Either way the storage was built under the stack the definition carried, so the pin keeps it. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The converter copied the resource definition's layer_stack column as is. LINSTOR resolves the stack per resource and often leaves that column empty, so the stack those volumes were built with lives only in LAYER_RESOURCE_IDS. A converted definition with an empty stack then took whatever blockstor resolved for it, not what is on disk: a LUKS volume lost its LUKS layer, and a definition in a group with another stack was read through that group's. An empty stack is now read off the replicas' layer chains. Replicas built in different ways leave it unset and are reported, as is a replica built otherwise than the stack its definition names, since the satellite would not bring it up as it is. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The page said an RD with an empty stack inherits its group's at read time, which is what put plaintext on disk under encrypted definitions. It now says the stack is fixed once on the definition, by the REST create path or by the controller's pin, what happens to a definition that already holds data, what the operator sees, and how to find such definitions. The migration guide and the REST create path's comment say the same. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A clone or restore wrote the source's stack onto the new definition as stored, reading an empty one as the default. Under a group that sets a stack, an empty one is not decided yet: the controller pins it to the group's stack or to the default depending on whether data is already there, and the satellite builds nothing until it does. The copy took the default regardless and kept it for good, so a shell of an empty definition in a LUKS group stayed plaintext with no data forcing it. The stack is now resolved through the group. An undecided source, leftover or prepared target is answered 503 (the CLI exits with an error) before anything is written, and a group that cannot be read is an error rather than the default. A volume-less clone copies no bytes, so it takes only a stack the source chose and is otherwise left for its own group to decide. A copy short of what its group asks for carries the unapplied-group-layer-stack annotation the controller puts on a definition pinned over data. The restore marker goes onto a prepared target only if, in the patch that writes it, the target's stack still has the source's layers, so a stack rewritten after the target was judged is refused rather than restored into. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The pin compared the stack it applied with the group's in order and case, while clone and restore compared them as sets. A group spelling its stack in another order or case flagged every definition holding data as short of it. Both now share one comparison. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The recorder writes events.k8s.io events, which the stand's controller role did not grant, so the layer-stack warning was dropped. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The unapplied-group-layer-stack annotation was set on any difference from the group's stack, so an encrypted copy in a plaintext group was flagged as if it were the plaintext volume the annotation exists to find. It is now set only when a layer the group asks for is missing. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
LINSTOR builds a diskless replica as DRBD over STORAGE whatever the definition's stack, so counting it made the replicas of every LUKS definition with a tiebreaker look divergent and left its stack unset. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…group Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A thick LV is allocated with -Wn -Zn, without zeroing, and nothing wiped a volume's signatures when it was deleted, so a new LV starts with whatever its extents held. When that was a deleted LUKS volume's header, the LUKS layer found a header, skipped the format and opened it with the cluster passphrase every volume shares: the new volume showed the old one's contents. Any other leftover, a filesystem or DRBD metadata, made the LUKS layer refuse to format a volume that holds nothing. CreateVolume now creates the LV under a temporary name, the volume's name with a .bsnew suffix, runs wipefs over it and only then renames it to the volume's name, so nothing under a volume's name carries a signature another volume left. wipefs erases the signatures libblkid recognises, the LUKS header magic among them, and nothing else: the rest of the old volume's blocks stays on the extents, and a plaintext volume can read it back from the raw device. Zeroing the LV would close that, at the cost of writing a large thick LV in full before it can be used. An LV already under the volume's name is the volume and is never wiped. A temporary LV left by a run that stopped before the rename is wiped and renamed rather than created again. A failed listing, wipe or rename fails CreateVolume, and a failed wipe or rename leaves nothing under the volume's name. The orphan sweeper only knows LVs named like a volume, so it never sees a temporary one; DeleteVolume removes a leftover one along with the volume. DeleteVolume wipes nothing: lvremove refuses an LV that is still open, and that LV has to keep its data. The next CreateVolume over the freed extents wipes their signatures anyway. Thin LVM, ZFS and the file provider do not need this: a fresh thin LV, zvol or backing file reads zeroes where nothing has been written. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
… name A definition keeps its group's name as the operator spelled it (`--resource-group DfltRscGrp`), while the group's CR is stored under the lowercased name. The witness gate and the effective-props resolver (which the satellite, the dispatcher and the controller render DRBD options with) looked the group up by the spelled name, missed it, and went on as if there were no group: a STORAGE-only group got a DRBD witness, and the group's DRBD options were left out of the rendered config. Both now look the group up under the folded name, as the store does. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
An L6 cell and an L7 replay drive the layer-stack pin through the linstor CLI: a definition with nothing on disk, created in a group without a stack, has none, and takes the LUKS group's stack once rd modify moves it there; the REST create copies a group's stack itself, so only the move shows the controller's pin. One with data keeps DRBD,STORAGE when its group gains LUKS, is annotated with the group's stack, and gets the LayerStackNotInherited Event; a clone taken straight after the group change is refused as undecided until the pin and then succeeds with the source's stack. The cell also checks the data reads back unchanged. The replay runner gains two await kinds to read what the CLI does not show: rd_layer_stack (spec.layerStack) and rd_annotation. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
… to its group The mark recorded the group's stack at pin time and was never looked at again, so a definition whose group later dropped the layer it lacks still read as short of its group. Each reconcile now re-checks a flagged definition: the mark follows the group's current stack, and goes once the group no longer asks for a missing layer or is deleted. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…refusal The hint suggested wipefs for any signature, including DRBD metadata on a volume that is already in use, where wiping destroys the volume. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The refresh looked only at a definition that already carried the mark, and could now remove it, so a group that dropped a layer and took it back left the definition unflagged for good. Every definition with a decided stack is now checked against its group's current stack: flagged, with a warning Event, while it lacks a layer of it, and cleared once it does not. That also flags one whose group gains a layer after the pin. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…g it The next create renamed the leftover whatever its size, so a volume grown in between came up at the size the crashed call asked for. The leftover was never exposed under the volume's name, so it is removed and the LV is created again at the volume's current size. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…source Since the mark also flags a stack set by hand that lacks a layer of the group, a volume-less clone dropped such a stack, LUKS,STORAGE in a DRBD,STORAGE group say, and the controller pinned the clone to the group's plain stack. A stack pinned over data is always the default one, so only a flagged source with the default stack hands the clone nothing now. The annotation's comment and the docs describe its wider meaning. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…changes The mark caught up with a group edit only when the definition was reconciled for another reason or at the resync. A watch on groups now enqueues the definitions in one whose stack changed. The refresh's Event no longer tells a definition that holds nothing to migrate its data, and its comment says the group is read directly. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
11c1de8 to
04acafe
Compare
A definition that left its layer stack to its resource group was read two ways. The API and the placement gates took the group's stack, while the satellite built the volume from the definition's own empty stack. In a LUKS group that means a definition reported as encrypted with plaintext volumes under it.
Now the stack is decided once and stored on the definition:
blockstor.io/unapplied-group-layer-stackannotation and aLayerStackNotInheritedwarning event. The definition reconcile runs every 5s, so a group that gains a stack is picked up within that.luksFormaton a device that carries anyblkidsignature, and a probe that cannot open the device, or a path that is not a block device, counts as a refusal. The refusal is aLUKSDeviceHoldsDatawarning Event on the replica.wipefsover it, and only then renames it to the volume's name, so nothing under a volume's name carries a signature another volume left, and an LV already under that name is never wiped.wipefserases signatures only: the rest of the old volume's blocks stays on the extents and a plaintext volume can read them from the raw device. Zeroing would close that at the cost of writing a large thick LV in full first. Delete wipes nothing, sincelvremoverefuses an LV that is still open and that LV has to keep its data. That also closes a leak that predates this PR: a new LV on a deleted LUKS volume's extents found the old header and opened it with the shared cluster passphrase, exposing the old volume's data. Thin LVM, ZFS and file volumes read zeroes when new and need nothing.linstor-migratereads an empty definition stack from the layer chain of its diskful replicas and reports replicas that disagree.docs/layer-stack.mdsays where the stack is decided and how to find the affected definitions.The warning events need
events.k8s.ioevents (create, patch) in the controller and satellite RBAC.An L6 cell and an L7 replay (
rg-layer-stack-pin) cover the pin at operator-CLI level; the replay runner gainsrd_layer_stackandrd_annotationawaits. Not run on a stand yet.Stacked on #190.
A pinned definition's
unapplied-group-layer-stackannotation now follows its group on every reconcile, and goes once the group no longer asks for a missing layer. The luksFormat refusal no longer suggestswipefsfor DRBD metadata on a volume already in use.