lxd: Make the LXD shutdown sequence more concurrent to avoid long-running operations blocking unrelated instances shutdown #15016

gabrielmougard · 2025-02-18T09:07:28Z

closes: #13460
JIRA ticket: https://warthogs.atlassian.net/browse/LXD-2600

gabrielmougard · 2025-02-18T09:10:23Z

@tomponline we can start discussing the approach. I mark it as draft as I'm stillin the process to test it but just wanted to let you know about my idea. @simondeziel what was the test setup you used so that I can reproduce a bunch of long running operations and start testing this appraoch.

simondeziel · 2025-02-18T18:36:06Z

@simondeziel what was the test setup you used so that I can reproduce a bunch of long running operations and start testing this appraoch.

It's been a while and it seems it was an image refresh taking longer than usual. I think mimic something akin with:

$ lxc launch ubuntu-minimal-daily:24.04 c1
Launching c1
$ lxc exec c1 -- sleep inifinity

Then sudo systemctl stop snap.lxd.daemon.

github-advanced-security

ShellCheck found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

test/includes/lxd.sh

test/suites/shutdown.sh

tomponline · 2025-02-27T08:57:57Z

lxd/db/operationtype/operation_type.go

@@ -78,6 +80,140 @@ const (
 	ClusterHeal
 )

