fix: Migrate persistent kernel stats to Redis to reduce database txn overheads #532

kyujin-cho · 2022-02-14T10:02:50Z

codecov · 2022-02-14T10:03:25Z

Codecov Report

Merging #532 (3c86a82) into main (02bf6a9) will decrease coverage by 0.17%.
The diff coverage is 35.05%.

@@            Coverage Diff             @@
##             main     #532      +/-   ##
==========================================
- Coverage   49.37%   49.20%   -0.18%     
==========================================
  Files          55       55              
  Lines        9008     9079      +71     
==========================================
+ Hits         4448     4467      +19     
- Misses       4560     4612      +52

Impacted Files	Coverage Δ
src/ai/backend/manager/config.py	`63.73% <ø> (ø)`
src/ai/backend/manager/registry.py	`18.07% <0.00%> (-0.14%)`	⬇️
src/ai/backend/manager/models/kernel.py	`44.60% <25.92%> (+0.42%)`	⬆️
src/ai/backend/manager/cli/__main__.py	`47.54% <34.37%> (-6.23%)`	⬇️
src/ai/backend/manager/cli/context.py	`70.37% <53.33%> (-21.94%)`	⬇️
src/ai/backend/manager/models/utils.py	`78.01% <0.00%> (-5.68%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02bf6a9...3c86a82. Read the comment docs.

achimnol

Just two minor code issues.

src/ai/backend/manager/api/resource.py

fregataa

leave a minor comment.

fregataa · 2022-03-03T08:59:14Z

src/ai/backend/manager/api/resource.py

@@ -311,12 +314,23 @@ async def get_container_stats_for_period(request: web.Request, start_date, end_d
        result = await conn.execute(query)
        rows = result.fetchall()

+    async def _pipe_builder(r: Redis) -> RedisPipeline:


굉장히 사소하긴 하지만, 여기 async가 없어도 되나요?

good catch 👍

No, that's wrong;;;
https://github.com/lablup/backend.ai-common/blob/main/src/ai/backend/common/redis.py#L228-L234
It should return just pipeline, not awaitable of pipeline!

achimnol

common.redis.execute() accepts a function that returns either:

async function that executes Redis commands
non-async function that returns a Redis pipeline to execute

To use pipeline, the callable must NOT be async.

refs: https://github.com/lablup/backend.ai-common/blob/3a6466fe/src/ai/backend/common/redis.py#L228-L234

achimnol · 2022-03-21T17:04:10Z

common.redis.execute() accepts a function that returns either:

async function that executes Redis commands

non-async function that returns a Redis pipeline to execute

To use pipeline, the callable must NOT be async.

refs: https://github.com/lablup/backend.ai-common/blob/3a6466fe/src/ai/backend/common/redis.py#L228-L234

I just added a PR to allow async pipeline builders: lablup/backend.ai-common#125.

* Statistics are stored and queried per-container (=per-kernel), not per-session. * SessionStatistics should be implemented separately by aggregating its belong KernelStatistics.

achimnol

I think it's better to merge redis clear-history with clear-history command. How about your thoughts?

* Introduce more-itertools as a new dependency * Use attr-class instead of typed dict - When there is no need for serialization, it's generally faster and safer to use typed attr-classes or dataclasses. * Adjust the log message for Redis stats records cleanup.

kyujin-cho added 2 commits February 14, 2022 19:00

Do not store stat data on postgres db

07135b1

add new dataloader resolver which resolves container stat from redis

921838f

kyujin-cho added the refactor label Feb 14, 2022

kyujin-cho added this to the 21.09 milestone Feb 14, 2022

kyujin-cho self-assigned this Feb 14, 2022

kyujin-cho added 4 commits February 14, 2022 19:03

Fix lint error

50f36b3

add description at sample config file

3da694c

add towncrier report

c842528

rename towncrier report

abbe147

achimnol suggested changes Mar 2, 2022

View reviewed changes

src/ai/backend/manager/api/resource.py Outdated Show resolved Hide resolved

src/ai/backend/manager/api/resource.py Outdated Show resolved Hide resolved

kyujin-cho added 5 commits March 3, 2022 10:34

Merge branch 'main' into refactor/stats-on-redis

f76fced

accept PR review

f4c9119

add redis clear-history command

7685a9f

Remove debug calls

a9cee85

Fix typo

ddd50ba

fregataa reviewed Mar 3, 2022

View reviewed changes

fix pipe builder returning awaitable

ae95a52

kyujin-cho requested review from fregataa and achimnol March 6, 2022 05:12

achimnol suggested changes Mar 21, 2022

View reviewed changes

Merge branch 'main' into refactor/stats-on-redis

51289ad

achimnol modified the milestones: 21.09, 22.03 Mar 28, 2022

achimnol added 5 commits March 28, 2022 07:56

refactor: Rename the debug option name

3639013

fix: Remove duplicate line

52d9400

refactor: Apply batching when querying KernelStatistics

356b1bd

Merge branch 'main' into refactor/stats-on-redis

b335ff6

Merge branch 'main' into refactor/stats-on-redis

769e485

achimnol added 2 commits March 28, 2022 16:21

fix: lint/type error

0de7be3

refactor: Rename by_session to by_kernel

b2bcf5d

* Statistics are stored and queried per-container (=per-kernel), not per-session. * SessionStatistics should be implemented separately by aggregating its belong KernelStatistics.

achimnol suggested changes Mar 28, 2022

View reviewed changes

kyujin-cho and others added 5 commits March 29, 2022 12:12

merge redis clear-history command to clear-history command

4692dcd

fix: Remove no longer used command group

f52adc1

docs: Update news fragment

2cb6035

fix: Store last_stat when kernels are terminated as a backup

3c86a82

achimnol approved these changes Mar 29, 2022

View reviewed changes

achimnol changed the title ~~refactor: store stat data on redis only~~ refactor: Migrate persistent kernel stats to Redis to reduce database txn overheads Mar 29, 2022

achimnol changed the title ~~refactor: Migrate persistent kernel stats to Redis to reduce database txn overheads~~ fix: Migrate persistent kernel stats to Redis to reduce database txn overheads Mar 29, 2022

achimnol merged commit 17158e7 into main Mar 29, 2022

achimnol deleted the refactor/stats-on-redis branch March 29, 2022 08:19

achimnol mentioned this pull request Mar 31, 2022

Add Redis persistent DB mgmt commands when clearing history #565

Merged

achimnol mentioned this pull request Apr 12, 2023

Improve scalability of manager with many agents and sessions (more than hundreds) lablup/backend.ai#1222

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Migrate persistent kernel stats to Redis to reduce database txn overheads #532

fix: Migrate persistent kernel stats to Redis to reduce database txn overheads #532

kyujin-cho commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading

achimnol left a comment

fregataa left a comment

fregataa Mar 3, 2022

kyujin-cho Mar 3, 2022

achimnol Mar 21, 2022

achimnol left a comment •

edited

Loading

achimnol commented Mar 21, 2022

achimnol left a comment

fix: Migrate persistent kernel stats to Redis to reduce database txn overheads #532

fix: Migrate persistent kernel stats to Redis to reduce database txn overheads #532

Conversation

kyujin-cho commented Feb 14, 2022 • edited Loading

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

achimnol left a comment

Choose a reason for hiding this comment

fregataa left a comment

Choose a reason for hiding this comment

fregataa Mar 3, 2022

Choose a reason for hiding this comment

kyujin-cho Mar 3, 2022

Choose a reason for hiding this comment

achimnol Mar 21, 2022

Choose a reason for hiding this comment

achimnol left a comment • edited Loading

Choose a reason for hiding this comment

achimnol commented Mar 21, 2022

achimnol left a comment

Choose a reason for hiding this comment

kyujin-cho commented Feb 14, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading

achimnol left a comment •

edited

Loading