feat(BA-3489): Implement ScalingGroup User Group Association Actions #7654
Conversation
github-actions
bot
added
size:L
jopemachine
changed the title
HyeockJinKim
force-pushed
the
main
branch
2 times, most recently
from
9552aac to
4af738e
Compare
jopemachine
force-pushed
the
feat/BA-3489
branch
from
70a6ad6 to
0de9aed
Compare
jopemachine
force-pushed
the
feat/BA-3489
branch
from
ee1fb3b to
07c80bc
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements actions for associating and disassociating scaling groups with user groups (projects), enabling management of scaling group-to-project relationships through the repository and service layers.
Key Changes:
- Added
AssociateScalingGroupWithUserGroupActionandDisassociateScalingGroupWithUserGroupActionclasses with corresponding action results - Implemented repository methods for association, disassociation, and checking association existence
- Added comprehensive unit tests for both service and repository layers covering success and edge cases
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/services/scaling_group/actions/associate_with_user_group.py |
New action class for associating scaling groups with user groups |
src/ai/backend/manager/services/scaling_group/actions/disassociate_with_user_group.py |
New action class for disassociating scaling groups from user groups |
src/ai/backend/manager/services/scaling_group/service.py |
Added service methods to handle association/disassociation operations |
src/ai/backend/manager/services/scaling_group/processors.py |
Registered new action processors for the association operations |
src/ai/backend/manager/repositories/scaling_group/repository.py |
Added repository interface methods for association operations and existence checks |
src/ai/backend/manager/repositories/scaling_group/db_source/db_source.py |
Implemented database operations for association management |
src/ai/backend/manager/repositories/scaling_group/creators.py |
Added ScalingGroupForProjectCreatorSpec for creating associations |
src/ai/backend/manager/repositories/scaling_group/purgers.py |
New file with batch purger implementation for removing associations |
tests/unit/manager/services/scaling_group/test_scaling_group_service.py |
Added service layer tests for association/disassociation actions |
tests/unit/manager/repositories/scaling_group/test_scaling_group_repository.py |
Added repository tests verifying database operations and edge cases |
changes/7654.feature.md |
Changelog entry documenting the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def test_associate_scaling_group_with_user_group_success( | ||
| self, | ||
| scaling_group_repository: ScalingGroupRepository, | ||
| sample_scaling_group_for_purge: str, | ||
| test_user_domain_group: tuple[uuid.UUID, str, uuid.UUID], | ||
| ) -> None: | ||
| """Test associating a scaling group with a user group (project).""" | ||
| # Given: A scaling group and a project (group) | ||
| sgroup_name = sample_scaling_group_for_purge | ||
| _, _, project_id = test_user_domain_group | ||
|
|
||
| # When: Associate the scaling group with the project | ||
| creator = Creator( | ||
| spec=ScalingGroupForProjectCreatorSpec( | ||
| scaling_group=sgroup_name, | ||
| project=project_id, | ||
| ) | ||
| ) | ||
| await scaling_group_repository.associate_scaling_group_with_user_group(creator) | ||
|
|
||
| # Then: Association should exist | ||
| association_exists = ( | ||
| await scaling_group_repository.check_scaling_group_user_group_association_exists( | ||
| scaling_group=sgroup_name, | ||
| user_group=project_id, | ||
| ) | ||
| ) | ||
| assert association_exists is True |
There was a problem hiding this comment.
There is no test case to verify the behavior when attempting to associate a scaling group with a user group that is already associated. Given the unique constraint on the (scaling_group, group) columns in ScalingGroupForProjectRow, attempting to create a duplicate association should be tested to ensure proper error handling.
| @@ -0,0 +1 @@ | |||
| Implement ScalingGroup User Group Association, Disassociation Actions | |||
There was a problem hiding this comment.
The changelog filename uses issue number 7654, but the PR description indicates this resolves issue #7489 (BA-3489). The filename should match the issue number referenced in the PR.
| Implement ScalingGroup User Group Association, Disassociation Actions | |
| #7489 (BA-3489): Implement ScalingGroup User Group Association and Disassociation actions |
| ) -> None: | ||
| """Associates a single scaling group with a user group (project).""" | ||
| async with self._db.begin_session() as session: | ||
| await execute_creator(session, creator) |
There was a problem hiding this comment.
The associate_scaling_group_with_user_group method doesn't handle IntegrityError exceptions that would occur when trying to create a duplicate association. The ScalingGroupForProjectRow table has a unique constraint on (scaling_group, group), so attempting to associate the same scaling group with the same project twice will raise an IntegrityError. This should be caught and converted to a more meaningful domain-specific exception, similar to how the create_scaling_group method handles duplicate scaling group names.
| await execute_creator(session, creator) | |
| try: | |
| await execute_creator(session, creator) | |
| except sa.exc.IntegrityError: | |
| # Convert duplicate association DB error into a domain-specific conflict. | |
| raise ScalingGroupConflict( | |
| "Scaling group is already associated with the specified user group." | |
| ) |
resolves #7489 (BA-3489)
Checklist: (if applicable)
ai.backend.testdocsdirectory