lxd: add all-projects param to the operation API #12078

AuruTus · 2023-07-26T02:26:20Z

Add all-projects query param support for operationsGet API and update its swagger doc (both 1.0/operations and 1.0/operations?recursion=1).

Fix #11808

roosterfish

Hi @AuruTus, can you please add an API extension since this is adding extra functionality? For reference check #12045 and especially this commit 48ace10.

AuruTus · 2023-07-27T12:46:32Z

Hi @AuruTus, can you please add an API extension since this is adding extra functionality? For reference check #12045 and especially this commit 48ace10.

hi @roosterfish, thx for the review. I got it a bit more now. And I'm supposed to add r.CheckExtension for client's api call in codes, right? CMIIW 😃

roosterfish · 2023-07-27T12:55:31Z

Yes please.

lxd/operations.go

monstermunchkin · 2023-07-27T13:38:43Z

@AuruTus please sign-off all your commits (git commit -s), and rebase your PR as it seems to have conflicts.

AuruTus · 2023-07-27T14:05:43Z

@monstermunchkin hi master, all is done and signoffs are ok now.

lxd/operations.go

client/lxd_operations.go

lxd/operations.go

tomponline

Thanks for this.

We should add a test for this though.

@monstermunchkin do you know of any precedent for testing operation listings that may help here?

lxd/db/operations.go

doc/api-extensions.md

monstermunchkin · 2023-07-28T07:06:34Z

do you know of any precedent for testing operation listings that may help here?

We don't currently test operations, and we don't have a way of creating them via the API. What do you think about adding a new internal API endpoint called internal/testing/operations which can be used for creating operations? That way we could create fake operations and test against them.

lxd/operations.go

tomponline · 2023-07-28T07:13:42Z

do you know of any precedent for testing operation listings that may help here?

We don't currently test operations, and we don't have a way of creating them via the API. What do you think about adding a new internal API endpoint called internal/testing/operations which can be used for creating operations? That way we could create fake operations and test against them.

I'm not sure that is needed. It looks like the test_exec test function has a good example of how to inspect operations from lxc exec calls, so as operations hang around for 5s after they have completed, I suggest we use lxc exec as an easy way to create operations in different projects.

Signed-off-by: Aurutus <emslhy@hotmail.com>

tomponline · 2023-07-28T22:42:09Z

Please can you rebase so your commits so they tell the final logical set of changes rather than the evolution over the PR review. Thanks

AuruTus · 2023-07-29T01:24:39Z

Please can you rebase so your commits so they tell the final logical set of changes rather than the evolution over the PR review. Thanks

@tomponline hi master, sorry I don't understand how to operate such things and those above are already generated by rebasing and force-pushing. Could give me some guides?

tomponline · 2023-07-29T05:52:17Z

Hey, so what I meant was that this latest commit f7a2c8d alters a commit earlier on your PR. Because we don't squash merge but instead pull all of your individual commits into the main branch we like that each commit within the pr represent the final version of it. Does that make sense? Thanks

AuruTus · 2023-07-29T12:06:22Z

Hey, so what I meant was that this latest commit f7a2c8d alters a commit earlier on your PR. Because we don't squash merge but instead pull all of your individual commits into the main branch we like that each commit within the pr represent the final version of it. Does that make sense? Thanks

Oh I got it! 😄 . Will fix this later with the test sh script together. BTW, have a nice weekend.

Signed-off-by: Aurutus <emslhy@hotmail.com>

AuruTus · 2023-07-30T10:19:52Z

@tomponline hi master, sorry I encounter some problems in local test env now, so I have to push the rebased commits first.

The instance starting command has error as:

Creating x1
Starting x1                                
ERROR  [2023-07-30T09:05:27Z] Failed starting instance                      action=start created="2023-07-30 09:05:25.0278038 +0000 UTC" ephemeral=false instance=x1 instanceType=container project=default stateful=false used="1970-01-01 00:00:00 +0000 UTC"
Error: Failed to run: /root/go/bin/lxd forkstart x1 /root/workspace/lxd/test/tmp.617/P15/containers /root/workspace/lxd/test/tmp.617/P15/logs/x1/lxc.conf: exit status 1
Try `lxc info --show-log local:x1` for more info

