Optimize Project Queries during limits checks#14369
Merged
tomponline merged 5 commits intocanonical:mainfrom Dec 9, 2024
Merged
Optimize Project Queries during limits checks#14369tomponline merged 5 commits intocanonical:mainfrom
tomponline merged 5 commits intocanonical:mainfrom
Conversation
434585e to
e992a7e
Compare
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 ']'
|
tomponline
reviewed
Nov 5, 2024
tomponline
reviewed
Nov 5, 2024
tomponline
reviewed
Nov 5, 2024
tomponline
reviewed
Nov 5, 2024
tomponline
reviewed
Nov 5, 2024
| // 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) { |
Member
There was a problem hiding this comment.
As maps are returned by reference, is there a reason the map stores a *ProjectArgs and not just ProjectArgs?
Contributor
Author
There was a problem hiding this comment.
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] = argssince 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.
tomponline
requested changes
Nov 5, 2024
Member
There was a problem hiding this comment.
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 <wesley.hershberger@canonical.com>
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
This eliminates 3 Profile queries that InstanceList was also doing here. Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
e992a7e to
211ee91
Compare
Member
|
@MggMuggins can we close this for now until you get chance to look at this again? |
Contributor
Author
|
This is ready to merge as far as I'm concerned, unless you'd like to rename |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The use of
InstanceListI introduced in #14318 causes 3 queries (GetProfiles,GetConfig,GetDevices) to be run twice. This eliminates the duplication.TODO
ProjectsList->GetProjectInstancesAndProfilesProjectArgs->ProjectInstances(ProjectEntities?ProjectInstancesAndProfiles?, ...?)