feat: Limit session types per scaling group #523
Conversation
…rbidden type's session
…lidation using trafaret.
vesselofgod
changed the title
Codecov Report
@@ Coverage Diff @@
## main #523 +/- ##
==========================================
+ Coverage 48.80% 48.88% +0.07%
==========================================
Files 54 54
Lines 9001 9185 +184
==========================================
+ Hits 4393 4490 +97
- Misses 4608 4695 +87
Continue to review full report at Codecov.
|
| schedulerOption = t.Dict( | ||
| { | ||
| tx.AliasedKey(['allowed_session_type', 'session_type', 'sessionType'], | ||
| default='interactive') >> 'allowed_session_type': tx.Enum(SessionTypes), | ||
| }).allow_extra('*') |
There was a problem hiding this comment.
This should be provided at the time of instantiation (i.e., the model field definitions), not hard-coded inside the class.
There was a problem hiding this comment.
At first, I received the trafaret object as a parameter of StructuredJSONBColumn, but when I tested it, an error occurred
.
There was a problem hiding this comment.
If you pass a trafaret object when instantiating the column class, it should work as expected.
Example:
sa.Column('scheduler_opts', StructuredJSONBColumn(
t.Dict({
t.Key('allowed_session_types', default=['interactive', 'batch']):
t.List(tx.Enum(SessionTypes)),
}).allow_extra('*'),
), nullable=False, default={}),There was a problem hiding this comment.
Also there is no need to use tx.AliasedKey because it is for legacy clients which use different names for parameter keys.
There was a problem hiding this comment.
When I edit source code, I referred to task_template_v1 in the session_template.py, but I didn't fix it according to the allowed_session_types...😥
There was a problem hiding this comment.
Thank you for the good point!
| sgroup_name = scheduled_session.scaling_group | ||
| async with self.db.begin() as conn: | ||
| query = ( | ||
| sa.select(scaling_groups.c.scheduler_opts) | ||
| .select_from(scaling_groups) | ||
| .where(scaling_groups.c.name == sgroup_name) | ||
| ) | ||
| result = await conn.execute(query) | ||
| row = result.first() | ||
| allowed_session_type = row['scheduler_opts']['allowed_session_type'] |
There was a problem hiding this comment.
The location of this check should be moved to PENDING -> SCHEDULED transition handler, not here in SCHEDULED -> PREPARING transition handler, to complete the decision before proceeding to the actual preparation step.
|
I've implemented the |
There was a problem hiding this comment.
Hm.. I think the check should be implemented as part of the check_scaling_group predicate.
This has the following advantages over the current implementation:
- The user becomes able to know why the session is pending as the reason can be recorded as a part of predicate check results.
- There no need to issue a separate database transaction. (Note: currently the newly added transaction uses
db.begin()but it should usedb.begin_readonly()because it contains only a single SELECT query.)
* feat: Check `allowed_session_type` in `scheduler_opts` during the scaling-group predicate check * feat: Add `StructuredJSONBColumn` to provide validation/conversion of JSONB columns using Trafarets. Backported-From: main (22.03) Backported-To: 21.09
resolves lablup/backend.ai#343
Related to: lablup/backend.ai-webui#1190
Limit session types per scaling group
Check allowed_session_type in scheduled_opts and don't start forbidden type's session
If it is not a allowed_session_type in scaling_group, do not start the session and stay preparing status
Add a column StructuredJSONBColumn to check scheduler_opts's validation using trafaret.
For validation check, modify "process_result_value" method (receive a result-row column value to be converted)