Repository navigation
feat(application): add continuous recording to 503 media capture service - #848
Alain Uyidi (auyidi1) wants to merge 5 commits into
Conversation
📚 Documentation Health ReportGenerated on: 2026-10-06 08:48:01 UTC 📈 Documentation Statistics
🏗️ Three-Tree Architecture Status
🔍 Quality Metrics
This report is automatically generated by the Documentation Automation workflow. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
==========================================
+ Coverage 42.15% 44.62% +2.46%
==========================================
Files 42 45 +3
Lines 7334 8139 +805
==========================================
+ Hits 3092 3632 +540
- Misses 4242 4507 +265
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Katrien De Graeve (katriendg)
left a comment
Katrien De Graeve (katriendg)
left a comment
There was a problem hiding this comment.
Thanks for adding continuous recording and the accompanying unit tests. A few inline comments for the recording correctness issues noted inline: startup cleanup can remove another pod's active segment, metadata assumes the requested duration rather than the footage actually captured, and retention leaves old-format videos behind while deleting their sidecars.
|
|
||
| // Segments left by an interrupted earlier run are incomplete | ||
| let camera_path = self.config.output_base_path.join(&self.config.camera_id); | ||
| match remove_partial_segments(&camera_path).await { |
There was a problem hiding this comment.
Can we distinguish abandoned partial files from files another recorder is still writing before deleting them? This removes every .partial file for the camera, including an active recording owned by another pod. The chart uses the default rolling-update strategy, so pods can overlap even with replicaCount: 1; additional replicas share the same camera directory too.
A local probe confirmed that cleanup unlinks an open recording file and the original writer then cannot rename it to its final path. Please enforce exclusive ownership per camera or make cleanup ownership-aware, and add a regression test proving startup cannot delete another active recorder's segment.
There was a problem hiding this comment.
Fixed in c83e654. Partial cleanup is now ownership-safe without a lock. Startup, and every cleanup run, only deletes partial files that haven't been modified for 2 minutes. An active ffmpeg keeps rewriting its partial file, and 2 minutes covers the 10 s RTSP I/O timeout plus the 60 s ffmpeg grace period. That works even when the volume is shared across nodes, where file locks may not hold. startup_keeps_active_partial_and_removes_abandoned_one keeps a writer appending to one partial file while startup cleanup runs, then renames it after cleanup, and checks that only the idle partial was removed. To avoid overlap at all, continuous mode now uses the Recreate rollout strategy, and the chart fails to render with replicaCount above 1.
| } | ||
| fs::rename(&partial, &output).await?; | ||
|
|
||
| let segment_end = segment_start + chrono::Duration::from_std(self.config.segment_duration)?; |
There was a problem hiding this comment.
Can we validate the actual captured duration before publishing this metadata? -t limits FFmpeg's output duration; a successful exit does not prove that the requested duration was recorded. An early stream EOF can produce a shorter valid file, but this code still advertises the full configured interval. Also, segment_start is captured before connecting to the camera, so connection/setup time shifts the reported start away from the captured footage.
Please derive the interval from capture timing and measured media duration, and explicitly handle short or empty output. Add tests for delayed startup and an early-ending stream, including a successful process exit with less footage than requested, so time-range queries do not report coverage that the video does not contain.
There was a problem hiding this comment.
Fixed in c83e654. After ffmpeg exits, ffprobe measures the file. The window ends when ffmpeg finished and starts the measured duration earlier, so connection and setup time is excluded. The segment file name now uses that start, keeping it consistent with the metadata. Recordings under 1 s are rejected, which covers an empty file from a successful exit. A short segment keeps its real duration and logs a warning. Tests cover delayed startup, an early-ending stream with a successful exit, empty and near-empty output, and tolerance. A real ffprobe test measures a generated clip. End to end against MediaMTX, a stream that stopped mid-segment produced a 3.0 s segment with 3 s metadata. A long keyframe interval reproduced your exact case: ffmpeg exited 0 with an empty file, and it's now rejected instead of published.
|
|
||
| fn start_cleanup_task(&self, retention: Duration) { | ||
| let camera_path = self.config.output_base_path.join(&self.config.camera_id); | ||
| let extension = self.config.output_format.extension(); |
There was a problem hiding this comment.
Can retention handle both supported video formats, regardless of the current recording setting? After switching OUTPUT_FORMAT from mp4 to mkv, cleanup only matches MKV videos, but is_segment_file still matches every JSON sidecar. A local probe confirmed that an expired MP4 remains while its metadata is deleted.
This leaves old-format recordings outside the application's retention policy and separates them from their metadata. Please clean up expired MP4 and MKV segments consistently while preserving triggered clips, and add regression coverage for format changes in both directions.
There was a problem hiding this comment.
Fixed in c83e654. Retention now matches segment_* videos in both supported formats (mp4 and mkv) whatever the current OUTPUT_FORMAT is, plus their .json sidecars and stale partial files. Triggered clips don't use the segment_ prefix and are still preserved. retention_cleans_every_video_format_after_format_changes expires an MP4 and an MKV segment with sidecars alongside triggered clips in both formats and recent segments, and checks that only the expired segments and sidecars are removed.
ec4dde2 to
c83e654
Compare
📚 Documentation Health ReportGenerated on: 2026-10-07 17:53:45 UTC 📈 Documentation Statistics
🏗️ Three-Tree Architecture Status
🔍 Quality Metrics
This report is automatically generated by the Documentation Automation workflow. |
- record back-to-back ffmpeg segments with JSON metadata to the ACSA volume - write hourly camera paths shared with triggered clips and the query API - validate CAMERA_ID, redact RTSP credentials, rename only complete segments - add chart values, mock RTSP cameras for 508, and an ADR 🎥 - Generated by Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hip-safe - derive segment window from ffprobe duration and ffmpeg finish time - reject empty recordings; name segments after the actual footage start - remove only idle partial files so active recorders keep their segments - apply retention to mp4 and mkv segments; Recreate strategy in continuous mode 🎥 - Generated by Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- write in-progress segments to a staging dir on the same volume - verify staging is outside the synced dir and on the same filesystem - require a camera ID in the chart when continuous recording is enabled - add MEDIA_STAGING_DIR and mediaCapture.storage.stagingDir 🎥 - Generated by Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c83e654 to
a19632e
Compare
📚 Documentation Health ReportGenerated on: 2026-10-08 11:39:20 UTC 📈 Documentation Statistics
🏗️ Three-Tree Architecture Status
🔍 Quality Metrics
This report is automatically generated by the Documentation Automation workflow. |
…849) # Pull Request ## Description Lets `111-assets` express media connector file-system tasks, and moves the media connector examples and the 508 README to the documented `streams` format. [Configure the media connector](https://learn.microsoft.com/azure/iot-operations/discover-manage-assets/howto-use-media-connector) configures tasks as asset streams. Each stream has a `streamConfiguration` (`taskType`, `autostart`, `format`, `snapshotsPerSecond`, `duration`) and a destination: `Mqtt` with `topic`, or `Storage` with `path`. This PR fixes four problems: * **`111-assets` couldn't express a `Storage` destination.** The stream destination type allowed only `topic`, `retain`, and `qos`. Terraform silently dropped `path`, and Bicep warned with `BCP037`. * **The 508 README and `alert-dataflow.tfvars.example` used the old `datasets` format.** They also used settings the media connector doesn't document (`intervalSeconds`, `quality`, `storagePath`, `durationSeconds`), and paths like `/clips`, which the connector can't write. Learn requires a mounted volume root or a path under `/tmp`. * **The 508 README device examples didn't match `namespaced_devices`.** They used `endpoint`, `target_address`, and `username_secret_ref`, and pointed to a `media-connector-assets.tfvars.example` that doesn't exist. * **The `111-assets` CI wrapper's `namespaced_assets` variable had drifted from the component.** It had no `event_groups` or `management_groups` and used old field names, so Terraform would silently drop ONVIF PTZ actions and event groups. This was noted as a follow-up in #847. ## Related Issue Follow-up to #847 and #848. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [x] Blueprint modification or addition - [x] Component modification or addition - [x] Documentation update - [ ] CI/CD pipeline change - [ ] Other (please describe): ## Implementation Details * **Component (Terraform):** `namespaced_assets[].streams[].destinations[].configuration` adds optional `path`. `main.tf` now omits unset destination settings instead of sending them as `null`. * **Component (Bicep):** a new exported `AssetStreamDestination` type adds optional `path`; `AssetStream.destinations` uses it. Dataset destinations are unchanged. * **Blueprints:** `full-multi-node-cluster`, `minimum-single-node-cluster`, and `dual-peered-single-node-cluster` keep `namespaced_assets` identical to the component. The Bicep blueprints import the component types. * **CI wrapper:** `src/100-edge/111-assets/ci/terraform` `namespaced_assets` is replaced with the component definition. * **`alert-dataflow.tfvars.example`:** media assets now use `streams`: * snapshots to MQTT; * a 30-second MKV clip and snapshots to `Storage` paths under `/tmp`, with a comment explaining that the blueprint's connector templates don't mount a volume; * credential references in `<secret>/<key>` form. * **508 README:** * Device, asset, and scenario examples are rewritten to the component schema. * The task-type table lists the documented settings and destinations. * `Storage` path rules are explained. * The live-streaming scenario uses `az iot ops ns asset media stream add`, because the RTSP media server settings vary by connector version. * The README's `ms.date` changes to ISO format; #848 makes the same edit. * **Generated READMEs** are regenerated with `npm run tf-docs` and `npm run bicep-docs`. ## Testing Performed - [x] Terraform plan/apply - [ ] Blueprint deployment test - [ ] Unit tests - [ ] Integration tests - [ ] Bug fix includes regression test (see [Test Policy](docs/contributing/testing-validation.md)) - [x] Manual validation - [x] Other: Bicep build and parameter compilation; TFLint ## Validation Steps 1. `terraform fmt -check`, `terraform init -backend=false`, and `terraform validate` pass for the component and the CI wrapper. TFLint with `.tflint.hcl` passes for the component, the CI wrapper, and the three blueprints. 2. `terraform plan` on a root module with every `full-multi-node-cluster` variable definition: * accepts `alert-dataflow.tfvars.example` and each of the five new HCL snippets in the 508 README; * shows, via the same expression `main.tf` uses, that request bodies carry only `path` for `Storage` destinations and only the set fields for `Mqtt` destinations. * The example and the new snippets also pass `terraform fmt -check`. 3. Bicep: * `bicep build` passes for the `111-assets` component and CI wrapper and for the `full-multi-node-cluster`, `minimum-single-node-cluster`, and `only-edge-iot-ops` blueprints. * `bicep build-params` with a `Storage` destination compiles and passes `path` through. On `main`, the same parameters warn with `BCP037`. * `bicep format` leaves `types.bicep` unchanged. 4. `scripts/tf-docs-check.sh` and `scripts/bicep-docs-check.sh` report no pending updates. 5. markdownlint, markdown-table-formatter, cspell, and markdown-link-check pass for the 508 README. Gitleaks finds no leaks. ## Checklist - [x] I have updated the documentation accordingly - [ ] I have added tests to cover my changes - [ ] All new and existing tests passed - [x] I have run `terraform fmt` on all Terraform code - [x] I have run `terraform validate` on all Terraform code - [x] I have run `az bicep format` on all Bicep code - [x] I have run `az bicep build` to validate all Bicep code - [x] I have checked for any sensitive data/tokens that should not be committed - [x] Lint checks pass (run applicable linters for changed file types) ## Security Review - [x] No credentials, secrets, or tokens are hardcoded or logged - [x] RBAC and identity changes follow least-privilege principles - [x] No new network exposure or public endpoints introduced without justification - [x] Dependency additions or updates have been reviewed for known vulnerabilities - [ ] Container image changes use pinned digests or SHA references ## Additional Notes * **Not deployed to a live cluster.** * **Possible follow-up:** let the Akri connector template module mount a volume, so file-system tasks can write to persistent storage instead of `/tmp`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📚 Documentation Health ReportGenerated on: 2026-10-08 13:22:16 UTC 📈 Documentation Statistics
🏗️ Three-Tree Architecture Status
🔍 Quality Metrics
This report is automatically generated by the Documentation Automation workflow. |
📚 Documentation Health ReportGenerated on: 2026-10-08 13:40:05 UTC 📈 Documentation Statistics
🏗️ Three-Tree Architecture Status
🔍 Quality Metrics
This report is automatically generated by the Documentation Automation workflow. |
This branch has not been deployed
4 participants
Pull Request
Description
Adds a continuous recording mode to
503-media-capture-service. The service records back-to-back segments from an RTSP stream withffmpeginto the ACSA cloud-backed volume and writes a JSON metadata file next to each segment. ACSA uploads both to Blob Storage, where the video query API from #846 lists them by camera and hour. The PR also adds mock RTSP cameras for508-media-connectorand an ADR.This is the third focused replacement for #468 (after #846 and #847).
Related Issue
Replaces part of #468.
Type of Change
Implementation Details
Breaking Change
Triggered clips now go to
{MEDIA_CLOUD_SYNC_DIR}/{CAMERA_ID}/{YYYY}/{MM}/{DD}/{HH}/instead of{MEDIA_CLOUD_SYNC_DIR}/{YYYY-MM-DD}/. Continuous segments and triggered clips therefore share one prefix per camera and hour. Nothing else in the repository reads the old layout, and the README calls out the change.Continuous Recording
CONTINUOUS_RECORDING_ENABLED=true(chart:mediaCapture.continuousRecording.enabled, defaultfalse). Triggered mode remains the default.ffmpegwrites to<segment>.partial, and the file is renamed only afterffmpegsucceeds, so ACSA never sees a half-written segment under a final name. Partial files from an interrupted run are deleted at startup.camera_id,location,segment_start,segment_end,duration_seconds, andfile_name. These are the fields the query API reads.LOCAL_RETENTION_HOURSare deleted;0disables cleanup. Onlysegment_*files are deleted, never triggered clips. Directories are removed only when they're empty and older than the cutoff.Review Round 1 (Katrien De Graeve (@katriendg))
Recreaterollout strategy and fails to render withreplicaCountabove 1, so recorders for one camera never overlap.ffprobemeasures the file. The window ends when ffmpeg finishes and starts the measured duration earlier, so connection time isn't counted. The segment file name uses the same start.OUTPUT_FORMATis. Triggered clips are still preserved.ffmpegran through blockingstd::process::Command::output()inside an async function, holding a Tokio worker for the whole segmenttokio::process::Commandwithkill_on_drop, plus a timeout of segment length + 60 sffmpegstderr was logged on failure, and it echoes the RTSP URL including credentialsCAMERA_IDwas used in file system paths without validation[A-Za-z0-9_-]{1,128}, the same rule as the video query API, in both modesOUTPUT_FORMATwas passed toffmpeg -funcheckedmp4ormkvexpect()panicked on missing environment variables.jsonfiles, so directories were never removedmd5dependency, contradicting the query API's prefix layoutCargo.tomlandCargo.lockare unchangedfeedDelaySecondsunder the new block (soVIDEO_FEED_DELAY_SECONDSrendered empty), setCAMERA_IDonly in continuous mode, and added an unusedAZURE_STORAGE_CONNECTION_STRINGfalse;feedDelaySecondsleft in place;mediaCapture.camera.idandlocationset in both modes; no storage connection stringyaml/media-capture-mqtt-triggered.yamlwith a site-specific registry, IP address, camera names, and topicsdocker-compose.ymlchanged a pinned Mosquitto image to:latestazure-iot-operations, with an unpinned imagemock-camerasnamespace, image pinned by digest, running as non-root with a read-only root file system, all capabilities dropped, and resource limitsTesting Performed
Validation Steps
Round 1 adds 7 tests, and
cargo test --lockedpasses all 36:ffprobeoutput parsing;ffprobemeasurement of a generated clip, skipped when ffmpeg isn't installed.End to end against MediaMTX with a stream that stops mid-segment, the recorder wrote a 10.0 s segment and a 3.0 s segment. Their metadata matched the
ffprobedurations, and the start times excluded the connection delay. A stream with a long keyframe interval made ffmpeg exit 0 with an empty file, which is now rejected.Ran the
rust-tests.ymljob steps in an Ubuntu 24.04 container with the same apt packages and stable Rust:cargo test --locked: 29 tests pass, 9 of them new. They cover camera ID validation, credential redaction, output formats, the path layout, metadata fields, partial-file handling, and cleanup.cargo clippy --all-targets: no warnings in new or changed code. The remaining warnings are already onmain.rustfmt --checkpasses on the new files.End to end with MediaMTX and an
ffmpegtest source:.partialfile, which the next start removes.helm lintpasses.helm templaterendersCAMERA_ID,VIDEO_FEED_DELAY_SECONDS, and the continuous variables correctly in both modes.kubeconform -strictvalidates all 7 mock camera resources. The/live.sdp/<route>stream path was confirmed against the pinned image running as non-root with a read-only root file system.markdownlint, markdown-table-formatter, cspell, and yamllint pass. The frontmatter warnings and two dead links reported for the changed files are in unchanged lines on
main, except the 508 README date, which this PR fixes. Gitleaks finds no leaks.Checklist
terraform fmton all Terraform codeterraform validateon all Terraform codeaz bicep formaton all Bicep codeaz bicep buildto validate all Bicep codeSecurity Review
Additional Notes
admin/password, matching508-media-connector/docker-compose.yml, and creates cluster-internal services only.full-multi-node-cluster/terraform/alert-dataflow.tfvars.exampledescribe media connector tasks withdatasetsandintervalSeconds. Configure the media connector documentsstreamswithautostart,snapshotsPerSecond, andduration, and storage destinations for file paths. feat(application): add video capture query blueprint, 520-video-query-api, and ONVIF camera tooling #468's rewrite of that section mixed both schemas, so it isn't included here.