0.17 devel - #2071
Open
Donaim wants to merge 465 commits into
Open
0.17 devel#2071Donaim wants to merge 465 commits into
Donaim wants to merge 465 commits into
Conversation
…inates and enhance validation checks
Bug: DatasetSerializer.create() used validated_data.get('save_in_db', keep_file)
to determine whether to retain an uploaded file. DRF's BooleanField injects
False into validated_data even when the client omitted the field entirely.
This caused every file uploaded via kiveapi.add_dataset() (which does not
send save_in_db) to be discarded: the file appeared uploaded with MD5 but
dataset_file was null, is_purged was True, and has_data was False.
Fix: check 'save_in_db' in self.initial_data before reading its value.
Only override the default (keep_file=True for direct uploads,
keep_file=False for external-file paths) when the request explicitly
supplied the field. This distinguishes 'omitted' from 'explicitly false'.
Add 4 tests to DatasetSerializerTests: - test_create_from_upload_omits_save_in_db_retains_file: direct file upload without save_in_db must retain the file (the regression fix). - test_create_from_upload_explicit_save_in_db_true: explicit true retains file (existing behavior preserved). - test_create_from_upload_explicit_save_in_db_false: explicit false discards file (existing behavior preserved, if intentional). - test_create_from_external_omits_save_in_db_does_not_retain: external file path without save_in_db must not retain a DB copy (existing external-file default preserved).
…move enp5s0 Three intertwined changes: 1. ensure_managed_vm_network() replaces choose_existing_vm_network(). Creates a managed Incus bridge (default kive-lab-br) if it does not exist, or repairs an existing one (sets ipv4.nat=true, ipv6.address=none). Fails if the requested network exists but is unmanaged. 2. ensure_vm_nic() enforces the target managed network. Rejects macvlan, stale parent=..., wrong network=... from instance-level or profile-level eth0. Overrides with an instance-level device using network=NAME. Profile-matching network=NAME is still accepted as no-op. 3. VM cloud-init no longer sets user.network-config by default. The Ubuntu cloud image gets DHCP automatically from the Incus-managed NIC. enable_network_config() for VM mode now skips the config entirely, removing the hardcoded enp5s0 interface name. Also adds: - wait_vm_dhcp_lease(): polls incus network list-leases before provisioning. - print_network_diagnostics(): prints host-side diagnostics on DHCP failure. - dnsmasq version UNKNOWN diagnostic in _ensure_incus_network_create().
…e wait Replace old choose_existing_vm_network tests with ensure_managed_vm_network tests: - create when missing - prefer kive-lab-br over incusbr0 - use incusbr0 fallback when kive-lab-br absent - repair bridge (set NAT, disable IPv6) - fail on unmanaged bridge - respect requested network name Replace old NIC tests: - adds with network=NAME (no more unmanaged parent/nnictype tests) - noop on matching network - replaces wrong network, macvlan, stale kive-devel-br - overrides profile macvlan, noop on profile matching Add tests: - wait_vm_dhcp_lease returns IP and None - VM cloud-init does not set network-config by default - _ensure_incus_network_create handles dnsmasq UNKNOWN error
…ght, profile override fix
Four changes:
1. Host egress repair (_repair_managed_bridge extended):
- Enables net.ipv4.ip_forward if disabled.
- Creates nftables table 'inet kive_egress' with forward chain (accept
for iifname/oifname with ct state established,related) and NAT
postrouting chain (masquerade for bridge CIDR).
- All nft operations are idempotent (add fails gracefully if exists).
2. VM egress preflight (_check_vm_egress in runner.py):
- After DHCP lease is confirmed, runs a bounded script inside the VM
via incus exec that tests raw IPv4 (1.1.1.1:443) and DNS+TCP
(archive.ubuntu.com:80).
- Fails fast if raw IPv4 egress is broken (likely host NAT/firewall).
- If incus exec is unavailable, logs a warning but does not block
(the provision script inside the guest does its own checks).
3. Profile NIC override fix (ensure_vm_nic):
- When profile eth0 is wrong, now uses 'incus config device override'
+ 'device set' + 'device unset' instead of blindly adding a
duplicate instance-level device. Removes stale parent/nictype keys.
4. Provision-script diagnostics improved:
- Now tests 1.1.1.1:443 (raw IPv4) before archive.ubuntu.com:80
(DNS+TCP), with distinct error messages per layer.
- 'raw IPv4 egress failed; likely host forwarding/NAT/firewall.' vs
'DNS resolution worked but TCP egress to archive.ubuntu.com:80 failed.'
…script Update tests to match the new host egress repair, profile override with 'device override', MAC-based DHCP lease matching, and the improved provision-script diagnostics (1.1.1.1:443 test, new timeout values).
… rules The host egress nftables function used subprocess.run(['nft', ...]) which fails when nft is not on the PATH (the tool runs through Guix wrappers). Switch to cmds.nft.run() which handles the Guix environment wrapper. Also wrap the call in try/except FileNotFoundError for robustness.
…les; sysctl timeout Three fixes: 1. _check_vm_egress (runner.py): check result.returncode and stderr for known Incus transport errors (websocket, agent not running, connection refused, not connected) before interpreting output. If a transport error is detected, skip the egress check and continue to provisioning (the guest provision script will verify connectivity). Previously, 'websocket: bad handshake' was treated as raw IPv4 egress failure. 2. _ensure_host_egress_nftables (network.py): instead of 'add' with check=False (which could accumulate duplicate rules), now deletes and recreates the owned 'inet kive_egress' table on each run. This guarantees idempotency with no duplicate rule accumulation. 3. _ensure_host_ip_forward (network.py): wraps both sysctl calls in try/except with timeouts so sudo prompts or slow commands don't hang.
Add TestVmEgressCheck with 7 tests: - transport_error_websocket_skips: websocket error → no SystemExit - transport_error_agent_not_running_skips: agent not running → skip - transport_error_connection_refused_skips: connection refused → skip - transport_error_not_connected_skips: not connected → skip - non_transport_error_does_not_abort: non-transport rc=1 → warn, continue - successful_egress_returns: both OK markers → pass - fails_on_missing_raw_ipv4: no raw IPv4 marker → SystemExit
…ion; add verification Three changes: 1. Replace _ensure_host_egress_nftables with _ensure_host_egress_iptables. The nftables code passed 'inet kive_egress' as a single argv token instead of separate 'inet' and 'kive_egress'. The iptables approach is the same proven pattern from backends/incus_host.py: idempotent -C checks before -I insertion for DOCKER-USER, FORWARD, and POSTROUTING masquerade. 2. Bridge repair now runs on all paths. Previously _repair_managed_bridge() was skipped when the bridge was newly created. Now both existing and new bridges go through the same repair step (ipv4.nat, ipv4.routing, ipv4.firewall, ip_forward, iptables rules). 3. Added _verify_host_egress() which logs sysctl ip_forward, incus network show, and iptables rules after repair.
…onal bridge options Fix test_ensure_managed_vm_network_repairs_bridge to mock _ensure_host_egress_iptables and _verify_host_egress, and add side_effect entries for ipv4.routing and ipv4.firewall checks.
…king model Remove all direct host firewall mutation from build_vm/network.py: - Delete _ensure_host_ip_forward (sysctl), _ensure_host_egress_iptables, _add_masquerade_rule, _verify_host_egress. - _repair_managed_bridge now only uses incus network set commands (ipv4.nat, ipv4.routing, ipv4.firewall, ipv6.address). - Incus manages iptables/nftables internally when ipv4.nat, ipv4.routing, and ipv4.firewall are enabled. The egress failure message in runner.py/_check_vm_egress now points to Incus host networking diagnostics (incus network show, incus info, dnsmasq --version, nft --version, iptables --version) instead of attempting repair.
…irectly; remove obsolete mocks Add tests: - test_network_code_does_not_call_subprocess_iptables - test_network_code_does_not_call_subprocess_sysctl Remove mocks for deleted functions (_ensure_host_ip_forward, _ensure_host_egress_iptables, _verify_host_egress) from the bridge repair test.
…detect stale dnsmasq/Docker Major changes to bridge management: 1. Deterministic CIDR: use 10.77.77.1/24 instead of ipv4.address=auto. Makes debugging and stale-state detection easier. Configurable via KIVE_VM_BRIDGE_CIDR / KIVE_VM_BRIDGE_DNS constants. 2. validate_incus_bridge_config: never changes ipv4.address on an existing bridge. If the CIDR differs, fails with cleanup commands. Safe mutable keys (ipv4.nat, ipv4.routing, ipv4.firewall, ipv4.dhcp, ipv6.address, raw.dnsmasq) are still repaired silently. 3. validate_live_bridge_address: compares live kernel bridge address against Incus config. Fails if stale state (e.g. bridge has 10.77.77.1/24 on kernel but 10.166.248.1/24 in config). 4. check_cidr_conflict: checks that the desired CIDR does not overlap with existing host routes. 5. detect_docker_forward_drop: checks iptables FORWARD policy for Docker conflict (diagnostic only, not run from normal build-vm). 6. diagnose_dnsmasq_bind_failure: converts 'failed to create listening socket' errors into targeted stale-dnsmasq diagnostics. 7. raw.dnsmasq option added to bridge creation to set DNS server option (1.1.1.1, 8.8.8.8).
Add _mock_bridge_selection helper for cleaner bridge selection tests. Update repair test to use new deterministic CIDR and mutable key structure. Fix tests to mock validate_incus_bridge_config and validate_live_bridge_address where appropriate.
Introduce a single canonical method for computing the sandbox input filename of a ContainerDataset. KEYWORD_ARG_TYPES (optional-single and optional-multiple) get a filename based on ContainerDataset.name (or Dataset.name as fallback) with the ContainerDataset.id appended to guarantee uniqueness per binding rather than per Dataset row. This means the same Dataset used twice in one run produces two distinct filenames. FIXED_ARG_TYPES continue to use the argument name, unchanged. No callers yet; this is a pure addition.
Replace the old branching logic that used Dataset.unique_filename() for optional args and argument.name for fixed args. The old code could overwrite earlier-stage files when the same Dataset row was bound to an optional-multiple argument more than once, and the filename chosen here could disagree with the one later produced by _format_kw_args(). Now every ContainerDataset gets exactly one staged filename via _sandbox_input_filename(). The loop variable is renamed from 'dataset' to 'container_dataset' to avoid confusion.
Replace containerdataset.dataset.name with Command._sandbox_input_filename(cd) so that the paths emitted by _format_kw_args() exactly match the files staged by fill_sandbox(). Previously the command generation used the raw Dataset.name, which could differ from the filename chosen by fill_sandbox(). This mismatch meant the container received paths to files that did not exist in /mnt/input. Also materialize the generator passed to sort_by_position() as a list so that sorted() receives a reusable sequence (previously it consumed the generator and produced an empty list).
Prevent duplicate multi_position values for the same (run, argument) at the database level. The constraint is conditional: it only enforces uniqueness when multi_position IS NOT NULL, so it does not affect single-valued bindings where multi_position is None. This catches what serializer validation misses (e.g. concurrent requests or bulk operations).
ContainerDatasetSerializer.validate() enforces: - OPTIONAL_MULTIPLE_INPUT requires multi_position - all other argtypes must have multi_position=None ContainerRunSerializer.validate_datasets() enforces cross-dataset rules: - argument belongs to the run's app - input datasets must have data - duplicate multi_position within a request is rejected - multiple datasets for a single-valued argument are rejected These validations prevent invalid API-created ContainerDataset bindings from reaching the sandbox stage.
The new SandboxInputFilenameTests and RunContainerFormatKwArgsTests classes reference ContainerArgumentType enum values, so the import is needed.
test_full_argument_formatting: - Mock ContainerDataset objects now need .id attribute because _sandbox_input_filename() appends container_dataset.id - Expected command paths now include the ContainerDataset.id suffix: /mnt/input/optional_input -> /mnt/input/optional_input_100 ContainerRunCreateValidationTests (6 tests): - fixed positional input staging name matches argument name - optional single input includes ContainerDataset.id in staged name - serializer rejects missing/duplicate/misplaced multi_position - serializer allows same Dataset at two different positions RunContainerMultiInputTests (2 end-to-end tests): - fill_sandbox with duplicate dataset content produces 2 distinct files - every _format_kw_args path matches a file in sandbox/input
Add an expected-failure regression test proving that load_log leaves log_size unset for purge scanning.
Stop assigning ContainerLog.log_size in load_log while preserving stale storage cleanup.
Add an expected-failure regression test that forbids unbounded source-file reads.
Copy recovered log sources directly to the temporary log instead of buffering them in memory.
Add an expected-failure regression test proving save_exception appends without unbounded reads.
Check the existing log size and final byte, then append the diagnostic without unbounded reads.
Add expected-failure regression tests for wide state output and step-record diagnostics.
Request a wide state field, parse job-step records, and include same-job steps in failure diagnostics.
Add expected-failure regression tests for short and long logs containing invalid UTF-8.
Add an expected-failure integration test proving malformed application stderr does not lose the Slurm diagnostic.
Stream malformed process output safely while preserving log replacement and purge-size behavior.
Add a directory-relative-path length constant, migrate ContainerDataset.name, and cover its validation boundary.
Resolve the positional output-directory TODO in full command construction.
Verify ordinary outputs and deterministic directory members are saved alongside each other.
Verify empty and missing directory outputs leave ordinary outputs unaffected.
Cover the production output-collection path instead of its private helpers.
Increase diroutput confidence
Co-authored-by: rhliang <rliang@bccfe.ca>
Co-authored-by: rhliang <rliang@bccfe.ca>
Co-authored-by: rhliang <rliang@bccfe.ca>
Co-authored-by: rhliang <rliang@bccfe.ca>
Co-authored-by: rhliang <rliang@bccfe.ca>
This reverts commit b473ea0.
This reverts commit de87c80.
This reverts commit 7e55215.
This reverts commit b34610f.
Fix container run exit cases
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the Kive 0.17 development branch.
Main changes:
utils/dev;save_in_dbis omitted.Upgrade note
This PR changes how
ContainerRun.md5is calculated so that dataset-binding identity and ordering are represented correctly.As a result,
ContainerRun.md5values created by Kive 0.16 and earlier are not directly comparable with hashes created by Kive 0.17. A rerun created after upgrading may therefore appear as changed when compared with an otherwise identical pre-upgrade run.We intentionally do not migrate or version historical run hashes because existing runs are expected to be purged by now. Cross-version
has_changedresults should not be relied upon.