[DSPX-3302] (3/5) otdf-local multi-instance refactor#452
[DSPX-3302] (3/5) otdf-local multi-instance refactor#452dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a multi-instance refactor for the otdf-local CLI, enabling the management of isolated test environments. It introduces new instance and scenario subcommands, updates the configuration system to be instance-aware, and integrates with otdf-sdk-mgr for binary management. Service launchers for KAS and the platform now support per-instance port offsets and directory structures. Review feedback highlights a potential TypeError in KAS feature handling and suggests a more direct approach for updating Pydantic model metadata.
| # Per-KAS features from instance.yaml override the legacy heuristic. | ||
| instance = self.settings.load_instance() | ||
| kas_pin = instance.kas.get(self._kas_name) if instance is not None else None | ||
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} |
There was a problem hiding this comment.
The dict(kas_pin.features) call will raise a TypeError if kas_pin.features is None. Since features are typically optional in the configuration schema, this should be handled defensively to avoid crashing when no features are specified for a KAS instance.
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} | |
| extra_features: dict[str, bool] = dict(kas_pin.features or {}) if kas_pin is not None else {} |
| else: | ||
| raise typer.BadParameter(f"{scenario_path} has unknown kind {kind!r}") | ||
| # Ensure the metadata name matches the chosen directory name. | ||
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) |
There was a problem hiding this comment.
Updating the metadata name by dumping and re-creating the entire Metadata object is unnecessarily complex and inefficient. Since Pydantic models are mutable by default, you can update the field directly on the existing object.
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) | |
| instance.metadata.name = name |
There was a problem hiding this comment.
Code Review
This pull request implements a multi-instance architecture for the otdf-local CLI, allowing for the management and execution of isolated test environments. Key updates include new subcommands for instance and scenario handling, offset-based port allocation, and instance-specific directory structures for logs and configurations. Feedback from the review suggests several improvements: adding a null check for KAS features to avoid runtime errors, using Pydantic's model_copy for cleaner metadata updates, adopting shlex.join for safer command display, and adding missing type hints to enhance code maintainability.
| # Per-KAS features from instance.yaml override the legacy heuristic. | ||
| instance = self.settings.load_instance() | ||
| kas_pin = instance.kas.get(self._kas_name) if instance is not None else None | ||
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} |
There was a problem hiding this comment.
If kas_pin.features is None in the instance configuration, calling dict() on it will raise a TypeError: 'NoneType' object is not iterable. You should provide a default empty dictionary or add a check.
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} | |
| extra_features: dict[str, bool] = dict(kas_pin.features or {}) if kas_pin is not None else {} |
| else: | ||
| raise typer.BadParameter(f"{scenario_path} has unknown kind {kind!r}") | ||
| # Ensure the metadata name matches the chosen directory name. | ||
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) |
There was a problem hiding this comment.
This line is quite verbose. Since Metadata is a Pydantic model, you can use model_copy with the update parameter to achieve the same result more cleanly.
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) | |
| instance.metadata = instance.metadata.model_copy(update={"name": name}) |
| pytest_args.extend(extra) | ||
|
|
||
| cmd = ["uv", "run", "pytest", *pytest_args] | ||
| typer.echo(f" Running: {' '.join(cmd)} (cwd={xtest_root})") |
There was a problem hiding this comment.
Using ' '.join(cmd) for display can be misleading if any of the arguments contain spaces. It is safer to use shlex.join to format the command string for the console.
| typer.echo(f" Running: {' '.join(cmd)} (cwd={xtest_root})") | |
| import shlex | |
| typer.echo(f" Running: {shlex.join(cmd)} (cwd={xtest_root})") |
| return Ports.get_kas_port(name, base=instance.ports.base) | ||
| return Ports.get_kas_port(name) | ||
|
|
||
| def load_instance(self): |
There was a problem hiding this comment.
Adding a return type hint here would improve IDE support and maintainability. Since the Instance type is imported locally within the method, you can use a string forward reference.
| def load_instance(self): | |
| def load_instance(self) -> "Instance | None": |
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-instance test harness capability, allowing for the management and execution of isolated OpenTDF environments with distinct configurations, port ranges, and platform versions. Key additions include new CLI subcommands for instance management (init, ls, rm) and scenario execution, an instance-aware settings system, and integration with otdf-sdk-mgr to resolve versioned binaries. Feedback identifies a critical issue where the up command still relies on static port constants, which will break health checks for non-default instances. Additionally, improvements were suggested regarding safer dictionary handling for KAS features and more idiomatic use of Pydantic's model_copy.
| if instance is not None: | ||
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance | ||
| # Invalidate the cached Settings so subsequent commands see the new value | ||
| get_settings.cache_clear() |
There was a problem hiding this comment.
The --instance flag correctly updates the environment and clears the settings cache. However, the up command in this file (specifically at lines 192 and 225) still relies on static Ports constants for health checks and port resolution. This will cause health checks to fail when a non-default instance with a different ports_base is active. The up command should be updated to use the instance-aware settings.get_kas_port(name) or the port property of the service instances, and it should iterate over the instances managed by kas_manager instead of Ports.all_kas_names().
| else: | ||
| raise typer.BadParameter(f"{scenario_path} has unknown kind {kind!r}") | ||
| # Ensure the metadata name matches the chosen directory name. | ||
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) |
There was a problem hiding this comment.
Since Metadata is a Pydantic model, you can use model_copy with the update parameter to modify the name. This is more idiomatic and concise than dumping to a dict and reconstructing the model.
| instance.metadata = Metadata(**{**instance.metadata.model_dump(exclude_none=True), "name": name}) | |
| instance.metadata = instance.metadata.model_copy(update={"name": name}) |
| # Per-KAS features from instance.yaml override the legacy heuristic. | ||
| instance = self.settings.load_instance() | ||
| kas_pin = instance.kas.get(self._kas_name) if instance is not None else None | ||
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} |
There was a problem hiding this comment.
If kas_pin.features is None, calling dict() on it will raise a TypeError. It's safer to provide an empty dictionary as a fallback.
| extra_features: dict[str, bool] = dict(kas_pin.features) if kas_pin is not None else {} | |
| extra_features: dict[str, bool] = dict(kas_pin.features or {}) if kas_pin is not None else {} |
X-Test Results✅ js-v0.15.0 |
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
c6a7895 to
ebc0c15
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-03-multi-instance
branch
from
c69afd6 to
a8ef24a
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
ebc0c15 to
14e5c1e
Compare
Refactors otdf-local from a single-instance CLI (one platform checkout,
fixed ports, hardcoded six KAS instances) into a multi-instance harness
where each named instance under tests/instances/<name>/ owns its own
opentdf.yaml, keys, KAS configs, and port range.
Why
---
A single bug report often describes a *combination* — platform v0.9.0
with Java SDK 0.7.8 and a KAS at a pre-release. Today a developer has
to hand-edit configs and re-checkout the platform to reproduce. After
this change:
otdf-local instance init java-078 --from-scenario .../scenario.yaml
otdf-local --instance java-078 up
brings up exactly the topology the scenario describes, using platform
binaries that otdf-sdk-mgr already provisioned (each instance, and each
KAS within an instance, can reference a different pinned version). Two
instances on disjoint ports.base can coexist on a developer laptop.
What changes
------------
otdf-local now depends on otdf-sdk-mgr via a uv path source so both
tools share the canonical Scenario/Instance schema.
Settings (otdf_local.config.settings):
- New instance_name (env-overridable via OTDF_LOCAL_INSTANCE_NAME),
instance_dir, instances_root, instance_yaml properties.
- platform_dir becomes optional; legacy sibling-discovery only kicks
in when no per-instance configuration is present.
- platform_binary_for(dist) resolves to the otdf-sdk-mgr-managed
xtest/platform/dist/<dist>/service binary.
- keys_dir, logs_dir, config_dir, platform_config, and
get_kas_config_path switch to per-instance paths whenever
instance.yaml exists; legacy behavior is preserved otherwise.
- load_instance() reads the per-instance manifest via the shared
Pydantic model.
Ports (otdf_local.config.ports):
- KAS_OFFSETS exposes the offset table (alpha=+101, beta=+202, ...,
km2=+606) so multiple instances on different bases get disjoint
port ranges. The legacy 8080-based constants are preserved as
defaults.
- get_kas_port(name, base=...) computes the port relative to base.
Services (otdf_local.services.platform / .kas):
- PlatformService.start() and KASService.start() use the pinned dist
binary at xtest/platform/dist/<dist>/service when an instance is
loaded, with cwd set to the recorded worktree so the binary finds
its embedded resources. Legacy `go run ./service` path runs
unchanged when no instance is active.
- KASService.is_key_management defers to the manifest's `mode` field
instead of the legacy name-based heuristic; per-KAS features (e.g.
ec_tdf_enabled) pass through to opentdf.yaml.
- KASManager constructs only the KAS instances listed in
instance.yaml's kas: map. start_standard / start_km filter on
is_key_management so subset topologies still work.
utils.keys.setup_golden_keys:
- Writes key files into the target directory (per-instance keys_dir
or legacy platform_dir) and uses absolute paths in the generated
keys_config so the binary finds them regardless of cwd.
CLI:
- New top-level --instance option threads through every command via
OTDF_LOCAL_INSTANCE_NAME.
- New `instance` subcommand group: init [--from-scenario PATH],
ls --json, rm.
- New `scenario` subcommand: `run <path>` translates the scenario's
suite block into `pytest --sdks-encrypt ... --sdks-decrypt ...
--containers ...` under xtest/ with OTDF_LOCAL_INSTANCE_NAME set.
Tests (otdf-local/tests/test_multi_instance.py):
- Port arithmetic at default and alternate bases.
- Settings round-trip with and without an instance.yaml.
- platform_binary_for resolves under the otdf-sdk-mgr-managed
xtest/platform/ tree.
.gitignore additions:
- tests/instances/ (per-instance config and logs)
- xtest/scenarios/*.installed.json (provisioning records)
- .claude/tmp/
Backward compatibility:
- `otdf-local up` with no --instance flag keeps working against a
sibling platform/ checkout.
Refs: https://virtru.atlassian.net/browse/DSPX-3302
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-3302-03-multi-instance
branch
from
a8ef24a to
78b2ca6
Compare
|
1 participant
Summary
Third PR in the five-part stack. Refactors
otdf-localfrom a single-instance CLI to a multi-instance harness. Each named instance undertests/instances/<name>/owns its ownopentdf.yaml, keys, KAS configs, and port range, and references platform binaries managed byotdf-sdk-mgr(PR 2).Highlights
instance_name,instance_dir,instances_root,platform_binary_for(dist). Per-instance paths kick in wheneverinstance.yamlexists; legacy single-instance behavior is preserved when it doesn't.instance.ports.basevia a newKAS_OFFSETStable. Two instances on different bases coexist.xtest/platform/dist/<dist>/servicebinary when an instance is loaded;go run ./servicelegacy path runs unchanged otherwise. KAS features (ec_tdf_enabled, etc.) come frominstance.yaml'skas.<name>.features.KASManagerrestricts the managed set to KAS names listed in the manifest (subset topologies work).utils.keys.setup_golden_keyswrites keys into the target dir and emits absolute paths so the binary finds them regardless of cwd.--instance NAMEotdf-local instance init <name> [--from-scenario PATH] [--ports-base N] [--platform DIST]otdf-local instance ls --json,otdf-local instance rm <name> -yotdf-local scenario run <path>(translates suite block to pytest args)otdf-local/pyproject.tomldeclaresotdf-sdk-mgrvia[tool.uv.sources]./instances/,xtest/scenarios/*.installed.json,.claude/tmp/.test_multi_instance.pycovering port arithmetic, settings round-trip with/without an instance, and binary resolution.Backward compatibility
uv run otdf-local upwithout--instancestill works against a siblingplatform/checkout. Migration to multi-instance is opt-in viainstance init.Stack
Test plan
cd otdf-local && uv run pytest tests/ -m 'not integration'→ 27 passing (20 existing + 5 new + 2 integration kept)uv run otdf-local instance init demo --from-scenario <path>→ directory layout correctuv run otdf-local instance ls --json→ enumerates instanceuv run otdf-local --instance demo instance ls→--instanceflag threads throughJira: https://virtru.atlassian.net/browse/DSPX-3302
🤖 Generated with Claude Code