-
Notifications
You must be signed in to change notification settings - Fork 37
issue-4734: added executable for writing pid to cgroup file #4735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cloud/blockstore/libs/endpoints_vhost/external_vhost_server.cpp
Outdated
Show resolved
Hide resolved
cloud/blockstore/libs/endpoints_vhost/external_vhost_server.cpp
Outdated
Show resolved
Hide resolved
cloud/blockstore/libs/endpoints_vhost/external_vhost_server.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| int main(int argc, char** argv) | ||
| { | ||
| if (argc < 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Давай сюда вставим Y_ABORT_UNLESS под макросом NDEBUG что путь в sys/fs/cgroup?
Тогда в тестах оно стрелять не будет
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
может достаточно будет написать тесты на функции хелперы? ведь бинарь только парсит входные аргументы и вызывает вспомогательные функции
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Путь можно в параметре передавать:
blockstore-cgroup-pid-writer --cgroup-base-path 'test-path/cgroup' ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
нужно какую-то защиту иметь что мы меняем только файлы внутри "/sys/fs/cgroup" а так если будем передавать --cgroup-base-path 'test-path/cgroup' снаружи, то любой сможет вызвать этот бинарь и поменять файлы в каких попало директориях
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit d074cb2.
🟢 linux-x86_64-relwithdebinfo: all tests PASSED for commit d074cb2.
|
72fbfeb to
cddacfb
Compare
|
|
||
| int main(int argc, char** argv) | ||
| { | ||
| if (argc < 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему бы не заиспользовать last_getopt.h ?
| @@ -0,0 +1,39 @@ | |||
| #include <cloud/blockstore/libs/common/cgroups_helpers.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не уверен, что ради двух мелких функций стоит это делать, особенно в common - можно просто накопипастить.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
нужно написать какие-то тесты, при этом написать интеграционные тесты, запускающие бинарь не получится, т.к. мы хотим зашить проверку внутрь бинаря на то, что изменяются только файлы внутри /sys/fs/cgroup
и передавать снаружи этот путь нельзя
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Передаешь тестовый путь через параметр --cgroup-root-path - пишешь тесты с запуском бинаря. По умолчанию путь на /sys/fs/cgroup - какие проблемы?
|
|
||
| int main(int argc, char** argv) | ||
| { | ||
| if (argc < 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Путь можно в параметре передавать:
blockstore-cgroup-pid-writer --cgroup-base-path 'test-path/cgroup' ...
cloud/blockstore/libs/endpoints_vhost/external_vhost_server.cpp
Outdated
Show resolved
Hide resolved
| if (ExternalCgroupsPidWriterBinaryPath) { | ||
| AddToCGroupsWithExternalExecutable( | ||
| ExternalCgroupsPidWriterBinaryPath, | ||
| process.Pid, | ||
| Cgroups); | ||
| } else { | ||
| AddToCGroups(process.Pid, Cgroups); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это лучше вынести, чтоб не раздувать StartProcess 2ca98f7
| @@ -0,0 +1,39 @@ | |||
| #include <cloud/blockstore/libs/common/cgroups_helpers.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Передаешь тестовый путь через параметр --cgroup-root-path - пишешь тесты с запуском бинаря. По умолчанию путь на /sys/fs/cgroup - какие проблемы?
#4734