And this is the log print

Name: x1
Status: STOPPED
Type: container
Architecture: x86_64
Created: 2023/07/30 09:05 UTC
Last Used: 2023/07/30 09:05 UTC

Log:

lxc x1 20230730090527.569 WARN     cgfsng - cgroups/cgfsng.c:fchowmodat:1252 - No such file or directory - Failed to fchownat(42, memory.oom.group, 1000000000, 0, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW )
lxc x1 20230730090527.571 ERROR    start - start.c:print_top_failing_dir:99 - Permission denied - Could not access /root. Please grant it x access, or add an ACL for the container root
lxc x1 20230730090527.571 ERROR    sync - sync.c:sync_wait:34 - An error occurred in another process (expected sequence number 1)
lxc x1 20230730090527.571 ERROR    lxccontainer - lxccontainer.c:wait_on_daemonized_start:877 - Received container state "ABORTING" instead of "RUNNING"
lxc x1 20230730090527.571 ERROR    start - start.c:__lxc_start:2074 - Failed to spawn container "x1"
lxc x1 20230730090527.571 WARN     start - start.c:lxc_abort:1039 - No such process - Failed to send SIGKILL via pidfd 43 for process 29334
lxc 20230730090527.741 ERROR    af_unix - af_unix.c:lxc_abstract_unix_recv_fds_iov:218 - Connection reset by peer - Failed to receive response
lxc 20230730090527.741 ERROR    af_unix - af_unix.c:lxc_abstract_unix_recv_fds_iov:218 - Connection reset by peer - Failed to receive response
lxc 20230730090527.741 ERROR    commands - commands.c:lxc_cmd_rsp_recv_fds:127 - Failed to receive file descriptors for command "get_state"
lxc 20230730090527.741 ERROR    commands - commands.c:lxc_cmd_rsp_recv_fds:127 - Failed to receive file descriptors for command "get_init_pid"

I've been stuck here for a while, and didn't find a way to solve it. 😢

tomponline · 2023-07-31T14:05:46Z

Does it run on the main branch, is is just your branch it does not run on?

AuruTus · 2023-08-01T07:24:14Z

Does it run on the main branch, is is just your branch it does not run on?

Yeah, tried both main and my own branch. Cannot setup instances with lxc launch testimage x1 in most test scripts. I try to change the /etc/sub{g,u}id for root like what I searched on google, but it doesn't work. (And I'm using WSL2. Alr followed the How to install LXD guide to install deps and can build it)

monstermunchkin · 2023-08-01T08:05:31Z

@AuruTus looks like a permission issue. Try granting execute permission for /root: chmod o+x /root

AuruTus · 2023-08-01T23:54:04Z

@AuruTus looks like a permission issue. Try granting execute permission for /root: chmod o+x /root

Cool! It works now. Thx master. I can continue my test. BTW, is there any guide on integration test result assertion? I noticed that there exists an expression like some command || false, but it won't output failed or error message or even stop the rest test cases.

MusicDin · 2023-08-02T07:45:35Z

@AuruTus You can use set -e in your tests. It is an option that tells the shell to exit immediately if any command exits with a non-zero status. Test in backup.sh is an example that uses set -e option.

AuruTus · 2023-08-02T11:42:24Z

Hi masters, I think these test cases is enough for standalone mode query. (I cannot create a new network successfully on my Ubuntu 22.04 distro when creating a new project, so I directly test with the default project).

And there're still some issues for my cluster testing. I cannot mount /sys/kernel/security. I tried other clustering test cases, but they also cannot build up the cluster successfully. They have the same error when setting up netns.

And this the log printed when I wrote scripts

==> TEST BEGIN: clustering operations get
==> Setup clustering bridge br3987
2023/08/02 09:15:29 auth - running at http://127.0.0.1:56263
==> Setup clustering netns lxd39871
mount: /sys/kernel/security: mount point does not exist.
cat: /root/workspace/lxd/test/tmp.939/ns/lxd39871/PID: No such file or directory
Error: argument "" is wrong: Invalid "netns" value