+// StringToOperationType converts a string (case-insensitive) to an Operation Type.
+// Returns Unknown if the string doesn't match any known type.
+func StringToOperationType(opTypeString string) Type {


@markylaing is there an existing/better way to do this so we dont forget to add this mapping for new entity types? Seems similar to what we've done before.

gabrielmougard · 2025-02-27T09:08:58Z

If one wants to reproduce the tests: this should be working https://paste.ubuntu.com/p/MGxxJz35N7/ (still fuguring out why this is different in the CI)

The OperationTracker and the tracking groups contained in it, is a handy way to keep track of operation objects by associating them with their entity URLs. This operation tracker is meant to track instances and volumes related entities in a separate fashion so that we don't hold a global mutex to take a look to an entity if we want to look an an other entity of a different type at the same type. Signed-off-by: Gabriel Mougard <[email protected]>

…perationTracker aware This enables `instancesShutdown` to react upon operation changes (from busy to idle) and to context cancellation while keeping the shutdown order correct if instances have `boot.stop.priority` set. Signed-off-by: Gabriel Mougard <[email protected]>

…rationTracker and context Signed-off-by: Gabriel Mougard <[email protected]>

…ss` utility methods This is needed to create dummy operations using the internal API in the tests. Signed-off-by: Gabriel Mougard <[email protected]>

For the testing phase, we need a convenient way to create dummy operations in LXD with a certain duration. Signed-off-by: Gabriel Mougard <[email protected]>

test/includes/check.sh

test/includes/operations.sh

… tests Signed-off-by: Gabriel Mougard <[email protected]>

Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard · 2025-03-01T15:05:49Z

@tomponline the tests are passing now

simondeziel

There are many test scenarios which is good for coverage but this test is pretty expansive in terms of runtime (~500s so >8 minutes). For comparison, the second longest test finishes in ~135s (2m15s).

I suggested switching from alpine to busybox but I don't know how much of a difference it will make. If you have other ideas to speed it up, please let me know.

simondeziel · 2025-03-02T20:12:38Z

lxd/db/operationtype/operation_type.go

+	opTypeString = strings.ReplaceAll(opTypeString, " ", "")
+	opTypeString = strings.ToLower(opTypeString)


Suggested change

opTypeString = strings.ReplaceAll(opTypeString, " ", "")

opTypeString = strings.ToLower(opTypeString)

opTypeString = strings.ReplaceAll(strings.ToLower(opTypeString), " ", "")

simondeziel · 2025-03-02T20:14:01Z

lxd/db/operationtype/operation_type.go

+	switch opTypeString {
+	case "clusterbootstrap":
+		return ClusterBootstrap
+	case "clusterjoin":
+		return ClusterJoin
+	case "backupcreate":
+		return BackupCreate
+	case "backuprename":


Is there a specific logic to the order of the case? If not, could we sort them alphabetically?

simondeziel · 2025-03-02T20:19:20Z

lxd/operations.go

+		// and to avoid the situation where a single very long running operation can block the shutdown of unrelated instances and the unmount of unrelated storage volumes.
+		if opAPI.Resources != nil {
+			for resourceName, resourceEntries := range opAPI.Resources {
+				if resourceName == "instances" || resourceName == "storage_volumes" || resourceName == "storage_volume_snapshots" || resourceName == "backups" {


Reversing the if to do an early continue would reduce the indentation and make it more readable IMHO.

simondeziel · 2025-03-02T20:22:00Z

lxd/operations/operations.go

+	opClassString = strings.ReplaceAll(opClassString, " ", "")
+	opClassString = strings.ToLower(opClassString)


Suggested change

opClassString = strings.ReplaceAll(opClassString, " ", "")

opClassString = strings.ToLower(opClassString)

opClassString = strings.ReplaceAll(strings.ToLower(opClassString), " ", "")

simondeziel · 2025-03-02T20:24:29Z

test/includes/check.sh

+
+  local all_found=true
+  for message in "${expected_messages[@]}"; do
+    if grep -q "$message" "$logfile"; then


Suggested change

if grep -q "$message" "$logfile"; then

if grep -qF "$message" "$logfile"; then

simondeziel · 2025-03-02T20:39:28Z

test/suites/shutdown.sh

@@ -0,0 +1,671 @@
+test_shutdown() {
+    ensure_import_testimage


This doesn't seem to be used (yet).

simondeziel · 2025-03-02T20:43:10Z

test/suites/shutdown.sh

+    delete_instances 5
+    rm "$scenario_name.log"
+
+    # The following scenarios are only relevant for LXD with storage backend other than ceph.


Could you expand the comment to explain why ceph isn't relevant here.

simondeziel · 2025-03-02T20:44:07Z

test/suites/shutdown.sh

+    lxc storage volume delete mypool backups_volume
+    lxc storage volume delete mypool images_volume
+    lxc storage delete mypool
+}


Missing \n.

simondeziel · 2025-03-02T20:45:01Z

test/suites/shutdown.sh

+    lxc config set i1 boot.stop.priority 0
+    lxc config set i2 boot.stop.priority 1
+    lxc config set i3 boot.stop.priority 2
+    lxc config set i4 boot.stop.priority 2
+    lxc config set i5 boot.stop.priority 3


Is there a scenario where some of the instances have a stop prio while others don't have any? I don't know if that'd be a relevant thing to test for though.

simondeziel · 2025-03-02T21:00:04Z

test/includes/lxd.sh

+
+  for i in $(seq 1 "$n"); do
+    echo "Creating instance i$i..."
+    lxc launch images:alpine/3.18 "i$i"


Could we use the regular busybox image called testimage with a small root disk instead?

Suggested change

lxc launch images:alpine/3.18 "i$i"

lxc launch testimage "i${i}" -d "${SMALL_ROOT_DISK}"

gabrielmougard closed this Feb 25, 2025

gabrielmougard reopened this Feb 25, 2025

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch from cc831d8 to 34c2e89 Compare February 25, 2025 17:36

github-advanced-security bot found potential problems Feb 25, 2025

View reviewed changes

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch from 34c2e89 to 258618c Compare February 26, 2025 09:16

github-advanced-security bot found potential problems Feb 26, 2025

View reviewed changes

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch 18 times, most recently from 2542323 to 42ae29a Compare February 27, 2025 08:53

tomponline reviewed Feb 27, 2025

View reviewed changes

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch from 42ae29a to 8cc26b7 Compare February 27, 2025 08:58

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch 16 times, most recently from 3400433 to 21aaf3f Compare February 28, 2025 15:30

gabrielmougard added 4 commits February 28, 2025 17:03

lxd/daemon: Update LXD shutdown sequence caller site to work with Ope…

4ab8cf7

…rationTracker and context Signed-off-by: Gabriel Mougard <[email protected]>

lxd/operations: Add StringToOperationType and `StringToOperationCla…

c58c6ea

…ss` utility methods This is needed to create dummy operations using the internal API in the tests. Signed-off-by: Gabriel Mougard <[email protected]>

lxd/operations: Add internal operationWaitHandler API handler logic.

b0fc7b9

For the testing phase, we need a convenient way to create dummy operations in LXD with a certain duration. Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch from 21aaf3f to 4437875 Compare February 28, 2025 16:03

github-advanced-security bot found potential problems Feb 28, 2025

View reviewed changes

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch 2 times, most recently from 162fc79 to 37c2dfa Compare February 28, 2025 16:26

gabrielmougard added 2 commits February 28, 2025 18:09

test/includes: various helper functions for the LXD shutdown sequence…

cbf99ce

… tests Signed-off-by: Gabriel Mougard <[email protected]>

test: Add lxd shutdown sequence tests

5755fc6

Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard force-pushed the fix/improve-shutdown-sequence branch from 37c2dfa to 5755fc6 Compare February 28, 2025 17:09

gabrielmougard marked this pull request as ready for review March 1, 2025 15:05

simondeziel reviewed Mar 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lxd: Make the LXD shutdown sequence more concurrent to avoid long-running operations blocking unrelated instances shutdown #15016

lxd: Make the LXD shutdown sequence more concurrent to avoid long-running operations blocking unrelated instances shutdown #15016

gabrielmougard commented Feb 18, 2025

gabrielmougard commented Feb 18, 2025

simondeziel commented Feb 18, 2025

github-advanced-security bot left a comment

tomponline Feb 27, 2025

gabrielmougard commented Feb 27, 2025 •

edited

Loading

gabrielmougard commented Mar 1, 2025

simondeziel left a comment

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

simondeziel Mar 2, 2025

		opTypeString = strings.ReplaceAll(opTypeString, " ", "")
		opTypeString = strings.ToLower(opTypeString)

	opTypeString = strings.ReplaceAll(opTypeString, " ", "")
	opTypeString = strings.ToLower(opTypeString)
	opTypeString = strings.ReplaceAll(strings.ToLower(opTypeString), " ", "")

		opClassString = strings.ReplaceAll(opClassString, " ", "")
		opClassString = strings.ToLower(opClassString)

	opClassString = strings.ReplaceAll(opClassString, " ", "")
	opClassString = strings.ToLower(opClassString)
	opClassString = strings.ReplaceAll(strings.ToLower(opClassString), " ", "")

	if grep -q "$message" "$logfile"; then
	if grep -qF "$message" "$logfile"; then

	lxc launch images:alpine/3.18 "i$i"
	lxc launch testimage "i${i}" -d "${SMALL_ROOT_DISK}"

lxd: Make the LXD shutdown sequence more concurrent to avoid long-running operations blocking unrelated instances shutdown #15016

Are you sure you want to change the base?

lxd: Make the LXD shutdown sequence more concurrent to avoid long-running operations blocking unrelated instances shutdown #15016

Conversation

gabrielmougard commented Feb 18, 2025

gabrielmougard commented Feb 18, 2025

simondeziel commented Feb 18, 2025

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmougard commented Feb 27, 2025 • edited Loading

gabrielmougard commented Mar 1, 2025

simondeziel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmougard commented Feb 27, 2025 •

edited

Loading