Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ type InstanceServer interface {
// Operation functions
GetOperationUUIDs() (uuids []string, err error)
GetOperations() (operations []api.Operation, err error)
GetOperationsAllProjects() (operations []api.Operation, err error)
GetOperation(uuid string) (op *api.Operation, ETag string, err error)
GetOperationWait(uuid string, timeout int) (op *api.Operation, ETag string, err error)
GetOperationWaitSecret(uuid string, secret string, timeout int) (op *api.Operation, ETag string, err error)
Expand Down
29 changes: 29 additions & 0 deletions client/lxd_operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,35 @@ func (r *ProtocolLXD) GetOperations() ([]api.Operation, error) {
return operations, nil
}

func (r *ProtocolLXD) GetOperationsAllProjects() ([]api.Operation, error) {
err := r.CheckExtension("operations_get_query_all_projects")
if err != nil {
return nil, err
}

AuruTus marked this conversation as resolved.
Show resolved Hide resolved
apiOperations := map[string][]api.Operation{}

path := "/operations"

v := url.Values{}
v.Set("recursion", "1")
v.Set("all-projects", "true")

// Fetch the raw value
_, err = r.queryStruct("GET", fmt.Sprintf("%s?%s", path, v.Encode()), nil, "", &apiOperations)
if err != nil {
return nil, err
}

// Turn it into just a list of operations
Copy link
Member

@MusicDin MusicDin Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this later too. 😄

operations := []api.Operation{}
for _, v := range apiOperations {
operations = append(operations, v...)
}

return operations, nil
}

// GetOperation returns an Operation entry for the provided uuid.
func (r *ProtocolLXD) GetOperation(uuid string) (*api.Operation, string, error) {
op := api.Operation{}
Expand Down
5 changes: 5 additions & 0 deletions doc/api-extensions.md
Original file line number Diff line number Diff line change
Expand Up @@ -2273,3 +2273,8 @@ This allows copying storage volume snapshots to and from remotes.
## `zfs_delegate`
This implements a new `zfs.delegate` volume Boolean for volumes on a ZFS storage driver.
When enabled and a suitable system is in use (requires ZFS 2.2 or higher), the ZFS dataset will be delegated to the container, allowing for its use through the `zfs` command line tool.

## `operations_get_query_all_projects`
AuruTus marked this conversation as resolved.
Show resolved Hide resolved

This adds new support of query param `all-projects` for GET api call of both `/1.0/operations` and `/1.0/operations?recursion=1`,
which allows to bypass the project name filter.
20 changes: 20 additions & 0 deletions doc/rest-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12012,6 +12012,16 @@ paths:
get:
description: Returns a JSON object of operation type to operation list (URLs).
operationId: operations_get
parameters:
- description: Project name
example: default
in: query
name: project
type: string
- description: Retrieve operations from all projects
in: query
name: all-projects
type: boolean
produces:
- application/json
responses:
Expand Down Expand Up @@ -12251,6 +12261,16 @@ paths:
get:
description: Returns a list of operations (structs).
operationId: operations_get_recursion1
parameters:
- description: Project name
example: default
in: query
name: project
type: string
- description: Retrieve operations from all projects
in: query
name: all-projects
type: boolean
produces:
- application/json
responses:
Expand Down
10 changes: 10 additions & 0 deletions lxd/db/operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ import (
"github.com/canonical/lxd/lxd/db/query"
)

// GetAllNodesWithOperations returns a list of nodes that have operations of all projects.
func (c *ClusterTx) GetAllNodesWithOperations(ctx context.Context) ([]string, error) {
AuruTus marked this conversation as resolved.
Show resolved Hide resolved
stmt := `
SELECT DISTINCT nodes.address
FROM operations
JOIN nodes ON nodes.id = operations.node_id
`
return query.SelectStrings(ctx, c.tx, stmt)
}

// GetNodesWithOperations returns a list of nodes that have operations.
func (c *ClusterTx) GetNodesWithOperations(ctx context.Context, project string) ([]string, error) {
stmt := `
Expand Down
48 changes: 43 additions & 5 deletions lxd/operations.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,16 @@ func operationCancel(s *state.State, r *http.Request, projectName string, op *ap
// ---
// produces:
// - application/json
// parameters:
// - in: query
// name: project
// description: Project name
// type: string
// example: default
// - in: query
// name: all-projects
// description: Retrieve operations from all projects
// type: boolean
// responses:
// "200":
// description: API endpoints
Expand Down Expand Up @@ -419,6 +429,16 @@ func operationCancel(s *state.State, r *http.Request, projectName string, op *ap
// ---
// produces:
// - application/json
// parameters:
// - in: query
// name: project
// description: Project name
// type: string
// example: default
// - in: query
// name: all-projects
// description: Retrieve operations from all projects
// type: boolean
// responses:
// "200":
// description: API endpoints
Expand Down Expand Up @@ -450,9 +470,18 @@ func operationCancel(s *state.State, r *http.Request, projectName string, op *ap
func operationsGet(d *Daemon, r *http.Request) response.Response {
s := d.State()

projectName := projectParam(r)
projectName := queryParam(r, "project")
allProjects := shared.IsTrue(queryParam(r, "all-projects"))
recursion := util.IsRecursionRequest(r)

if allProjects && projectName != "" {
return response.SmartError(
api.StatusErrorf(http.StatusBadRequest, "Cannot specify a project when requesting all projects"),
)
} else if !allProjects && projectName == "" {
projectName = project.Default
}

localOperationURLs := func() (shared.Jmap, error) {
// Get all the operations.
localOps := operations.Clone()
Expand All @@ -461,7 +490,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
body := shared.Jmap{}

for _, v := range localOps {
if v.Project() != "" && v.Project() != projectName {
if !allProjects && v.Project() != "" && v.Project() != projectName {
continue
}

Expand All @@ -485,7 +514,7 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
body := shared.Jmap{}

for _, v := range localOps {
if v.Project() != "" && v.Project() != projectName {
if !allProjects && v.Project() != "" && v.Project() != projectName {
continue
}

Expand Down Expand Up @@ -561,7 +590,11 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
err = s.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
var err error

membersWithOps, err = tx.GetNodesWithOperations(ctx, projectName)
if allProjects {
membersWithOps, err = tx.GetAllNodesWithOperations(ctx)
} else {
membersWithOps, err = tx.GetNodesWithOperations(ctx, projectName)
}
if err != nil {
return fmt.Errorf("Failed getting members with operations: %w", err)
}
Expand Down Expand Up @@ -613,7 +646,12 @@ func operationsGet(d *Daemon, r *http.Request) response.Response {
}

// Get operation data.
ops, err := client.UseProject(projectName).GetOperations()
var ops []api.Operation
if allProjects {
ops, err = client.GetOperationsAllProjects()
} else {
ops, err = client.UseProject(projectName).GetOperations()
}
if err != nil {
logger.Warn("Failed getting operations from member", logger.Ctx{"address": memberAddress, "err": err})
continue
Expand Down
1 change: 1 addition & 0 deletions shared/version/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ var APIExtensions = []string{
"network_allocations",
"storage_api_remote_volume_snapshot_copy",
"zfs_delegate",
"operations_get_query_all_projects",
}

// APIExtensionsCount returns the number of available API extensions.
Expand Down
2 changes: 2 additions & 0 deletions test/main.sh
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ if [ "${1:-"all"}" != "cluster" ]; then
run_test test_warnings "Warnings"
run_test test_metrics "Metrics"
run_test test_storage_volume_recover "Recover storage volumes"
run_test test_operations_get_all "operations_get_all"
run_test test_operations_get_by_project "operations_get_by_project"
fi

# shellcheck disable=SC2034
Expand Down
62 changes: 62 additions & 0 deletions test/suites/operations.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
test_operations_get_all() {
assert_get_all_operations() {
(
set -e
echo "==> get all-projects operations normally"
result="$(lxc query "/1.0/operations?all-projects=true" | jq '.success[0]' | wc -l)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')"

test "${result}" -eq 1 || false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test "${result}" -eq 1 || false
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the || false.

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

)
}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sleep 1
assert_get_all_operations

lxc stop "${name}" --force
lxc delete "${name}"
Comment on lines +15 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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}
}
Comment on lines +31 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comments as above apply here.