nsenter: failed to parse pid: ''
==> Spawn bootstrap cluster node in lxd39871 with storage driver dir
==> Setting up directory backend in /root/workspace/lxd/test/tmp.939/qom
==> Spawning lxd in /root/workspace/lxd/test/tmp.939/qom
cat: /root/workspace/lxd/test/tmp.939/ns/lxd39871/PID: No such file or directory
==> print test log
Error: Get "http://unix.socket/1.0": dial unix /root/workspace/lxd/test/tmp.939/qom/unix.socket: connect: no such file or directory
make: *** [Makefile:281: tmp] Error 1

monstermunchkin · 2023-08-02T12:58:13Z

And there're still some issues for my cluster testing. I cannot mount /sys/kernel/security. I tried other clustering test cases, but they also cannot build up the cluster successfully. They have the same error when setting up netns.

You're on WSL2, right? Perhaps this will fix your problem: microsoft/WSL#8840

If not, you could try setting up an Ubuntu VM and do your tests in there.

AuruTus · 2023-08-03T02:53:37Z

And there're still some issues for my cluster testing. I cannot mount /sys/kernel/security. I tried other clustering test cases, but they also cannot build up the cluster successfully. They have the same error when setting up netns.

You're on WSL2, right? Perhaps this will fix your problem: microsoft/WSL#8840

If not, you could try setting up an Ubuntu VM and do your tests in there.

got it! will try this after fixing static analysis.

Signed-off-by: Aurutus <emslhy@hotmail.com>

monstermunchkin · 2023-08-03T08:34:41Z

test/suites/operations.sh

+ set -e
+ echo "==> get all-projects operations normally"
+ result="$(lxc query "/1.0/operations?all-projects=true" | jq '.success[0]' | wc -l)"
+ test "${result}" -eq 1 || false


You can drop the || false.

Since this tests operations of all projects, limiting it to just one doesn't make sense. Instead you should have assert_get_all_operations take an argument which represents the number of expected operations.

monstermunchkin · 2023-08-03T08:35:25Z

test/suites/operations.sh

+ result="$(lxc query "/1.0/operations?recursion=1&all-projects=true" | jq '.success[0]' | grep -c 'status_code')"
+ test "${result}" -eq 1 || false


You can drop the || false.

Same as above, you should pass the expected number of operations as an argument.

monstermunchkin · 2023-08-03T08:59:58Z

test/suites/operations.sh

