fix(crypto): add extra 16MiB space to engine and replicas by mantissahz · Pull Request #4624 · longhorn/longhorn-manager

mantissahz · 2026-03-31T13:34:40Z

Which issue(s) this PR fixes:

Issue # longhorn/longhorn#9205

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Merge after this PR longhorn/go-common-libs#174

coderabbitai · 2026-03-31T13:34:48Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dd682624-561b-482a-bd16-6a8e2b6b8046

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

derekbit · 2026-04-06T03:08:04Z

@mantissahz Is the PR ready?

mantissahz · 2026-04-07T00:38:39Z

Hi @derekbit,
No. The PR longhorn/go-common-libs#174 should be merged, and the package dependency should be updated first

webhook/resources/volumeattachment/validator.go

webhook/resources/volume/validator.go

Copilot

Pull request overview

This PR addresses encrypted volume sizing by accounting for the fixed 16 MiB LUKS2 header (Issue longhorn/longhorn#9205; issue context unavailable in this review environment), and adds safeguards so migrations/upgrades only proceed when the engine image supports the required crypto behavior.

Changes:

Add a shared “backend size” calculation (volume size + 16 MiB header when applicable) and use it when creating/upgrading engine/replica processes and when reconciling expansion.
Add webhook validations to block encrypted live migration actions when the engine image CLI API version is too old to support the new sizing behavior.
Introduce cryptsetup version detection and a pre-live-upgrade encrypted device resize step for the fixed-header behavior.

Reviewed changes

Copilot reviewed 14 out of 17 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
webhook/resources/volumeattachment/validator.go	Adds encrypted migratable-volume validation gated on engine CLI API version.
webhook/resources/volume/validator.go	Adds validation when setting MigrationNodeID for encrypted volumes to require sufficient engine CLI API version.
util/util.go	Adds cryptsetup version detection and a “precise backend size” helper used by controllers/engine process args.
types/types.go	Adds StorageClass parameter keys for node-stage secret lookup.
controller/engine_controller.go	Adds pre-live-upgrade expansion/resize flow for encrypted volumes and updates expansion logic to compare against backend size.
controller/replica_controller.go	Passes encryption flag into replica instance creation requests.
controller/volume_controller.go	Uses backend size when determining expansion state and cancellation.
engineapi/types.go	Updates `VolumeExpand` interface to accept an explicit size.
engineapi/proxy_volume.go	Plumbs explicit size through proxy `VolumeExpand`.
engineapi/instance_manager.go	Adjusts engine/replica process `--size` args for encrypted volumes; adds encryption flag to create/upgrade requests.
engineapi/enginesim.go	Updates simulator signature for `VolumeExpand(engine, size)`.
engineapi/engine.go	Updates engine-binary signature for `VolumeExpand(engine, size)`.
csi/crypto/crypto.go	Reuses shared LUKS2 header size constant from go-common-libs.
vendor/modules.txt	Updates vendored `go-common-libs` version.
vendor/github.com/longhorn/go-common-libs/types/crypto.go	Adds constants and `GetBackendSize()` helper for LUKS2 header sizing.
go.mod	Bumps `github.com/longhorn/go-common-libs` dependency version.
go.sum	Updates checksums for the bumped `go-common-libs` version.

controller/engine_controller.go

webhook/resources/volume/validator.go

webhook/resources/volumeattachment/validator.go

engineapi/instance_manager.go

util/util.go

controller/engine_controller.go

webhook/resources/volume/validator.go

controller/engine_controller.go

derekbit · 2026-04-09T02:32:47Z

Pull request overview

This PR addresses encrypted volume sizing by accounting for the fixed 16 MiB LUKS2 header (Issue longhorn/longhorn#9205; issue context unavailable in this review environment), and adds safeguards so migrations/upgrades only proceed when the engine image supports the required crypto behavior.

Changes:

Add a shared “backend size” calculation (volume size + 16 MiB header when applicable) and use it when creating/upgrading engine/replica processes and when reconciling expansion.

Add webhook validations to block encrypted live migration actions when the engine image CLI API version is too old to support the new sizing behavior.

Introduce cryptsetup version detection and a pre-live-upgrade encrypted device resize step for the fixed-header behavior.

Reviewed changes

Copilot reviewed 14 out of 17 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
webhook/resources/volumeattachment/validator.go Adds encrypted migratable-volume validation gated on engine CLI API version.
webhook/resources/volume/validator.go Adds validation when setting MigrationNodeID for encrypted volumes to require sufficient engine CLI API version.
util/util.go Adds cryptsetup version detection and a “precise backend size” helper used by controllers/engine process args.
types/types.go Adds StorageClass parameter keys for node-stage secret lookup.
controller/engine_controller.go Adds pre-live-upgrade expansion/resize flow for encrypted volumes and updates expansion logic to compare against backend size.
controller/replica_controller.go Passes encryption flag into replica instance creation requests.
controller/volume_controller.go Uses backend size when determining expansion state and cancellation.
engineapi/types.go Updates VolumeExpand interface to accept an explicit size.
engineapi/proxy_volume.go Plumbs explicit size through proxy VolumeExpand.
engineapi/instance_manager.go Adjusts engine/replica process --size args for encrypted volumes; adds encryption flag to create/upgrade requests.
engineapi/enginesim.go Updates simulator signature for VolumeExpand(engine, size).
engineapi/engine.go Updates engine-binary signature for VolumeExpand(engine, size).
csi/crypto/crypto.go Reuses shared LUKS2 header size constant from go-common-libs.
vendor/modules.txt Updates vendored go-common-libs version.
vendor/github.com/longhorn/go-common-libs/types/crypto.go Adds constants and GetBackendSize() helper for LUKS2 header sizing.
go.mod Bumps github.com/longhorn/go-common-libs dependency version.
go.sum Updates checksums for the bumped go-common-libs version.

@copilot Do you review PRs based on the instructions?

derekbit · 2026-04-09T15:24:52Z

controller/engine_controller.go

 }

-func IsValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int) (bool, error) {
+func IsValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int, encrypted bool) (bool, int64, error) {


Suggested change

func IsValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int, encrypted bool) (bool, int64, error) {

func isValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int, encrypted bool) (bool, int64, error) {

derekbit · 2026-04-09T15:29:18Z

util/util.go

 	return false
 }
