Shell command - #1585
Conversation
|
Required keyword not found in PR title or description. |
Eco CI Output - Old Energy EstimationEco CI Output [RUN-ID: 22398142951]: 🌳 CO2 Data: Total cost of whole PR so far:
|
|||||||||||||||||||||||||||||||||||
|
An interesting piece. I am not quite clear what the current idea is for this tool as it can only be run in non cluster mode and thus will have skewed measurements by default. Would help me better judge it if you could explain the use case you are seeing. As if I really want to just measure a small script on my machine, why do I not just use codecarbon? Having said I can the the implementation of this not too far away. From the design of the code I would increase re-usage in some locations:
Currently both methods contain too much duplicate code that can easily get out of sync without necessarily breaking. But as said: Understanding that this is a concept I see the implementation not too far away. If we find some good use cases I can see this in GMT. Currently I do not understand that though :) |
|
So right now I am using it for the benchmarking of the new X tooling. I need a comparison of the overhead of the docker containers. I am aware, that the measurements will not be 100% but I want to get an idea of degrees of. I also want to use the GMT compare functionality to easily look at how things differ. I can run the job 5 times and then get a good idea how big the jitter is (Not very large on my Laptop if I kill most processes. Under 2%) I discard runs when some cron script started or there is one job which is out of line. Then I very often have something and I just want to get an idea of how much resources it uses but don't want to go the full docker way. CodeCarbon is great but I am used to the GMT tooling and I want the dashboard so I can compare and dig deeper. Not all things need to be 100%, especially if need something fast to give me an idea if it is worth investing my time. To be merged this needs some work, I agree. This is only something I am using privately and I wanted to share as I finally had the time to clean it up a little. I didn't want to touch anything in the "core" GMT as if this is not merged I will have to maintain it off record and I don't want merge conflicts. That is why a lot of the logic is in the shells file and not in the "core" code. It is always the discussion: ease of use vs correct measurements. |
|
This will be kept open and re-worked when we need it again. Effectively when we think this bigger:
|
|
Eco CI Output [RUN-ID: 29749556648]:
🌳 CO2 Data: Total cost of whole PR so far: |
|||||||||||||||||||||||||||||||||||
…n-coding-solutions/green-metrics-tool into shell-command
|
@ArneTR so this PR has changed a little in that the GMT now also supports host mode execution. It also keeps the shell command which is now just a wrapper that creates a usage scenario with the command wrapped. Ready for initial review |
ArneTR
left a comment
ArneTR
left a comment
There was a problem hiding this comment.
Code looks good. Did not check the tests or the shell script though.
Now firing up AI
| if self._allow_unsafe: | ||
| print(TerminalColors.WARNING, arrows('Warning: Runner is running in unsafe mode'), TerminalColors.ENDC) | ||
|
|
||
| host_flows = [flow['name'] for flow in self.__usage_scenario.get('flow', []) if flow_runs_on_host(flow)] |
There was a problem hiding this comment.
The maintainability would be better with a any() and not User(...) if clause. Using first a list comprehension and then a 2-level if clouds this intent for me as a reader
| if not metric_provider._metric_name.endswith('_container') and metric_provider._metric_name != 'network_connections_tcpdump_system' and not allow_other: | ||
| continue | ||
|
|
||
| if metric_provider._metric_name.endswith('_container') and not self.__containers: |
There was a problem hiding this comment.
maybe also a warning that you see in the UI later? Or do you feel that would get to verbose?
| if not metric_provider._metric_name.endswith('_container') and metric_provider._metric_name != 'network_connections_tcpdump_system' and not allow_other: | ||
| continue | ||
|
|
||
| if metric_provider._metric_name.endswith('_container') and not self.__containers: # was skipped and never booted |
There was a problem hiding this comment.
Thinking out loud: May moving the providers dynamically to the "Disabled Providers" array? We have that already as manually configurable but could also use it for this use case ...?
| self.__notes_helper.add_note( note=cmd_obj['note'], detail_name=flow_detail_name, timestamp=int(time.time_ns() / 1_000)) | ||
|
|
||
| print(TerminalColors.HEADER, '\nExecuting ', cmd_obj['type'], 'command on container', flow['container'], TerminalColors.ENDC) | ||
| if run_on_host: |
There was a problem hiding this comment.
Simplify to print(TerminalColors.HEADER, '\nExecuting ', cmd_obj['type'], 'command on', flow_detail_name, TerminalColors.ENDC)
|
|
||
| print(TerminalColors.HEADER, '\nRunning flow: ', flow['name'], TerminalColors.ENDC) | ||
|
|
||
| run_on_host = flow_runs_on_host(flow) |
There was a problem hiding this comment.
why not call it runs_on_host with s just like the function name?
| if cmd_obj['type'] == 'playwright': | ||
| docker_exec_command.append(cmd_obj.get('shell', 'sh')) | ||
| docker_exec_command.append('-ec') | ||
| if run_on_host: # should already be caught by the schema checker. Last line of defense |
There was a problem hiding this comment.
debateable. I would rather prefer a test for that.
I know we have that in multiple places these double guards, but in the end they pollute the code
| VALID_CHARS_SPACE = VALID_CHARS.copy() | ||
| VALID_CHARS_SPACE.add(' ') | ||
|
|
||
| def flow_runs_on_host(flow): |
There was a problem hiding this comment.
I would not go for python objects here but stick to the YAML syntax.
Either the value is omitted or it is null without quotes. that produces an actual python None
| runs_on_host = flow_runs_on_host(flow) | ||
|
|
||
| for command in flow['commands']: | ||
| if runs_on_host and command['type'] != 'console': |
There was a problem hiding this comment.
I would also rather do a test that schema checker works as expected here than an extra guard in code
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe changes add host-executed flow support with schema validation, capability authorization, host-specific shell argv construction, and container-provider skipping. A standalone 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)tests/data/usage_scenarios/host_execution.ymlTraceback (most recent call last): tests/data/usage_scenarios/host_execution_mixed.ymlTraceback (most recent call last): tests/data/usage_scenarios/schema_checker/schema_checker_invalid_host_execution_playwright.ymlTraceback (most recent call last):
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cf9e469-f66a-48e2-a9b3-fce900c19532
📒 Files selected for processing (18)
docker/structure.sqllib/host_platform.pylib/scenario_runner.pylib/schema_checker.pylib/user.pylib/utils.pymigrations/2026_07_21_host_execution_capability.sqlrunner.pyshell.pytests/data/usage_scenarios/host_execution.ymltests/data/usage_scenarios/host_execution_mixed.ymltests/data/usage_scenarios/schema_checker/schema_checker_invalid_host_execution_playwright.ymltests/data/usage_scenarios/schema_checker/schema_checker_valid_host_execution.ymltests/lib/test_host_platform.pytests/lib/test_schema_checker.pytests/lib/test_user.pytests/test_runner.pytests/test_shell.py
| if metric_provider._metric_name.endswith('_container') and not self.__containers: | ||
| print(TerminalColors.WARNING, arrows(f"Skipping {metric_provider.__class__.__name__} as it needs container IDs and no containers are part of this run"), TerminalColors.ENDC) | ||
| continue | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Container-skip check omits network_connections_tcpdump_system, unlike every other branch in this function.
Lines 2207/2210/2232/2235 all treat _container-suffixed providers and network_connections_tcpdump_system identically (both need container/docker-network context). The new skip-when-no-containers check only tests .endswith('_container'), so in a container-less run (e.g. shell.py, or any host_execution.yml-style scenario) tcpdump will still be started even though there are no containers/docker network for it to attach to — inconsistent with _add_containers_to_metric_providers, which also never feeds it container data.
Additionally, the skip message here still uses a plain print() rather than self._append_and_print_warning(), so — per the earlier review comment asking whether this should also surface in the UI — it never gets persisted to runs.warnings for display in reports.
🐛 Suggested fix: include tcpdump in both skip checks
- if metric_provider._metric_name.endswith('_container') and not self.__containers:
- print(TerminalColors.WARNING, arrows(f"Skipping {metric_provider.__class__.__name__} as it needs container IDs and no containers are part of this run"), TerminalColors.ENDC)
+ if (metric_provider._metric_name.endswith('_container') or metric_provider._metric_name == 'network_connections_tcpdump_system') and not self.__containers:
+ self._append_and_print_warning(f"Skipping {metric_provider.__class__.__name__} as it needs container IDs and no containers are part of this run")
continue- if metric_provider._metric_name.endswith('_container') and not self.__containers: # was skipped and never booted
+ if (metric_provider._metric_name.endswith('_container') or metric_provider._metric_name == 'network_connections_tcpdump_system') and not self.__containers: # was skipped and never booted
continueAlso applies to: 2238-2240
| def flow_runs_on_host(flow): | ||
| # container: None requests host execution and arrives either as YAML null or as the literal string 'None' | ||
| return flow.get('container') in (None, 'None') | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
flow_runs_on_host treats the string 'None' as host execution — collides with a container literally named "None".
container names come from free-form service/container_name strings (schema only requires non-empty + no invalid chars), so nothing stops a user from having a real container named "None". Because flow_runs_on_host also matches the plain string 'None', such a flow would silently run directly on the host instead of via docker exec, bypassing the docker sandbox and triggering the security-sensitive 'host' orchestrator capability check for what should be an ordinary containerized flow. This is exactly the ambiguity the earlier review comment flagged ("stick to the YAML syntax ... produces an actual python None").
🛡️ Suggested fix: drop the string-'None' fallback, require real YAML null
def flow_runs_on_host(flow):
- # container: None requests host execution and arrives either as YAML null or as the literal string 'None'
- return flow.get('container') in (None, 'None')
+ # container: (blank) or container: null requests host execution.
+ # Unquoted `container: None` is intentionally NOT treated as host execution here,
+ # since PyYAML parses it as the plain string "None", which would otherwise collide
+ # with a legitimately-named container called "None".
+ return flow.get('container') is NoneAlso applies to: 232-233
| finally: | ||
| # Explicitly close the psycopg pool to avoid thread finalization warnings on interpreter shutdown. | ||
| try: | ||
| if hasattr(DB, "instance") and hasattr(DB.instance, "_pool"): | ||
| DB.instance.shutdown() | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silently swallowed exception in DB pool shutdown.
The bare try/except Exception: pass around DB.instance.shutdown() hides any failure during cleanup with no trace at all (flagged by Ruff S110/BLE001). Since this runs after every CLI invocation, a real shutdown failure (e.g. leaked connections) would go completely unnoticed.
🩹 Suggested fix: log instead of silently discarding
try:
if hasattr(DB, "instance") and hasattr(DB.instance, "_pool"):
DB.instance.shutdown()
- except Exception: # pylint: disable=broad-exception-caught
- pass
+ except Exception as exc: # pylint: disable=broad-exception-caught
+ error_helpers.log_error("Could not cleanly shut down DB pool in shell.py", exception=exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally: | |
| # Explicitly close the psycopg pool to avoid thread finalization warnings on interpreter shutdown. | |
| try: | |
| if hasattr(DB, "instance") and hasattr(DB.instance, "_pool"): | |
| DB.instance.shutdown() | |
| except Exception: # pylint: disable=broad-exception-caught | |
| pass | |
| finally: | |
| # Explicitly close the psycopg pool to avoid thread finalization warnings on interpreter shutdown. | |
| try: | |
| if hasattr(DB, "instance") and hasattr(DB.instance, "_pool"): | |
| DB.instance.shutdown() | |
| except Exception as exc: # pylint: disable=broad-exception-caught | |
| error_helpers.log_error("Could not cleanly shut down DB pool in shell.py", exception=exc) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 241-242: try-except-pass detected, consider logging the exception
(S110)
[warning] 241-241: Do not catch blind exception: Exception
(BLE001)
Source: Linters/SAST tools
|
The tests are OK. The errors shown seem to be related to an test-data issue for a commit I pushed yesterday. WIll investigate separately |
* main: (tests): Moving Daily GitHub Actions Job also to Blacksmith (feat): Better stdout passing through in cluster errors Python version of phase_stats.py calculation (#1787) Test speedup (#1782) Bump fastapi from 0.140.13 to 0.141.1 (#1805) Bump uvicorn from 0.51.0 to 0.52.0 (#1804) CLAUDE.md shall be auto updated by Agent Carbondb backfill and copy over rework (#1802) (hotfix): Email jobs are now handled separately and not as bulk (rework): Making some more columns NOT NULL where we enforce values to make indexes cleaner and simplify mental overhead for data strucutres CLAUDE.md added as symlink of AGENTS.md to make CLAUDE Code also read it by default (test-fix): Race condition with outdated commit-hash variable Bump fastapi from 0.139.2 to 0.140.13 (#1799) Bump tqdm from 4.69.0 to 4.70.0 (#1798)
|
I made a merge commit with weaving in the changes I did in the big PR about test refactoring. Should be up to date now and functionally unchanged |
|
Sadly the merge did not cut it ... somehow the tests seem to look for an outdated plain container name without the parallel-worker suffix ... I cannot see why atm ... maybe you see more on monday with fresh eyes ... |
2 participants
Something to discuss :) I think this is really really useful but see the arguments against it.
Maybe just run it
Summary
shell.py, a CLI wrapper for measuring host shell commands with GMT.--print-phase-stats-tableand reusable table formatting utilities.