-
Notifications
You must be signed in to change notification settings - Fork 31
feat(runtime): add runtime process discovery service #808
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
|
The latest Buf updates on your PR. Results from workflow Buf CI / verify-proto (pull_request).
|
f24913c to
14b17d8
Compare
| def _start_cni_watcher(self, callback: Callable[[str, Workload], None]): | ||
| """Start watching CNI state directory for network changes.""" | ||
| class CNIEventHandler(FileSystemEventHandler): | ||
| def __init__(handler_self, adapter): |
Check notice
Code scanning / CodeQL
First parameter of a method is not named 'self' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
In general, to fix this type of issue you rename the first parameter of an instance method from a non-standard name to self and adjust all references inside that method (and any sibling methods in the same class that used the same non-standard name) to use self instead. This preserves behavior while complying with PEP 8 and static analysis expectations.
Here, within discovery/watcher/runtimes/containerd.py, the inner class CNIEventHandler defined inside _start_cni_watcher uses handler_self as the first parameter name for __init__, on_created, on_deleted, and _handle_cni_change. The best fix is to:
- Change the parameter name
handler_selftoselfin each of these four methods. - Replace all occurrences of
handler_selfin their bodies withself.
No other parts of the file need to change, since CNIEventHandler is only instantiated via its constructor (CNIEventHandler(self)), which is unaffected by the parameter name change. No new imports or external definitions are required.
-
Copy modified lines R300-R301 -
Copy modified line R303 -
Copy modified line R306 -
Copy modified line R308 -
Copy modified line R311 -
Copy modified line R313 -
Copy modified line R314
| @@ -297,20 +297,20 @@ | ||
| def _start_cni_watcher(self, callback: Callable[[str, Workload], None]): | ||
| """Start watching CNI state directory for network changes.""" | ||
| class CNIEventHandler(FileSystemEventHandler): | ||
| def __init__(handler_self, adapter): | ||
| handler_self.adapter = adapter | ||
| def __init__(self, adapter): | ||
| self.adapter = adapter | ||
|
|
||
| def on_created(handler_self, event): | ||
| def on_created(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def on_deleted(handler_self, event): | ||
| def on_deleted(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def _handle_cni_change(handler_self, filepath): | ||
| def _handle_cni_change(self, filepath): | ||
| # Extract container ID from filename | ||
| filename = os.path.basename(filepath) | ||
| parts = filename.split("-") | ||
| @@ -318,7 +311,7 @@ | ||
| # Try to find the container ID part (usually second) | ||
| for part in parts[1:]: | ||
| if len(part) >= 12: | ||
| workload = handler_self.adapter._get_workload(part[:12]) | ||
| workload = self.adapter._get_workload(part[:12]) | ||
| if workload: | ||
| callback(EventType.NETWORK_CHANGED, workload) | ||
| break |
| def __init__(handler_self, adapter): | ||
| handler_self.adapter = adapter | ||
|
|
||
| def on_created(handler_self, event): |
Check notice
Code scanning / CodeQL
First parameter of a method is not named 'self' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
In general, to fix this issue you should rename the first parameter of instance methods to self and update all references inside those methods to use self instead of the old name. This keeps the methods as normal instance methods while conforming to PEP 8 and satisfying the static analysis rule.
For this specific case in discovery/watcher/runtimes/containerd.py, within the nested CNIEventHandler class inside the _start_cni_watcher method, change the method signatures:
def __init__(handler_self, adapter):→def __init__(self, adapter):def on_created(handler_self, event):→def on_created(self, event):def on_deleted(handler_self, event):→def on_deleted(self, event):def _handle_cni_change(handler_self, filepath):→def _handle_cni_change(self, filepath):
Then, update all usages of handler_self within those methods to self:
handler_self.adapter = adapter→self.adapter = adapterhandler_self._handle_cni_change(...)→self._handle_cni_change(...)workload = handler_self.adapter._get_workload(...)→workload = self.adapter._get_workload(...)
No imports or additional methods are needed; this is a pure renaming within the shown snippet and does not change functionality.
-
Copy modified lines R300-R301 -
Copy modified line R303 -
Copy modified line R306 -
Copy modified line R308 -
Copy modified line R311 -
Copy modified line R313 -
Copy modified line R314
| @@ -297,20 +297,20 @@ | ||
| def _start_cni_watcher(self, callback: Callable[[str, Workload], None]): | ||
| """Start watching CNI state directory for network changes.""" | ||
| class CNIEventHandler(FileSystemEventHandler): | ||
| def __init__(handler_self, adapter): | ||
| handler_self.adapter = adapter | ||
| def __init__(self, adapter): | ||
| self.adapter = adapter | ||
|
|
||
| def on_created(handler_self, event): | ||
| def on_created(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def on_deleted(handler_self, event): | ||
| def on_deleted(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def _handle_cni_change(handler_self, filepath): | ||
| def _handle_cni_change(self, filepath): | ||
| # Extract container ID from filename | ||
| filename = os.path.basename(filepath) | ||
| parts = filename.split("-") | ||
| @@ -318,7 +311,7 @@ | ||
| # Try to find the container ID part (usually second) | ||
| for part in parts[1:]: | ||
| if len(part) >= 12: | ||
| workload = handler_self.adapter._get_workload(part[:12]) | ||
| workload = self.adapter._get_workload(part[:12]) | ||
| if workload: | ||
| callback(EventType.NETWORK_CHANGED, workload) | ||
| break |
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
|
|
||
| def on_deleted(handler_self, event): |
Check notice
Code scanning / CodeQL
First parameter of a method is not named 'self' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
In general, to fix this type of issue, rename the first parameter of normal instance methods to self and update all references inside those methods accordingly. This preserves functionality while satisfying PEP 8 and the static analysis rule. Only if the method truly doesn't use instance state should it be converted to a @staticmethod or moved outside the class.
For this specific case in discovery/watcher/runtimes/containerd.py, inside the _start_cni_watcher method, the nested CNIEventHandler class defines four instance methods (__init__, on_created, on_deleted, _handle_cni_change) that use handler_self as their first parameter name. We should rename that parameter to self in each method and adjust the internal references from handler_self to self. This change is limited to the inner class body and does not affect any call sites outside, because Python passes the instance as the first argument regardless of the parameter name, and external code never refers to handler_self directly. No new imports or helpers are required.
Concretely:
- In the block starting at line 299 where
class CNIEventHandler(FileSystemEventHandler):is defined, change:def __init__(handler_self, adapter):→def __init__(self, adapter):andhandler_self.adapter→self.adapter.def on_created(handler_self, event):→def on_created(self, event):andhandler_self._handle_cni_change(...)→self._handle_cni_change(...).def on_deleted(handler_self, event):→def on_deleted(self, event):andhandler_self._handle_cni_change(...)→self._handle_cni_change(...).def _handle_cni_change(handler_self, filepath):→def _handle_cni_change(self, filepath):andhandler_self.adapter...→self.adapter....
No other parts of the file need to change.
-
Copy modified lines R300-R301 -
Copy modified line R303 -
Copy modified line R306 -
Copy modified line R308 -
Copy modified line R311 -
Copy modified line R313 -
Copy modified line R314
| @@ -297,20 +297,20 @@ | ||
| def _start_cni_watcher(self, callback: Callable[[str, Workload], None]): | ||
| """Start watching CNI state directory for network changes.""" | ||
| class CNIEventHandler(FileSystemEventHandler): | ||
| def __init__(handler_self, adapter): | ||
| handler_self.adapter = adapter | ||
| def __init__(self, adapter): | ||
| self.adapter = adapter | ||
|
|
||
| def on_created(handler_self, event): | ||
| def on_created(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def on_deleted(handler_self, event): | ||
| def on_deleted(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def _handle_cni_change(handler_self, filepath): | ||
| def _handle_cni_change(self, filepath): | ||
| # Extract container ID from filename | ||
| filename = os.path.basename(filepath) | ||
| parts = filename.split("-") | ||
| @@ -318,7 +311,7 @@ | ||
| # Try to find the container ID part (usually second) | ||
| for part in parts[1:]: | ||
| if len(part) >= 12: | ||
| workload = handler_self.adapter._get_workload(part[:12]) | ||
| workload = self.adapter._get_workload(part[:12]) | ||
| if workload: | ||
| callback(EventType.NETWORK_CHANGED, workload) | ||
| break |
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
|
|
||
| def _handle_cni_change(handler_self, filepath): |
Check notice
Code scanning / CodeQL
First parameter of a method is not named 'self' Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
In general, to fix this kind of issue, rename the first parameter of instance methods to self and update all internal references accordingly. If a method truly does not use instance state, it should be converted to a @staticmethod or moved out of the class, but here the methods all access instance attributes (handler_self.adapter and methods on it), so they should remain instance methods.
For this file, the cleanest fix is to standardize on self for all instance methods of the inner CNIEventHandler class, not just _handle_cni_change. That means changing handler_self to self in the parameter lists of __init__, on_created, on_deleted, and _handle_cni_change, and updating all corresponding uses in their bodies. The class is defined inside _start_cni_watcher, so no external code references these parameter names directly; behavior remains unchanged. All changes occur within discovery/watcher/runtimes/containerd.py in the region around lines 299–323, and no additional imports or definitions are required.
-
Copy modified lines R300-R301 -
Copy modified line R303 -
Copy modified line R306 -
Copy modified line R308 -
Copy modified line R311 -
Copy modified line R313 -
Copy modified line R314
| @@ -297,20 +297,20 @@ | ||
| def _start_cni_watcher(self, callback: Callable[[str, Workload], None]): | ||
| """Start watching CNI state directory for network changes.""" | ||
| class CNIEventHandler(FileSystemEventHandler): | ||
| def __init__(handler_self, adapter): | ||
| handler_self.adapter = adapter | ||
| def __init__(self, adapter): | ||
| self.adapter = adapter | ||
|
|
||
| def on_created(handler_self, event): | ||
| def on_created(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def on_deleted(handler_self, event): | ||
| def on_deleted(self, event): | ||
| if event.is_directory: | ||
| return | ||
| handler_self._handle_cni_change(event.src_path) | ||
| self._handle_cni_change(event.src_path) | ||
|
|
||
| def _handle_cni_change(handler_self, filepath): | ||
| def _handle_cni_change(self, filepath): | ||
| # Extract container ID from filename | ||
| filename = os.path.basename(filepath) | ||
| parts = filename.split("-") | ||
| @@ -318,7 +311,7 @@ | ||
| # Try to find the container ID part (usually second) | ||
| for part in parts[1:]: | ||
| if len(part) >= 12: | ||
| workload = handler_self.adapter._get_workload(part[:12]) | ||
| workload = self.adapter._get_workload(part[:12]) | ||
| if workload: | ||
| callback(EventType.NETWORK_CHANGED, workload) | ||
| break |
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
8044b8b to
def6a6d
Compare
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
| """ | ||
|
|
||
| from abc import ABC, abstractmethod | ||
| from typing import Optional, Callable |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, remove the unused Optional name from the typing import, keeping only what is actually used. This eliminates the unnecessary dependency and resolves the CodeQL warning without changing any runtime behavior or public interface.
Concretely, in discovery/watcher/runtimes/interface.py, update line 14 so that it imports only Callable from typing. No other changes are required because Optional is not referenced anywhere in the provided code. The rest of the file, including existing imports and method signatures, remains unchanged.
-
Copy modified line R14
| @@ -11,7 +11,7 @@ | ||
| """ | ||
|
|
||
| from abc import ABC, abstractmethod | ||
| from typing import Optional, Callable | ||
| from typing import Callable | ||
|
|
||
| from models import Workload, Runtime, EventType | ||
|
|
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
| storage.list_all() | ||
| return jsonify({"status": "ready"}) | ||
| except Exception as e: | ||
| return jsonify({"error": f"Not ready: {e}"}), 503 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
In general, the fix is to avoid returning the raw exception object or its message to the client, and instead log the exception on the server while responding with a generic, non-sensitive error message and appropriate HTTP status code.
For this specific code in discovery/server/app.py, the best fix is:
- Keep the
try/exceptaroundstorage.list_all()in thereadyendpoint. - In the
exceptblock, log the exception using the existingloggerconfigured at the top of the file. - Replace the error response body so it does not include
e(or any exception detail). Use a generic message such as"Not ready"or"Service is not ready". - Keep returning HTTP 503 to preserve existing behavior for health probes.
Concretely:
- Modify lines 165–166 so that:
logger.exception("Readiness check failed")is called, which logs the stack trace and message server-side.- The
jsonifypayload becomes something like{"error": "Not ready"}(no interpolation ofe).
No new imports are needed: the module already imports logging and defines logger = logging.getLogger("discovery.server").
-
Copy modified lines R165-R167
| @@ -162,8 +162,9 @@ | ||
| try: | ||
| storage.list_all() | ||
| return jsonify({"status": "ready"}) | ||
| except Exception as e: | ||
| return jsonify({"error": f"Not ready: {e}"}), 503 | ||
| except Exception: | ||
| logger.exception("Readiness check failed") | ||
| return jsonify({"error": "Not ready"}), 503 | ||
|
|
||
| @app.route('/stats', methods=['GET']) | ||
| def stats(): |
| """ | ||
|
|
||
| from dataclasses import dataclass, field, asdict | ||
| from enum import Enum |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, remove the unused Enum import from the imports at the top of discovery/server/models.py. This aligns with the recommendation to delete unused imports and does not alter any existing functionality.
Concretely, in discovery/server/models.py, edit the import section so that the line from enum import Enum is removed, leaving all other imports (dataclass, field, asdict, Optional, json) unchanged. No additional methods, imports, or definitions are needed.
| @@ -3,7 +3,6 @@ | ||
| """ | ||
|
|
||
| from dataclasses import dataclass, field, asdict | ||
| from enum import Enum | ||
| from typing import Optional | ||
| import json | ||
|
|
| import signal | ||
| import sys | ||
| import threading | ||
| import time |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 13 hours ago
To fix an unused import, the appropriate change is to remove the import statement for the unused module, thereby simplifying dependencies and avoiding confusion.
Concretely, in discovery/watcher/main.py, the line import time at line 9 should be removed, because nothing in the shown file uses the time module. This change does not alter any existing behavior, as no code depends on time. No additional methods, definitions, or imports are needed to implement this fix; it’s a straightforward deletion of that single import line.
| @@ -6,7 +6,6 @@ | ||
| import signal | ||
| import sys | ||
| import threading | ||
| import time | ||
| from typing import Optional | ||
|
|
||
| from config import load_config, Config, logger |
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Signed-off-by: Ramiz Polic <rpolic@cisco.com>
Summary
This PR introduces an MVP runtime service that enables process discovery and lifecycle management for agent processes. Created as a shim-layer to expose a common API over any supported runtime. Supports docker, containerd, kubernetes.
Runtime gRPC API (
agntcy.dir.runtime.v1)DiscoveryService.ListProcesses— Returns a list of reachable processes from a given source taking into account network isolation. Gives the negotiation options e.g. static with metadata, protocol, or access type or semantic by the agent in runtime.