+
+func GetPreciseBackendSize(size int64, encrypted bool, cliAPIVersion int) (int64, error) {


Precise is a bit weird. Unable to understand the purpose of the volume from the name. Can we use GetActualBackendSize?

derekbit · 2026-04-09T15:30:12Z

util/util.go

+func IsCryptsetupVerWithFixed16MiBHeaderSize() (bool, error) {
+	ver, err := getCryptsetupVersion()
+	if err != nil {
+		return false, err
+	}
+
+	parsedVer, err := version.ParseSemantic(ver)
+	if err != nil {
+		return false, fmt.Errorf("failed to parse cryptsetup version %q: %w", ver, err)
+	}
+
+	// The fixed header size 16 MiB was introduced in cryptsetup 2.1.0. See: https://www.kernel.org/pub/linux/utils/cryptsetup/v2.1/v2.1.0-ReleaseNotes.
+	minVer, err := version.ParseSemantic("2.1.0")
+	if err != nil {
+		return false, fmt.Errorf("failed to parse minimum cryptsetup version: %w", err)
+	}
+
+	return parsedVer.AtLeast(minVer), nil
+}


Move the function to go-common-libs

derekbit · 2026-04-09T15:30:40Z

util/util.go

+func GetPreciseBackendSize(size int64, encrypted bool, cliAPIVersion int) (int64, error) {
+	if !encrypted {
+		return size, nil
+	}
+
+	if is16MiBHeaderPkgVersion, err := IsCryptsetupVerWithFixed16MiBHeaderSize(); err != nil || !is16MiBHeaderPkgVersion {
+		return size, err
+	}
+
+	return lhtypes.GetBackendSize(size, encrypted, cliAPIVersion), nil
+}


Move the function to go-common-libs.

derekbit · 2026-04-09T15:30:55Z

util/util.go

+func getCryptsetupVersion() (string, error) {
+	var err error
+
+	namespaces := []lhtypes.Namespace{lhtypes.NamespaceMnt, lhtypes.NamespaceNet}
+	nsexec, err := lhns.NewNamespaceExecutor(lhtypes.ProcessNone, lhtypes.HostProcDirectory, namespaces)
+	if err != nil {
+		return "", err
+	}
+
+	result, err := nsexec.Execute(nil, lhtypes.BinaryCryptsetup, []string{"--version"}, time.Hour)
+	if err != nil {
+		return "", errors.Wrap(err, "cannot find cryptsetup version info on host")
+	}
+
+	//command: cryptsetup --version; result: cryptsetup 2.4.3\n
+	fields := strings.Fields(result)
+	if len(fields) < 2 {
+		return "", fmt.Errorf("failed to parse cryptsetup version from output %q", result)
+	}
+	return fields[1], nil
+}


Move the function to go-common-libs.

derekbit · 2026-04-09T15:32:25Z

controller/engine_controller.go

+				"currentSize":         e.Status.CurrentSize,
+				"expectedBackendSize": needBacknedSize,
+				"volume":              e.Spec.VolumeName,
+			}).Info("Wait for the expected volume size to be updated before engine live upgrade for encrypted volume")


Suggested change

}).Info("Wait for the expected volume size to be updated before engine live upgrade for encrypted volume")

}).Info("Waiting for the expected volume size to be updated before engine live upgrade for encrypted volume")

derekbit · 2026-04-09T15:33:38Z

controller/engine_controller.go

+		}
+
+		e.Status.IsExpanding = true
+		ec.enqueueEngineAfter(e, 30*time.Second)


30 seconds is quite long. Why do we set a long duration?

derekbit · 2026-04-09T15:37:05Z

controller/engine_controller.go