+ (
+ set -e
+ echo "==> get all-projects operations normally"
+ result="$(lxc query "/1.0/operations?all-projects=true" | jq '.success[0]' | wc -l)"


Suggested change

result="$(lxc query "/1.0/operations?all-projects=true" | jq '.success[0]' | wc -l)"

result="$(lxc query "/1.0/operations?all-projects=true" | jq '.success | length')"

monstermunchkin · 2023-08-03T09:04:11Z

test/suites/operations.sh

+ test "${result}" -eq 1 || false
+
+ echo "==> get all-projects operations recursively"
+ result="$(lxc query "/1.0/operations?recursion=1&all-projects=true" | jq '.success[0]' | grep -c 'status_code')"


Suggested change

result="$(lxc query "/1.0/operations?recursion=1&all-projects=true" | jq '.success[0]' | grep -c 'status_code')"

result="$(lxc query "/1.0/operations?recursion=1&all-projects=true" | jq '[.success[] | select(.status_code)] | length')"

This will count the number of occurrences of status_code.

monstermunchkin · 2023-08-03T09:32:12Z

test/suites/operations.sh

+ ensure_import_testimage
+
+ # create container
+ name=x1
+ lxc launch testimage ${name}
+ lxc list ${name} | grep RUNNING
+
+ # get_all_operations
+ lxc query -X POST -d '{\"command\":[\"touch\",\"/root/foo1\"],\"record-output\":false}' /1.0/instances/${name}/exec
+ sleep 1
+ assert_get_all_operations
+
+ lxc stop "${name}" --force
+ lxc delete "${name}"


In this function you want to test if all operations of all projects are listed. However, you only create an operation in a single project.

Instead, you need to create a second project, and launch a container there as well. Then, you can run the exec commands for both containers, and count the number of operations.

Got it. Will refactor this snippet with above ones later. I try to test these on VM. Too many constraints on my WSL.

monstermunchkin · 2023-08-03T09:33:16Z

test/suites/operations.sh

+ lxc list ${name} | grep RUNNING
+
+ # get_all_operations
+ lxc query -X POST -d '{\"command\":[\"touch\",\"/root/foo1\"],\"record-output\":false}' /1.0/instances/${name}/exec


Suggested change

lxc query -X POST -d '{\"command\":[\"touch\",\"/root/foo1\"],\"record-output\":false}' /1.0/instances/${name}/exec

lxc exec -T --project=<whatever> "${name}" true

monstermunchkin · 2023-08-03T09:34:13Z

test/suites/operations.sh

+test_operations_get_by_project() {
+ assert_get_project_operations() {
+ # $1: query param for project name
+ (
+ set -e
+ echo "==> get $1 operations normally"
+ result="$(lxc query "/1.0/operations?project=$1" | jq '.success[0]' | wc -l)"
+ test "${result}" -eq 1 || false
+
+ echo "==> get $1 operations recursively"
+ result="$(lxc query "/1.0/operations?recursion=1&project=$1" | jq '.success[0]' | grep -c 'status_code')"
+ test "${result}" -eq 1 || false
+ )
+ }
+
+ ensure_import_testimage
+
+ project="default"
+
+ # create container
+ name=x1
+ lxc launch testimage ${name} --project ${project}
+ lxc list ${name} --project ${project} | grep RUNNING
+
+ # get opetaions with project name
+ lxc query -X POST -d '{\"command\":[\"touch\",\"/root/foo1\"],\"record-output\":false}' /1.0/instances/${name}/exec?project=${project}
+ sleep 1
+ assert_get_project_operations ${project}
+
+ lxc stop "${name}" --force --project ${project}
+ lxc delete "${name}" --project ${project}
+}


The same comments as above apply here.

MusicDin · 2023-08-01T10:17:47Z

client/lxd_operations.go

+ return nil, err
+ }
+
+ // Turn it into just a list of operations


Just a small nit, but can you end comments with a full stop? Thanks

Will add this later too. 😄

tomponline · 2023-08-17T10:56:32Z

@AuruTus hi, is this something you're still interested in progressing? Thanks

AuruTus · 2023-08-17T13:11:49Z

@AuruTus hi, is this something you're still interested in progressing? Thanks

sry, a bit busy recently. may have no enough free time to test it. Maybe can close this PR, for the code is really simple.

MusicDin · 2023-08-17T14:27:51Z

@AuruTus Would you mind if I finish this?

AuruTus · 2023-08-18T00:56:15Z

@AuruTus Would you mind if I finish this?

Oh that's great then. thx.

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Jul 26, 2023

roosterfish requested changes Jul 27, 2023

View reviewed changes

roosterfish reviewed Jul 27, 2023

View reviewed changes

lxd/operations.go Outdated Show resolved Hide resolved

AuruTus force-pushed the feat/all-projects-param branch from 309ff9c to 15a2b77 Compare July 27, 2023 14:03

monstermunchkin suggested changes Jul 27, 2023

View reviewed changes

tomponline requested changes Jul 28, 2023

View reviewed changes

tomponline reviewed Jul 28, 2023

View reviewed changes

lxd/db/operations.go Show resolved Hide resolved

tomponline reviewed Jul 28, 2023

View reviewed changes

doc/api-extensions.md Show resolved Hide resolved

tomponline reviewed Jul 28, 2023

View reviewed changes

lxd/operations.go Outdated Show resolved Hide resolved

AuruTus force-pushed the feat/all-projects-param branch from 15a2b77 to ba781d6 Compare July 28, 2023 08:15

AuruTus added 2 commits July 28, 2023 16:19

