Support deployment_config/working_dir value#356
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
| environment=_docker_configuration["environment"], | ||
| volumes=_docker_configuration["volumes"], | ||
| shm_size=_docker_configuration["shm_size"], | ||
| + privileged=_docker_configuration.get("privileged", False), |
There was a problem hiding this comment.
Any plan to bring these changes to ITF repo?
There was a problem hiding this comment.
Something similar is already there. Because today the itf_score version has been increased to 0.5.0, I reverted the patch.
| if "initial_run_target" in config or "initial_run_target" in merged_defaults: | ||
| new_config["initial_run_target"] = config.get( | ||
| "initial_run_target", merged_defaults.get("initial_run_target") | ||
| ) |
There was a problem hiding this comment.
I think this change it redundant. There is a validation that ensures that initial_run_target must always be defined (to Startup state)
| "working_dir": depl_cfg["working_dir"], | ||
| # Default the working directory to bin_dir (the directory the | ||
| # executable lives in) when not set explicitly. | ||
| "working_dir": depl_cfg.get("working_dir", depl_cfg["bin_dir"]), |
There was a problem hiding this comment.
This decision to use bin_dir as default exists in two different places, can this be moved to a single function (e.g. get_working_dir)?
| ], | ||
| "security_policy_details": "policy_name", | ||
| "number_of_restart_attempts": 1, | ||
| "working_dir": "/tmp", |
There was a problem hiding this comment.
Could you adapt one of the configs so that we have all cases covered?
- Taking the default working dir (as bin dir)
- Setting the working_dir in the defaults section
- Setting the working dir explicitly on the component (overwriting the default)
| @@ -35,6 +36,8 @@ def run_until_file_deployed( | |||
| :param file_path: path of the file to wait for on the target. | |||
| :param timeout_s: maximum seconds to wait for the file (default: 30). | |||
| :param poll_interval_s: seconds between file-existence checks (default: 0.5). | |||
There was a problem hiding this comment.
Maybe update the documentation that poll_interval_s is also used to poll process state
| time.sleep(poll_interval_s) | ||
|
|
||
| assert not proc.is_running(), ( | ||
| f"LCM still running {stop_timeout_s}s after SIGTERM" |
There was a problem hiding this comment.
What happens to other tests when the Process does not shutdown on receiving SIGTERM?
Is the process then killed with -9 to force termination so the next tests are not affected by this?
There was a problem hiding this comment.
I have added a kill -9 to not influence other tests.
| load("//tests/utils/bazel:integration.bzl", "integration_test") | ||
|
|
||
| exports_files( | ||
| ["lm_demo.json"], |
There was a problem hiding this comment.
I don't understand where is this file and why is it visible for examples folder?
There was a problem hiding this comment.
Me neither. Removed.
| case SCHED_FIFO: | ||
| return "SCHED_FIFO"; | ||
| case SCHED_RR: | ||
| return "SCHED_RR"; |
There was a problem hiding this comment.
I think SCHED_RR was not tested.
|
|
||
| const uid_t expected_uid = 666; | ||
| const gid_t expected_gid = 666; | ||
| const std::array<gid_t, 3> expected_supp_groups = {123, 321, 456}; |
There was a problem hiding this comment.
I think we cover at least the cases:
- No supplementary group ids configured
- Some supplementary group ids configured
| const uid_t expected_uid = 666; | ||
| const gid_t expected_gid = 666; | ||
| const std::array<gid_t, 3> expected_supp_groups = {123, 321, 456}; | ||
| const std::string expected_cwd = "/tmp"; |
There was a problem hiding this comment.
I think we should at least cover the cases:
- A specific working_dir configured
- The default working_dir is used
| { | ||
| bool all_pass = true; | ||
|
|
||
| const uid_t expected_uid = 666; |
There was a problem hiding this comment.
Can this be a reusable process, where you pass the expected values via CLI flags?
There was a problem hiding this comment.
Sure. Fixed.
| TEST_STEP("Activate RunTarget Startup") | ||
| { | ||
| score::cpp::stop_token stop_token; | ||
| auto result = client.ActivateRunTarget("Startup").Get(stop_token); |
There was a problem hiding this comment.
Launch Manager goes to Startup by default. I think this is redundant.
Looks like we can remove the whole control_daemon_mock from this test
| "gid": 666, | ||
| "supplementary_group_ids": [123, 321, 456], | ||
| "scheduling_policy": "SCHED_FIFO", | ||
| "scheduling_priority": 10 |
There was a problem hiding this comment.
I think we should also assert some default priority if not configured
Copied smoke test and renamed accordingly.
Allow execution of sched_setscheduler call by adding SYS_NICE capability to the docker environment
Some unit tests are currently broken however.
Before, when working_dir is not set it has been replaced with binary dir in c++ code. As this workaround is now done in python code, there is no need to keep this in c++ code.
With new version 0.5.0 this is no longer needed.
dad60c7 to
71264de
Compare
Two test modifications: - If no working_dir is defined, bin directory is used. - A working dir defined on deployment config level can be overridden on component level
if process does not react on SIGTERM
- add another process - parameterize sandbox_options_process.cpp
This fixes #343 .
Furthermore the config mapping tests have been fixed and are executed on the pipeline.
A feature integration test has been also added which is testing various options, not only working dir.