awslabs / damo

DAMON user-space tool
https://damonitor.github.io/
GNU General Public License v2.0
155 stars 26 forks source link

Support target_nid for migrate actions #100

Closed yunjeongmun closed 3 months ago

yunjeongmun commented 3 months ago

This pull request includes 2 patches that support 'target_nid' for 'migrate_hot' and 'migrate_cold' actions.

The first patch adds 'target_nid' under scheme to represent the migration target node ID for new DAMOS actions, migrate_hot and migrate_cold, have been introduced. The second patch extends the '--damos_action' to ensure that the node number follows when the action is 'migration_hot' or 'migration_cold'. The usage is as follows:

$ sudo damo start --damos_action migrate_hot 0
$ sudo damo start --damos_action migrate_cold 1

The other non migrate actions do not require the extra argument as is.

$ sudo damo start --damos_action pageout
honggyukim commented 3 months ago

Hi SeongJae,

This is related to #99 to support target_nid. Please have a look. Thanks!

sjp38 commented 3 months ago

Thank you for kindly double checking my humble comments. Nonetheless, I found this PR makes unit tests fail. I was lucky to find the root cause relatively easy, so just fixed it by myself: https://github.com/awslabs/damo/commit/79d0d69896815087e801b18e789d7b12edc10a83

Anyway, pushed this PR together with the fix. Thank you again for your great contribution!

honggyukim commented 3 months ago

Thanks for the review. We didn't know about the unittest, but it'd be helpful if you could tell us how to run the unittest.

In addition, we might be better to consider adding a GitHub action that runs the unittest automatically so that we can check the status before merging PRs.

sjp38 commented 3 months ago

how to run the unittest.

You can run it by

$ ./tests/unit/test.sh

Also, it runs as part of the damo testing: https://github.com/awslabs/damo/blob/next/CONTRIBUTING#L29

I now realize the document is suggesting running it for only finding issues. It would better to be encouraged to run for every PR if possible. I will try to update the document for that.

we might be better to consider adding a GitHub action that runs the unittest

I have no strong opinion here, mainly because damo maintenance overhead is not that big for now.

yunjeongmun commented 3 months ago

I'm so glad to contribute this project! Thanks for reviewing and fixing the bug :)

honggyukim commented 3 months ago

how to run the unittest.

You can run it by

$ ./tests/unit/test.sh

Thanks. I found that the test fails as you explained without having your fix.

$ ./tests/unit/test.sh 
PASS unit test_damo_records.py
PASS unit test_damo_scheme_dbgfs_conversion.py
FAIL unit test_damo_schemes_input.py

Also, it runs as part of the damo testing: https://github.com/awslabs/damo/blob/next/CONTRIBUTING#L29

Sorry, I didn't read it carefully.

I now realize the document is suggesting running it for only finding issues. It would better to be encouraged to run for every PR if possible. I will try to update the document for that.

we might be better to consider adding a GitHub action that runs the unittest

I have no strong opinion here, mainly because damo maintenance overhead is not that big for now.

I will think about it when I can find some time for this extra work.

honggyukim commented 3 months ago

we might be better to consider adding a GitHub action that runs the unittest

I have no strong opinion here, mainly because damo maintenance overhead is not that big for now.

I have made automatic unit test run setup whenever making a commit using pre-commit. Please see #102.