Skip to content

fix(BA-2788): Missing session type check#6354

Merged
HyeockJinKim merged 3 commits into
mainfrom
fix/missing-session-type-check
Oct 23, 2025
Merged

fix(BA-2788): Missing session type check#6354
HyeockJinKim merged 3 commits into
mainfrom
fix/missing-session-type-check

Conversation

@fregataa
Copy link
Copy Markdown
Member

@fregataa fregataa commented Oct 23, 2025 •
edited
Loading

resolves #6352 (BA-2788)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

@fregataa fregataa added this to the 25.15 milestone Oct 23, 2025
@fregataa fregataa requested a review from HyeockJinKim October 23, 2025 08:10
@fregataa fregataa self-assigned this Oct 23, 2025
Copilot AI review requested due to automatic review settings October 23, 2025 08:10
@github-actions github-actions Bot added size:L 100~500 LoC comp:manager Related to Manager component labels Oct 23, 2025
Copilot AI reviewed Oct 23, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements validation for session types against scaling group configurations. The change ensures that sessions can only be created with session types that are explicitly allowed by the target scaling group's configuration.

  • Adds a new SessionTypeRule validator to check if the requested session type is permitted by the scaling group
  • Integrates the new validation rule into the scheduling controller's validation pipeline
  • Includes comprehensive test coverage for both allowed and disallowed session type scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/ai/backend/manager/sokovan/scheduling_controller/validators/rules.py Implements SessionTypeRule validator and adds @override decorators to existing rules
src/ai/backend/manager/sokovan/scheduling_controller/validators/__init__.py Exports the new SessionTypeRule class
src/ai/backend/manager/sokovan/scheduling_controller/scheduling_controller.py Registers SessionTypeRule in the validator pipeline
tests/manager/sokovan/scheduling_controller/validators/test_rules.py Adds test cases for SessionTypeRule and a session spec factory fixture

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/ai/backend/manager/sokovan/scheduling_controller/validators/rules.py
Comment on lines +94 to +95


Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The validation doesn't return after finding a matching scaling group. If the session type is allowed, the loop continues unnecessarily and the method doesn't explicitly return. Add an explicit return after line 93, or add a break after line 93 and a return after the loop completes successfully.

Suggested change
return

Copilot uses AI. Check for mistakes.
HyeockJinKim
HyeockJinKim reviewed Oct 23, 2025
View reviewed changes
Comment thread src/ai/backend/manager/sokovan/scheduling_controller/validators/rules.py
Comment on lines +87 to +94
for sg in allowed_groups:
if sg.name == spec.scaling_group:
allowed_session_types = sg.scheduler_opts.allowed_session_types
if spec.session_type not in allowed_session_types:
raise InvalidAPIParameters(
f"Session type {spec.session_type} is not allowed in scaling group {sg.name}"
)

Copy link
Copy Markdown
Collaborator

@HyeockJinKim HyeockJinKim Oct 23, 2025

Choose a reason for hiding this comment

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

If the spec's scaling group does not exist in allow list, an error should be raised.

@HyeockJinKim HyeockJinKim added this pull request to the merge queue Oct 23, 2025
Merged via the queue into main with commit dbed927 Oct 23, 2025
28 checks passed
@HyeockJinKim HyeockJinKim deleted the fix/missing-session-type-check branch October 23, 2025 08:48
lablup-octodog pushed a commit that referenced this pull request Oct 23, 2025
@fregataa
fix(BA-2788): Missing session type check (#6354)
87efcae 
Backported-from: main (25.16)
Backported-to: 25.15
Backport-of: 6354
@lablup-octodog lablup-octodog mentioned this pull request Oct 23, 2025
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Oct 23, 2025
@lablup-octodog @fregataa
fix(BA-2788): Missing session type check (#6354) (#6356)
36e5e21 
Co-authored-by: Sanghun Lee <sanghun@lablup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@HyeockJinKim HyeockJinKim HyeockJinKim approved these changes

Assignees

@fregataa fregataa

Labels

comp:manager Related to Manager component size:L 100~500 LoC

Projects

None yet

Milestone

25.15

Development

Successfully merging this pull request may close these issues.

Session type is not checked when creating

3 participants

@fregataa @HyeockJinKim