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

Optimize Project Queries during limits checks #14369

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Oct 28, 2024

The use of InstanceList I introduced in #14318 causes 3 queries (GetProfiles, GetConfig, GetDevices) to be run twice. This eliminates the duplication.

TODO

  • Rename ProjectsList -> GetProjectInstancesAndProfiles
  • Rename ProjectArgs -> ProjectInstances (ProjectEntities? ProjectInstancesAndProfiles?, ...?)
  • Fix snapshots

@MggMuggins MggMuggins force-pushed the db-project-args branch 2 times, most recently from 434585e to e992a7e Compare October 29, 2024 23:23
@MggMuggins
Copy link
Contributor Author

I'm thinking the test failure is some kind of transient issue with dqlite:

Tests / System (cluster, dir) + lxc query /internal/testing/image-refresh ++ timeout --foreground 120 /home/runner/go/bin/lxc query /internal/testing/image-refresh --verbose ++ timeout --foreground 120 /home/runner/go/bin/lxc query /internal/testing/image-refresh --verbose ++ timeout --foreground 120 /home/runner/go/bin/lxc query /internal/testing/image-refresh --verbose time="2024-10-29T23:44:17Z" level=info msg="Downloading image" alias=testimage server="https://10.1.1.104:8443/" time="2024-10-29T23:44:17Z" level=info msg="Image downloaded" alias=testimage server="https://10.1.1.104:8443/" time="2024-10-29T23:44:17Z" level=error msg="Failed to distribute new image" err="close /home/runner/work/lxd/lxd/test/tmp.V1Y/dzG/images/7a09032a8f839cbadc8a2d91f1e8bfab7b34316288144679b93f3bbffaca1a69: file already closed" fingerprint=7a09032a8f839cbadc8a2d91f1e8bfab7b34316288144679b93f3bbffaca1a69 time="2024-10-29T23:44:17Z" level=error msg="Error deleting old image from database" ID=1 err="database is locked" fingerprint=426bf246bc8a7a08446f4636c3665742db1930845a3091cd7916094e8b81a30a time="2024-10-29T23:44:17Z" level=error msg="Error deleting old image from database" ID=3 err="database is locked" fingerprint=426bf246bc8a7a08446f4636c3665742db1930845a3091cd7916094e8b81a30a + for pid in ${pids} + wait 26165 + for pid in ${pids} + wait 26164 + '[' dir '!=' dir ']' + LXD_DIR=/home/runner/work/lxd/lxd/test/tmp.V1Y/OCo + lxd sql global 'select images.fingerprint from images join projects on images.project_id=projects.id where projects.name="foo"' + grep -F 426bf246bc8a | 426bf246bc8a7a08446f4636c3665742db1930845a3091cd7916094e8b81a30a | ++ LXD_DIR=/home/runner/work/lxd/lxd/test/tmp.V1Y/OCo ++ lxd sql global 'select images.fingerprint from images' ++ grep -cF 426bf246bc8a + '[' 3 -eq 1 ']'

@MggMuggins MggMuggins marked this pull request as ready for review October 30, 2024 19:10
lxd/db/instances.go Outdated Show resolved Hide resolved
lxd/db/instances.go Outdated Show resolved Hide resolved
lxd/db/instances.go Outdated Show resolved Hide resolved
lxd/db/instances.go Outdated Show resolved Hide resolved
// GetProjectInstancesAndProfiles gets all instances and profiles associated
// with some projects in as few queries as possible.
// Returns a map[projectName]args.
func (c *ClusterTx) GetProjectInstancesAndProfiles(ctx context.Context, projects ...*api.Project) (map[string]*ProjectArgs, error) {
Copy link
Member

Choose a reason for hiding this comment

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

As maps are returned by reference, is there a reason the map stores a *ProjectArgs and not just ProjectArgs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a great one; it allows this:

// line 106
projectArgs[profile.Project].Profiles = append(projectArgs[profile.Project].Profiles, *apiProfile)

as opposed to

args := projectArgs[profile.Project]
args.Profiles = append(args.Profiles, *apiProfile)
projectArgs[profile.Project] = args

since map keys aren't addressable. Not sure if the compiler would optimize that or not; I only really care about it being a bit more concise.

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Lets use the context where possible.

This allows us to use the logic from instanceProfilesFill AND use a custom
profiles filter so that we don't have to go through every profile in the
database.

Signed-off-by: Wesley Hershberger <[email protected]>
This eliminates 3 Profile queries that InstanceList was also doing here.

Signed-off-by: Wesley Hershberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants