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

Project limits: Use InstanceList #14318

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Oct 21, 2024

Follow-up to #14315

This tidies up the queries used for project limits checking; it likely results in improved performance for clusters with many entities.

@MggMuggins MggMuggins force-pushed the use-instance-list branch 2 times, most recently from fa5e693 to 14d5679 Compare October 21, 2024 22:41
@MggMuggins
Copy link
Contributor Author

Had a weird error on the CI, unlikely to be related but I'll at least document it here:

=== RUN   TestEndpoints_NetworkSocketBasedActivation
    endpoints_test.go:54: 
        	Error Trace:	/home/runner/work/lxd/lxd/lxd/endpoints/endpoints_test.go:54
        	            				/home/runner/work/lxd/lxd/lxd/endpoints/network_test.go:66
        	Error:      	Received unexpected error:
        	            	readdirnames /tmp/lxd-endpoints-test-1872275927: readdirent lxd-endpoints-test-1872275927: bad file descriptor
        	Test:       	TestEndpoints_NetworkSocketBasedActivation
--- FAIL: TestEndpoints_NetworkSocketBasedActivation (0.00s)

"ToAPI" is also used for the database types (db.Instance), and in that
context it implies trips to the database rather than just shuffling
fields around. Happy to rename as needed.

Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
@tomponline
Copy link
Member

Error: Failed checking if instance creation allowed: Failed getting usage of project entities: Instance "c1" in project "test-usage" has no "limits.memory" config, either directly or via a profile

This at least gets us from `O(n)` queries to `O(1)` (where n is instances).
InstanceList does a lot of unnecessary faffing around with profiles that
ends up getting thrown out (looks like about 4 queries whose results end
up being dropped).

Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
@MggMuggins
Copy link
Contributor Author

MggMuggins commented Oct 23, 2024

InstanceList won't grab profiles that are not in use by any instances. We need all the profiles in the project so that new instances can expand their config/devices.

I've put up a revision here that at least gets us from O(n) queries to O(1) (where n is instances); it does a lot of unnecessary faffing around with profiles that ends up getting thrown out (looks like about 4 queries whose results end up being dropped).

What do you think of something like this?

// lxd/db/projects.go
type ProjectArgs struct {
	Project   api.Project
	Profiles  []api.Profile
	Instances []api.Instance
}

func (c *ClusterTx) ProjectsList(ctx context.Context, filters ...cluster.ProjectFilter) ([]ProjectArgs, error)

I've done a little fiddling here and have about 3/4 an implementation if you'd like me to pursue that further. Otherwise I'll get back to bugs.

@MggMuggins MggMuggins marked this pull request as ready for review October 24, 2024 03:28
@MggMuggins MggMuggins requested a review from tomponline October 24, 2024 03:28
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.

@MggMuggins after the improvements landed in #14315 do you feel this is still worthwhile, are you saying InstanceList is doing unnecessary things all of the time or just in this context?

@MggMuggins
Copy link
Contributor Author

The changes I've made here go from O(n) queries to O(1) queries for fetchProject; I definitely think this is still worthwhile.

It's inefficient because InstanceList is still doing a couple of queries to get profiles that fetchProject must repeat in order to get all the Project's profiles. In general InstanceList is fine; it's just not quite the right tool for fetchProject.

@tomponline tomponline merged commit ead3a69 into canonical:main Oct 24, 2024
27 checks passed
@MggMuggins MggMuggins deleted the use-instance-list branch October 24, 2024 14:06
tomponline added a commit that referenced this pull request Dec 9, 2024
The use of `InstanceList` I introduced in #14318 causes 3 queries
(`GetProfiles`, `GetConfig`, `GetDevices`) to be run twice. This
eliminates the duplication.

## TODO
- [x] Rename `ProjectsList` -> `GetProjectInstancesAndProfiles`
- [ ] Rename `ProjectArgs` -> `ProjectInstances` (`ProjectEntities`?
`ProjectInstancesAndProfiles`?, ...?)
- [x] Fix snapshots
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