api: add operations_get_query_all_projects extension

3a22f8e

Signed-off-by: Aurutus <emslhy@hotmail.com>

lxd/db: add support for getting nodes without project name

0ef4012

Signed-off-by: Aurutus <emslhy@hotmail.com>

AuruTus force-pushed the feat/all-projects-param branch from ba781d6 to 10a3102 Compare July 28, 2023 08:20

AuruTus added 3 commits July 30, 2023 18:14

lxd: add all-projects param for operationsGet

1bb491b

Signed-off-by: Aurutus <emslhy@hotmail.com>

lxd: add param swagger doc for operationsGet

1293cef

Signed-off-by: Aurutus <emslhy@hotmail.com>

doc/rest-api: Refresh swagger YAML

e591fac

Signed-off-by: Aurutus <emslhy@hotmail.com>

AuruTus force-pushed the feat/all-projects-param branch from f7a2c8d to 09ecaed Compare July 30, 2023 10:20

AuruTus force-pushed the feat/all-projects-param branch from d496f40 to 83850db Compare August 2, 2023 11:34

AuruTus force-pushed the feat/all-projects-param branch from 83850db to 5282b4a Compare August 3, 2023 03:24

AuruTus added 2 commits August 3, 2023 11:25

client: add GetOperationsAllProjects

76320de

Signed-off-by: Aurutus <emslhy@hotmail.com>

tests: add standalone test cases for operations_get

e58eeb7

Signed-off-by: Aurutus <emslhy@hotmail.com>

AuruTus force-pushed the feat/all-projects-param branch from 5282b4a to e58eeb7 Compare August 3, 2023 03:25

monstermunchkin suggested changes Aug 3, 2023

View reviewed changes

MusicDin requested changes Aug 3, 2023

View reviewed changes

MusicDin mentioned this pull request Aug 22, 2023

lxd: add all-projects param to the operation API #12172

Merged

tomponline closed this in #12172 Aug 23, 2023

		result="$(lxc query "/1.0/operations?recursion=1&all-projects=true" \| jq '.success[0]' \| grep -c 'status_code')"
		test "${result}" -eq 1 \|\| false

	result="$(lxc query "/1.0/operations?all-projects=true" \| jq '.success[0]' \| wc -l)"
	result="$(lxc query "/1.0/operations?all-projects=true" \| jq '.success \| length')"

	lxc query -X POST -d '{\"command\":[\"touch\",\"/root/foo1\"],\"record-output\":false}' /1.0/instances/${name}/exec
	lxc exec -T --project=<whatever> "${name}" true

lxd: add all-projects param to the operation API #12078

lxd: add all-projects param to the operation API #12078

Conversation

AuruTus commented Jul 26, 2023

roosterfish left a comment

Choose a reason for hiding this comment

AuruTus commented Jul 27, 2023

roosterfish commented Jul 27, 2023

monstermunchkin commented Jul 27, 2023

AuruTus commented Jul 27, 2023 • edited Loading

tomponline left a comment

Choose a reason for hiding this comment

monstermunchkin commented Jul 28, 2023

tomponline commented Jul 28, 2023

tomponline commented Jul 28, 2023

AuruTus commented Jul 29, 2023 • edited Loading

tomponline commented Jul 29, 2023

AuruTus commented Jul 29, 2023

AuruTus commented Jul 30, 2023

tomponline commented Jul 31, 2023

AuruTus commented Aug 1, 2023

monstermunchkin commented Aug 1, 2023

AuruTus commented Aug 1, 2023

MusicDin commented Aug 2, 2023

AuruTus commented Aug 2, 2023

monstermunchkin commented Aug 2, 2023

AuruTus commented Aug 3, 2023

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

MusicDin Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline commented Aug 17, 2023

AuruTus commented Aug 17, 2023

MusicDin commented Aug 17, 2023

AuruTus commented Aug 18, 2023

AuruTus commented Jul 27, 2023 •

edited

Loading

AuruTus commented Jul 29, 2023 •

edited

Loading

MusicDin Aug 1, 2023 •

edited

Loading