+			if is16MiBHeaderPkgVersion {
+				if isExpanding, err := ec.expandEncryptedVolumeBeforeLiveUpgrade(volume, engine); err != nil || isExpanding {
+					// Wait for the expected volume size to be updated before engine live upgrade for encrypted volume
+					return err


Is it reasonable to return nil err if the returned values are "isExpanding == true" and "err == nil"?

derekbit · 2026-04-09T15:37:55Z

controller/engine_controller.go

+	if !v.Spec.Encrypted || e.Status.CurrentState != longhorn.InstanceStateRunning {
+		return false, nil
+	}


How do we handle the detached encrypted volume?

derekbit · 2026-04-09T15:44:34Z

controller/engine_controller.go

+	}
+
+	e.Status.CurrentSize = volumeInfo.Size
+	needBacknedSize := lhtypes.GetBackendSize(e.Spec.VolumeSize, v.Spec.Encrypted, cliAPIVersion)


Typo. needBackendSize
BTW, can we find a better name for needBackendSize. The name is not easy to understand the purppse.

derekbit · 2026-04-09T15:46:34Z

controller/engine_controller.go

+			}
+		}
+
+		e.Status.IsExpanding = true


You reuse e.Status.IsExpanding in the PR. What will happen if user trigger REAL volume exapsion? Should we reject the REAL volume exapsion request"

derekbit · 2026-04-09T15:47:40Z

controller/engine_controller.go

+	return e.Status.IsExpanding, nil
+}
+
+func (ec *EngineController) resizeEncryptedDeviceBeforeUpgradeFor16MiBHeader(v *longhorn.Volume) error {


nit: the function name is too long

derekbit · 2026-04-10T03:57:20Z

After disussing with @mantissahz, the PR doesn't handle the non-running volumes expansionl. We need to handle the logics.

of the encryption volume. ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

mantissahz requested a review from a team March 31, 2026 13:34

mantissahz self-assigned this Mar 31, 2026

derekbit requested a review from shuo-wu April 2, 2026 00:56

mantissahz force-pushed the issue9205 branch 2 times, most recently from 9d03fb8 to 36a457b Compare April 7, 2026 04:57

derekbit reviewed Apr 7, 2026

View reviewed changes

webhook/resources/volumeattachment/validator.go Outdated Show resolved Hide resolved

derekbit reviewed Apr 7, 2026

View reviewed changes

webhook/resources/volume/validator.go Outdated Show resolved Hide resolved

mantissahz force-pushed the issue9205 branch from 36a457b to 58e1513 Compare April 8, 2026 10:12

mantissahz marked this pull request as ready for review April 8, 2026 10:51

mantissahz marked this pull request as draft April 8, 2026 10:52

mantissahz requested review from a team April 8, 2026 10:53

derekbit force-pushed the issue9205 branch from 58e1513 to 86c2766 Compare April 8, 2026 11:19

mantissahz force-pushed the issue9205 branch 2 times, most recently from be389bb to baa81df Compare April 8, 2026 11:26

mantissahz marked this pull request as ready for review April 9, 2026 02:22

mantissahz requested review from Copilot and derekbit April 9, 2026 02:22

Copilot started reviewing on behalf of mantissahz April 9, 2026 02:23 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

mantissahz force-pushed the issue9205 branch from baa81df to 2ac5089 Compare April 9, 2026 04:11

derekbit requested review from WebberHuang1118 April 9, 2026 15:16

derekbit force-pushed the issue9205 branch from 2ac5089 to 24b6b59 Compare April 9, 2026 15:16

derekbit reviewed Apr 9, 2026

View reviewed changes

mantissahz added 5 commits April 13, 2026 22:06

fix(crypto): add extra 16MiB space to engine and replicas

e84e557

of the encryption volume. ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

fix(crypto): engine image ugprade for encrypted volumes

40f4445

ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

fix(crypto): cannot migrate enrypted volumes before cliAPIversion 12

998dbe9

ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

fix(crypto): check the cryptosetup version before expanding backend size

e3cb618

ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

fix(crypto): resize encrypted device after upgradeing

b4071d5

ref: longhorn/longhorn 9205 Signed-off-by: James Lu <james.lu@suse.com>

mantissahz force-pushed the issue9205 branch from 24b6b59 to b4071d5 Compare April 14, 2026 00:26

mantissahz mentioned this pull request Apr 14, 2026

[BUG] Encrypt volume provided size is 16MB shorter than the claimed size longhorn/longhorn#9205

Open

	func IsValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int, encrypted bool) (bool, int64, error) {
	func isValidForExpansion(engine *longhorn.Engine, cliAPIVersion, imAPIVersion int, encrypted bool) (bool, int64, error) {

	}).Info("Wait for the expected volume size to be updated before engine live upgrade for encrypted volume")
	}).Info("Waiting for the expected volume size to be updated before engine live upgrade for encrypted volume")

Conversation

mantissahz commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Uh oh!

coderabbitai bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

derekbit commented Apr 6, 2026

Uh oh!

mantissahz commented Apr 7, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derekbit commented Apr 9, 2026

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekbit commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mantissahz commented Mar 31, 2026 •

edited

Loading

coderabbitai bot commented Mar 31, 2026 •

edited

Loading