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: Add lxd/project/limits and fix PowerFlex instance size limits #13994

Merged
merged 13 commits into from
Sep 4, 2024

Conversation

MggMuggins
Copy link
Contributor

@MggMuggins MggMuggins commented Aug 27, 2024

Fixes #12567

Moves some the contents of lxd/project into a new module lxd/project/limits to remove an import cycle lxd/project -> lxd/storage/drivers -> lxd/project.

lxd/project split fairly cleanly in two; I tried to leave as much project-related API in lxd/project as I could. Most of what is left there is the string processing code for prepending instance/volume names with the project name. This is used fairly heavily in lxd/storage/drivers.

lxd/project/limits now contains just the code for enforcing project limits; the change in usage reads better IMO:

project.AllowInstanceCreation(s.GlobalConfig, tx, projectName, req)
// to 
limits.AllowInstanceCreation(s.GlobalConfig, tx, projectName, req)

This change allows us to import the storage drivers from lxd/storage/drivers in lxd/project/limits to get DefaultVMBlockFilesystemSize.

This introduces a new method to the internal driver interface (defaultVMBlockFilesystemSize() string) and exposes a function from lxd/storage/drivers to convert a driver name to the DefaultVMBlockFilesystemSize.

@MggMuggins MggMuggins changed the title Limits from drivers Introduce lxd/limits and fix instance size limits Aug 27, 2024
@MggMuggins MggMuggins changed the title Introduce lxd/limits and fix instance size limits Introduce lxd/limits and fix PowerFlex instance size limits Aug 27, 2024
@MggMuggins MggMuggins force-pushed the limits-from-drivers branch from a55def5 to 0b36059 Compare August 27, 2024 22:56
@tomponline
Copy link
Member

I'm happy to put up another PR against stable-5.21 with the "wrong way" to fix #12567 (simply checking against a literal "powerflex" in lxd/project) if that would be preferable as a backport.

I'll backport these changes as part of the normal process as they dont change the API or the DB.

@MggMuggins MggMuggins force-pushed the limits-from-drivers branch from 02644b5 to a0917f2 Compare August 28, 2024 20:24
@MggMuggins MggMuggins requested a review from roosterfish August 28, 2024 20:27
@MggMuggins MggMuggins force-pushed the limits-from-drivers branch 5 times, most recently from 4fae4c3 to 3a943fe Compare August 28, 2024 22:57
@MggMuggins MggMuggins marked this pull request as ready for review August 29, 2024 00:21
@MggMuggins MggMuggins requested a review from tomponline August 29, 2024 00:21
@MggMuggins
Copy link
Contributor Author

I've updated the PR description, made the requested change, and reworked how the default size was being passed around.

lxd/project/limits/permissions.go Outdated Show resolved Hide resolved
lxd/storage/drivers/load_test.go Show resolved Hide resolved
@tomponline tomponline changed the title Introduce lxd/limits and fix PowerFlex instance size limits Project: Add lxd/project/limits and fix PowerFlex instance size limits Aug 29, 2024
@MggMuggins MggMuggins requested a review from roosterfish August 29, 2024 20:09
@MggMuggins MggMuggins force-pushed the limits-from-drivers branch 3 times, most recently from f5b84aa to 8bb252c Compare August 30, 2024 17:07
@MggMuggins
Copy link
Contributor Author

@roosterfish @tomponline This is ready to go again

roosterfish
roosterfish previously approved these changes Sep 2, 2024
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

LGTM!

lxd/project/limits/permissions.go Outdated Show resolved Hide resolved
roosterfish
roosterfish previously approved these changes Sep 4, 2024
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good to me.

tomponline
tomponline previously approved these changes Sep 4, 2024
@tomponline
Copy link
Member

Thanks @MggMuggins please can you rebase to fix the conflict?

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
These should be grabbed via the device driver

Signed-off-by: Wesley Hershberger <[email protected]>
Since this function only return the distinct drivers, make way for
GetStoragePoolDrivers to give a mapping of poolName:driverName.

Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
Signed-off-by: Wesley Hershberger <[email protected]>
We can't enforce that the defaultVMBlockFilesystemSize method doesn't
use values in the driver before the driver is loaded. This will at least
catch that usage in the test suite.

Signed-off-by: Wesley Hershberger <[email protected]>
@MggMuggins MggMuggins dismissed stale reviews from tomponline and roosterfish via 969efa3 September 4, 2024 14:29
@MggMuggins
Copy link
Contributor Author

@tomponline Done

@tomponline tomponline merged commit f969631 into canonical:main Sep 4, 2024
29 checks passed
@MggMuggins MggMuggins deleted the limits-from-drivers branch September 4, 2024 16:07
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.

Update projects instance limit accounting
3 participants