Storage: Avoid filling volume config on `VolumeDBCreate` #14923

hamistao · 2025-02-05T03:43:09Z

This is a pre-requisite for #14595.

The whole idea here is that we want to adapt the provided volume config on VolumeDBCreate to populate volatile.rootfs.size without changing the original config passed in, as this would be a very easy and consistent way to keep track of volume sizes.

But we are currently relying on VolumeDBCreate to populate some fields within the volume for future use, which is giving many unrelated purposes to this function. So this instead proposed filling this config on GetNewVolume instead, as it makes sense for a new volume to have default conifg for the driver filled in. That, along with a few more tweaks, allows us to use VolumeDBCreate exclusively to create the database entry for the volume, adapting the volume config as needed without compromising the original config passed in as an argument.

lxd/storage/utils.go

lxd/storage/backend_lxd.go

tomponline · 2025-02-05T08:26:13Z

lxd/storage/drivers/driver_ceph_volumes.go

Hrm I'd be hesitant to start forcefully killing ongoing storage commands that are mutating the local or remote storage layers during a clean shutdown. This feels like its asking for trouble to me.

Was there a reason you didn't go with context.Background() here to ensure the commands safely completed?

I just failed to consider the risks of stopping these commands midway. Considering that doing so would simply leave the storage layers unharmed is far too optimistic.

tomponline · 2025-02-05T08:27:42Z

lxd/storage/drivers/driver_common.go

@@ -289,7 +289,7 @@ func (d *common) moveGPTAltHeader(devPath string) error {
 		return nil
 	}

-	_, err = shared.RunCommand(path, "--move-second-header", devPath)
+	_, err = shared.RunCommandContext(d.state.ShutdownCtx, path, "--move-second-header", devPath)


i dont think we should use d.state.ShutdownCtx for anything mutating the disk, as Go will then forcefully kill the command, leaving it in an inconsistent state.

We've been bitten before by this in the past where we killed an ongoing ceph map command, which then left unrecoverable structures in the kernel and the system had to be rebooted.

We've been bitten before by this in the past where we killed an ongoing ceph map command, which then left unrecoverable structures in the kernel and the system had to be rebooted.

Always useful to know some history, thanks for the insight!
I will make the necessary updates.

In general if a command isnt mutating something we can use d.state.shutdownCtx to exit more quickly, but if we're mutating something it'd be better to let it finish in my view (ofc if thats going to take a long time and some cleanup is going to happen on error, such as writing a large file out and then removing it on error in a defer then that would be different).

tomponline

Lets avoid introducing potential new issues by forcefully canceling commands that are mutating on disk structures during a clean shutdown. Use context.Background() for these until such time as you're prepared to test each one for its resulting behaviour.

Signed-off-by: hamistao <[email protected]>

…olumeConfig` Signed-off-by: hamistao <[email protected]>

This erros is always nil, so there is no reason for is to keep this return type. Signed-off-by: hamistao <[email protected]>

Signed-off-by: hamistao <[email protected]>

…onfig` Signed-off-by: hamistao <[email protected]>

…nfig` Signed-off-by: hamistao <[email protected]>

…lumeConfig` Signed-off-by: hamistao <[email protected]>

…nfig` Signed-off-by: hamistao <[email protected]>

…onfig` Signed-off-by: hamistao <[email protected]>

Relying on VolumeDBCreate to perform this adds unnecessary contraints. Such as not enabling to perform operations that require having a filled in volume config before create the DB volume unless one adds a redundant call to FillVolumeConfig. Or not being able to set keys that aren't supposed to go in the database before calling VolumeDBCreate, such as `size` or `state.size`. Signed-off-by: hamistao <[email protected]>

The intention is to cover for almost all cases where we were filling volume configuration on `VolumeDBCreate`. Now this makes more sense as it is expected for a newly created volume to have the default configuration for its pool. Signed-off-by: hamistao <[email protected]>

This simplification also fits nicely into the concept of expecting a volume with a previously filled config. Signed-off-by: hamistao <[email protected]>

Signed-off-by: hamistao <[email protected]>

These volume are not created with `GetNewVolume`, so we need to call `FillVolumeConfig` on them. Signed-off-by: hamistao <[email protected]>

Signed-off-by: hamistao <[email protected]>

hamistao force-pushed the move_volume_config_filling branch 2 times, most recently from 8a73064 to 1497ea0 Compare February 5, 2025 04:50