feat: archive init and wait container logs. Fixes #12640#16267
Conversation
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughAdds the opt-in ChangesArchive System Container Logs
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 ast-grep (0.44.1)manifests/quick-start-minimal.yamlast-grep skipped this file: it is too large to scan (11962847 bytes) manifests/quick-start-mysql.yamlast-grep skipped this file: it is too large to scan (11965039 bytes) manifests/quick-start-postgres.yamlast-grep skipped this file: it is too large to scan (11964973 bytes)
🔧 Checkov (3.3.8)manifests/quick-start-minimal.yamlCheckov skipped this file: it is too large to scan (11962847 bytes) manifests/quick-start-mysql.yamlCheckov skipped this file: it is too large to scan (11965039 bytes) manifests/quick-start-postgres.yamlCheckov skipped this file: it is too large to scan (11964973 bytes)
🔧 Trivy (0.72.0)Trivy execution failed: 2026-07-17T03:43:36Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: cloudformation scan error: fs filter error: fs filter error: walk error range error: stat .coderabbit-oasdiff.yaml: no such file or directory: range error: stat .coderabbit-oasdiff.yaml: no such file or directory 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.
🧹 Nitpick comments (4)
test/e2e/functional_test.go (1)
61-74: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider also asserting
init-logsabsence.The comment documents that
init-logsshould be absent sincewaitnever starts, but the test only checksstatus.Phase. Asserting the node's outputs don't containinit-logswould make this regression guard more precise and self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/functional_test.go` around lines 61 - 74, The regression test in TestArchiveSystemContainerLogsWhenInitFails only asserts the workflow phase, but it should also verify the documented behavior that init-logs is absent when wait never starts. Update the ExpectWorkflow assertion to inspect the workflow status/nodes and confirm the init node or its outputs do not contain init-logs, using the existing TestArchiveSystemContainerLogsWhenInitFails and status.Phase checks as the anchor for the new assertion.workflow/executor/executor.go (1)
748-759: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider logging when system container logs are skipped.
When
fs.ErrNotExistis hit for init/wait, the artifact is silently dropped with no trace. Since this is the documented failure mode for the feature's main debugging use case, a debug-level log noting the skip would aid troubleshooting without altering behavior.💡 Proposed addition
if err != nil { if errors.Is(err, fs.ErrNotExist) { + logging.RequireLoggerFromContext(ctx).WithField("container", containerName).Debug(ctx, "system container combined log file not found, skipping") continue } we.AddError(ctx, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/executor/executor.go` around lines 748 - 759, When `archiveSystem` is enabled in `we.saveContainerLogs` handling, the `fs.ErrNotExist` branch for `common.InitContainerName` and `common.WaitContainerName` currently skips artifacts silently; add a debug-level log in this branch so skipped system container logs are traceable without changing behavior. Use the existing `archiveSystem` loop in `executor.go` and the `saveContainerLogs` call site to include the container name and that the logs were not found.workflow/executor/executor_test.go (1)
676-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen "skip init and wait" test assertion.
The loop over
we.errorsis vacuously true ifwe.errorsis empty, so the test doesn't actually assert that no errors were recorded — only that if there happen to be any, none arefs.ErrNotExist. Add an explicit emptiness check to make the intended invariant a hard assertion.✅ Proposed fix
we.SaveLogs(ctx) + require.Empty(t, we.errors, "should not record any error when combined file does not exist") for _, err := range we.errors { assert.NotErrorIs(t, err, fs.ErrNotExist, "should not be a fatal error when combined file does not exist") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/executor/executor_test.go` around lines 676 - 697, The "skip init and wait when combined file does not exist" test in WorkflowExecutor.SaveLogs is too weak because iterating over we.errors does not assert that any errors were recorded. Update the test to explicitly assert that we.errors is empty after calling SaveLogs, while keeping the existing fs.ErrNotExist check for any unexpected entries, so the invariant is enforced by the test itself.cmd/argoexec/commands/logs_test.go (1)
33-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double-close via explicit call plus deferred
closer().Line 42 calls
closer()explicitly to flush before reading the file, and line 39'sdefer closer()will also fire on return, closing the same*os.Filetwice. This is functionally harmless (f.Close()errors are swallowed), but a cleaner pattern would drop the redundant defer or provide the production code with an explicit flush/close split, since relying on double-close obscures intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/argoexec/commands/logs_test.go` around lines 33 - 48, The test in logs_test.go is closing the same tee container log handle twice by both deferring closer in the t.Run body and calling closer explicitly before reading the combined file. Update the test to use a single close path: either remove the deferred closer and close once at the flush point, or adjust teeContainerLogs/closer semantics so the test can explicitly flush then close without a redundant second call. Use the teeContainerLogs and closer symbols to keep the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/argoexec/commands/logs_test.go`:
- Around line 33-48: The test in logs_test.go is closing the same tee container
log handle twice by both deferring closer in the t.Run body and calling closer
explicitly before reading the combined file. Update the test to use a single
close path: either remove the deferred closer and close once at the flush point,
or adjust teeContainerLogs/closer semantics so the test can explicitly flush
then close without a redundant second call. Use the teeContainerLogs and closer
symbols to keep the intent clear.
In `@test/e2e/functional_test.go`:
- Around line 61-74: The regression test in
TestArchiveSystemContainerLogsWhenInitFails only asserts the workflow phase, but
it should also verify the documented behavior that init-logs is absent when wait
never starts. Update the ExpectWorkflow assertion to inspect the workflow
status/nodes and confirm the init node or its outputs do not contain init-logs,
using the existing TestArchiveSystemContainerLogsWhenInitFails and status.Phase
checks as the anchor for the new assertion.
In `@workflow/executor/executor_test.go`:
- Around line 676-697: The "skip init and wait when combined file does not
exist" test in WorkflowExecutor.SaveLogs is too weak because iterating over
we.errors does not assert that any errors were recorded. Update the test to
explicitly assert that we.errors is empty after calling SaveLogs, while keeping
the existing fs.ErrNotExist check for any unexpected entries, so the invariant
is enforced by the test itself.
In `@workflow/executor/executor.go`:
- Around line 748-759: When `archiveSystem` is enabled in `we.saveContainerLogs`
handling, the `fs.ErrNotExist` branch for `common.InitContainerName` and
`common.WaitContainerName` currently skips artifacts silently; add a debug-level
log in this branch so skipped system container logs are traceable without
changing behavior. Use the existing `archiveSystem` loop in `executor.go` and
the `saveContainerLogs` call site to include the container name and that the
logs were not found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37e77fc7-d34a-4b4d-a27b-6d1698cbf4d4
⛔ Files ignored due to path filters (9)
api/openapi-spec/swagger.jsonis excluded by!**/api/openapi-spec/*.jsonpkg/apis/workflow/v1alpha1/generated.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/apis/workflow/v1alpha1/openapi_generated.gois excluded by!**/*_generated.gopkg/apis/workflow/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*.gosdks/java/client/docs/IoArgoprojWorkflowV1alpha1Artifact.mdis excluded by!**/sdks/java/client/**sdks/java/client/docs/IoArgoprojWorkflowV1alpha1ArtifactLocation.mdis excluded by!**/sdks/java/client/**sdks/java/client/docs/IoArgoprojWorkflowV1alpha1ArtifactPaths.mdis excluded by!**/sdks/java/client/**sdks/java/client/docs/IoArgoprojWorkflowV1alpha1ArtifactRepository.mdis excluded by!**/sdks/java/client/**sdks/java/client/docs/IoArgoprojWorkflowV1alpha1WorkflowSpec.mdis excluded by!**/sdks/java/client/**
📒 Files selected for processing (38)
.features/pending/archive-system-container-logs.mdapi/jsonschema/schema.jsoncmd/argoexec/commands/init.gocmd/argoexec/commands/logs.gocmd/argoexec/commands/logs_test.gocmd/argoexec/commands/wait.godocs/configure-archive-logs.mddocs/executor_swagger.mddocs/fields.mdmanifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yamlmanifests/base/crds/full/argoproj.io_cronworkflows.yamlmanifests/base/crds/full/argoproj.io_workflowartifactgctasks.yamlmanifests/base/crds/full/argoproj.io_workfloweventbindings.yamlmanifests/base/crds/full/argoproj.io_workflows.yamlmanifests/base/crds/full/argoproj.io_workflowtaskresults.yamlmanifests/base/crds/full/argoproj.io_workflowtasksets.yamlmanifests/base/crds/full/argoproj.io_workflowtemplates.yamlmanifests/base/crds/minimal/argoproj.io_workflowartifactgctasks.yamlmanifests/base/crds/minimal/argoproj.io_workfloweventbindings.yamlmanifests/base/crds/minimal/argoproj.io_workflowtaskresults.yamlmanifests/base/crds/minimal/argoproj.io_workflowtasksets.yamlmanifests/quick-start-minimal.yamlmanifests/quick-start-mysql.yamlmanifests/quick-start-postgres.yamlmanifests/quick-start-telemetry.yamlpkg/apis/workflow/v1alpha1/artifact_repository_types.gopkg/apis/workflow/v1alpha1/generated.protopkg/apis/workflow/v1alpha1/workflow_types.gopkg/plugins/executor/swagger.ymltest/e2e/functional_test.gotest/e2e/testdata/workflow-archive-init-fail.yamltest/e2e/testdata/workflow-archive-init-wait-logs.yamlui/src/shared/services/workflows-service.tsutil/logging/slog.goworkflow/controller/workflowpod.goworkflow/executor/executor.goworkflow/executor/executor_test.goworkflow/util/merge.go
- log a debug message when a system container's combined log file is missing and archiving is skipped - strengthen TestSaveLogs to assert exactly one error is recorded - remove double close in Test_teeContainerLogs Signed-off-by: myzk-a <rorosocksxion@gmail.com>
|
Addressed nitpicks #2–#4 in 284defa. Two notes:
|
Resolve conflicts from the init-less pod layout PR (argoproj#16161) and adapt system container log archiving to init-less mode: - SaveLogs derives the system container set from pod topology (supervisor in init-less, init/wait otherwise) - move the log tee into runAuxiliaryContainer so both wait and supervisor capture their own logs Signed-off-by: myzk-a <rorosocksxion@gmail.com>
…rgoproj#12640 Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
… sample Signed-off-by: myzk-a <rorosocksxion@gmail.com>
…-container-logs Signed-off-by: myzk-a <rorosocksxion@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workflow/controller/workflowpod.go (1)
1553-1575: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude system-log archiving when deciding whether to add the aux container. Resource/data templates still skip the wait/supervisor container when only
ArchiveSystemContainerLogsis enabled, becauseneedsAuxCtris driven bySaveLogsAsArtifact()only. That leaves nothing to collect the system logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/controller/workflowpod.go` around lines 1553 - 1575, Update the aux-container decision path, specifically the logic driven by needsAuxCtr and SaveLogsAsArtifact(), to also require the aux container when IsArchiveSystemContainerLogs(tmpl) is enabled. Preserve the existing behavior for regular log archiving and ensure resource/data templates retain the wait/supervisor container for system-log collection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@workflow/controller/workflowpod.go`:
- Around line 1553-1575: Update the aux-container decision path, specifically
the logic driven by needsAuxCtr and SaveLogsAsArtifact(), to also require the
aux container when IsArchiveSystemContainerLogs(tmpl) is enabled. Preserve the
existing behavior for regular log archiving and ensure resource/data templates
retain the wait/supervisor container for system-log collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e0cdba8-9650-4294-9b5c-a6b862c7e3b9
⛔ Files ignored due to path filters (3)
api/openapi-spec/swagger.jsonis excluded by!**/api/openapi-spec/*.jsonpkg/apis/workflow/v1alpha1/generated.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/apis/workflow/v1alpha1/openapi_generated.gois excluded by!**/*_generated.go
📒 Files selected for processing (24)
api/jsonschema/schema.jsondocs/executor_swagger.mddocs/fields.mddocs/workflow-controller-configmap.yamlmanifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yamlmanifests/base/crds/full/argoproj.io_cronworkflows.yamlmanifests/base/crds/full/argoproj.io_workflowartifactgctasks.yamlmanifests/base/crds/full/argoproj.io_workfloweventbindings.yamlmanifests/base/crds/full/argoproj.io_workflows.yamlmanifests/base/crds/full/argoproj.io_workflowtaskresults.yamlmanifests/base/crds/full/argoproj.io_workflowtasksets.yamlmanifests/base/crds/full/argoproj.io_workflowtemplates.yamlmanifests/base/crds/minimal/argoproj.io_workflowartifactgctasks.yamlmanifests/base/crds/minimal/argoproj.io_workfloweventbindings.yamlmanifests/base/crds/minimal/argoproj.io_workflowtaskresults.yamlmanifests/base/crds/minimal/argoproj.io_workflowtasksets.yamlmanifests/quick-start-minimal.yamlmanifests/quick-start-mysql.yamlmanifests/quick-start-postgres.yamlmanifests/quick-start-telemetry.yamlpkg/apis/workflow/v1alpha1/generated.protopkg/apis/workflow/v1alpha1/workflow_types.gopkg/plugins/executor/swagger.ymlworkflow/controller/workflowpod.go
🚧 Files skipped from review as they are similar to previous changes (20)
- docs/workflow-controller-configmap.yaml
- manifests/base/crds/minimal/argoproj.io_workflowartifactgctasks.yaml
- manifests/base/crds/full/argoproj.io_workflowtaskresults.yaml
- manifests/base/crds/full/argoproj.io_workflowartifactgctasks.yaml
- pkg/apis/workflow/v1alpha1/generated.proto
- manifests/base/crds/minimal/argoproj.io_workfloweventbindings.yaml
- manifests/base/crds/minimal/argoproj.io_workflowtaskresults.yaml
- pkg/apis/workflow/v1alpha1/workflow_types.go
- manifests/quick-start-minimal.yaml
- manifests/quick-start-mysql.yaml
- docs/fields.md
- api/jsonschema/schema.json
- manifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yaml
- manifests/base/crds/minimal/argoproj.io_workflowtasksets.yaml
- manifests/base/crds/full/argoproj.io_cronworkflows.yaml
- manifests/base/crds/full/argoproj.io_workfloweventbindings.yaml
- manifests/base/crds/full/argoproj.io_workflowtasksets.yaml
- docs/executor_swagger.md
- manifests/base/crds/full/argoproj.io_workflows.yaml
- manifests/quick-start-postgres.yaml
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
…iveSystemContainerLogs is set. Signed-off-by: myzk-a <rorosocksxion@gmail.com>
|
Good catch on the resource templates — fixed in 35fc008: Data templates are intentionally unchanged: they never get an aux container even with |
|
|
||
| ## Archiving Init and Wait Container Logs | ||
|
|
||
| By default, only the `main` container's logs are archived. Every Workflow Pod also runs Argo's own |
There was a problem hiding this comment.
Can we get one sentence per line please as per the docs guidelines? Just makes it much easier to diff in future. Thanks
There was a problem hiding this comment.
Thanks for the review, and for the pointer to the docs guidelines! Fixed in e5af2df — reformatted the whole section to one sentence per line.
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
|
Since #16161 (init-less pod layout) was merged, I've merged |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configure-archive-logs.md`:
- Line 71: Update the documentation sentence to hyphenate “garbage-collected”
when it modifies “Pods,” changing “garbage collected Pods” to “garbage-collected
Pods.”
- Line 68: Update the sentence describing Argo system containers to scope it to
legacy Workflow Pod layouts rather than every Workflow Pod, while preserving the
existing explanation of the init and wait containers and their log-archiving
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2a68a99-cdc0-418e-8455-baa65dca6537
📒 Files selected for processing (4)
docs/configure-archive-logs.mdtest/e2e/functional_test.goworkflow/controller/workflowpod.goworkflow/controller/workflowpod_initless_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- workflow/controller/workflowpod_initless_test.go
- test/e2e/functional_test.go
- workflow/controller/workflowpod.go
Signed-off-by: myzk-a <rorosocksxion@gmail.com>
…-container-logs Signed-off-by: myzk-a <rorosocksxion@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
workflow/controller/workflowpod_build_test.go (1)
43-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
archiveSystemContainerLogsto the stub and test the new feature.Since this PR introduces
archiveSystemContainerLogsand modifies theneedsAuxCtrlogic to consider it, thestubPodBuilderDepsshould support toggling this flag to allow pure testing of the new controller logic. Currently,IsArchiveSystemContainerLogshardcodesfalseon the stub.Consider adding an
archiveSystemContainerLogs boolfield tostubPodBuilderDepsand returning it inIsArchiveSystemContainerLogs.
Do you want me to generate a test case that verifiesarchiveSystemContainerLogs = truecorrectly schedules an auxiliary container (e.g., supervisor) for a resource template, similar to howTestBuild_InitlessResourceManifestFrom_SchedulesSupervisoris implemented?♻️ Proposed refactor for the stub
type stubPodBuilderDeps struct { executorImage string archiveLogs bool + archiveSystemContainerLogs bool initContainerName string // name of the init container legacyLayout asks for // gotTimeoutNow records the now value build threads into checkTemplateTimeouts, @@ -80,5 +81,5 @@ } func (s *stubPodBuilderDeps) IsArchiveLogs(_ *wfv1.Template) bool { return s.archiveLogs } -func (s *stubPodBuilderDeps) IsArchiveSystemContainerLogs(_ *wfv1.Template) bool { return false } +func (s *stubPodBuilderDeps) IsArchiveSystemContainerLogs(_ *wfv1.Template) bool { return s.archiveSystemContainerLogs } func (s *stubPodBuilderDeps) executorServiceAccountName(_ *wfv1.Template) string { return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/controller/workflowpod_build_test.go` around lines 43 - 84, Extend stubPodBuilderDeps with an archiveSystemContainerLogs bool field and update IsArchiveSystemContainerLogs to return that field instead of hardcoding false. Add a focused test covering archiveSystemContainerLogs=true for a resource template, asserting that build schedules the auxiliary supervisor container, following the pattern of TestBuild_InitlessResourceManifestFrom_SchedulesSupervisor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yaml`:
- Around line 969-972: Update every archiveSystemContainerLogs schema
description identified in the comment to describe both supported Pod layouts:
use “system-container logs,” explicitly mention init/wait containers for Pods
with init containers, and supervisor logs for init-less Pods. Keep the field
name and boolean schema unchanged across all occurrences.
In `@manifests/base/crds/full/argoproj.io_cronworkflows.yaml`:
- Around line 1059-1062: Update the API comment for archiveSystemContainerLogs
in the workflow types definition to document that supervisor logs are also
archived for init-less Pods, alongside init/wait container logs. Regenerate all
affected CRD manifests so every generated description reflects the complete
behavior.
In `@manifests/base/crds/full/argoproj.io_workflows.yaml`:
- Around line 982-985: Update every generated CRD description for
archiveSystemContainerLogs to describe archiving system-container logs,
explicitly covering legacy init/wait containers and init-less supervisor
containers. Apply the wording consistently across all occurrences in the
manifest.
In `@manifests/base/crds/full/argoproj.io_workflowtemplates.yaml`:
- Around line 967-970: The archiveSystemContainerLogs descriptions currently
mention only init/wait containers; update the descriptions at
manifests/base/crds/full/argoproj.io_workflowtemplates.yaml lines 967-970,
1013-1016, 3949-3952, 18168-18171, 22382-22386, 23730-23734, 25267-25270,
28073-28076, 29482-29485, 30784-30787, 35342-35346, and 36668-36672 to also
document supervisor logs for init-less Pods, using layout-neutral wording
consistently.
In `@manifests/quick-start-postgres.yaml`:
- Around line 969-972: Update the source comment for archiveSystemContainerLogs
to mention supervisor alongside init/wait containers, then regenerate the
manifests so the generated schema descriptions reflect that init-less Pods also
archive supervisor logs.
---
Nitpick comments:
In `@workflow/controller/workflowpod_build_test.go`:
- Around line 43-84: Extend stubPodBuilderDeps with an
archiveSystemContainerLogs bool field and update IsArchiveSystemContainerLogs to
return that field instead of hardcoding false. Add a focused test covering
archiveSystemContainerLogs=true for a resource template, asserting that build
schedules the auxiliary supervisor container, following the pattern of
TestBuild_InitlessResourceManifestFrom_SchedulesSupervisor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f027fa26-ff5d-4ddf-979d-c486d294903d
⛔ Files ignored due to path filters (5)
api/openapi-spec/swagger.jsonis excluded by!**/api/openapi-spec/*.jsonpkg/apis/workflow/v1alpha1/generated.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/apis/workflow/v1alpha1/openapi_generated.gois excluded by!**/*_generated.gopkg/apis/workflow/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*.gosdks/java/client/docs/IoArgoprojWorkflowV1alpha1WorkflowSpec.mdis excluded by!**/sdks/java/client/**
📒 Files selected for processing (16)
api/jsonschema/schema.jsondocs/fields.mdmanifests/base/crds/full/argoproj.io_clusterworkflowtemplates.yamlmanifests/base/crds/full/argoproj.io_cronworkflows.yamlmanifests/base/crds/full/argoproj.io_workflows.yamlmanifests/base/crds/full/argoproj.io_workflowtemplates.yamlmanifests/quick-start-minimal.yamlmanifests/quick-start-mysql.yamlmanifests/quick-start-postgres.yamlmanifests/quick-start-telemetry.yamlpkg/apis/workflow/v1alpha1/generated.protopkg/apis/workflow/v1alpha1/workflow_types.goworkflow/controller/workflowpod.goworkflow/controller/workflowpod_build_test.goworkflow/controller/workflowpod_initless_test.goworkflow/util/merge.go
🚧 Files skipped from review as they are similar to previous changes (10)
- workflow/util/merge.go
- pkg/apis/workflow/v1alpha1/workflow_types.go
- pkg/apis/workflow/v1alpha1/generated.proto
- api/jsonschema/schema.json
- workflow/controller/workflowpod.go
- manifests/quick-start-minimal.yaml
- manifests/quick-start-mysql.yaml
- docs/fields.md
- workflow/controller/workflowpod_initless_test.go
- manifests/quick-start-telemetry.yaml
…scriptions. Signed-off-by: myzk-a <rorosocksxion@gmail.com>
… tests. Signed-off-by: myzk-a <rorosocksxion@gmail.com>
Done in 7c47be2 — added the toggle to |
2 participants
Fixes #12640
Motivation
Today Argo only archives the
maincontainer's logs. When a Pod is garbage collected, the logs of Argo's own system containers — theinitcontainer (which loads input artifacts) and thewaitcontainer (which saves outputs and logs), or the singlesupervisorcontainer that plays both roles under the init-less pod layout (#16161) — are lost forever.This makes it hard to debug problems that happen outside the user's
maincontainer after the Pod is gone, for example:wait,initcontainer loaded.This PR lets users opt in to archiving the system container logs as artifacts, controlled independently from
archiveLogsso you only pay to store them when you need them.Modifications
Added a new
archiveSystemContainerLogsflag, settable at the controller config-map, workflow spec, or template level, following the same priority rules as the existingarchiveLogs.Since #16161 introduced the opt-in init-less pod layout, the set of system containers depends on the layout, and this PR supports both:
init-logs,wait-logssupervisor-logsinitfails andwaitnever starts (documented limitation)supervisor-logsis archived, because the supervisor still runs its post-main phaseArchitecture (after this change)
The system containers tee their own logs into per-container
combinedfiles on the shared/var/run/argoemptyDir volume — the same convention the emissary already uses formain. The auxiliary container (wait, orsupervisorin init-less mode) then reads those files back inSaveLogsand uploads each as an artifact, alongside the existingmain-logs. The tee is unconditional (the flag is not yet readable at that point); the archiving decision is made once, inSaveLogs.flowchart TD subgraph Pod["Workflow Pod (legacy layout)"] direction TB Init["init container<br/>(argoexec init)"] Main["main container<br/>(user cmd via emissary)"] Wait["wait container<br/>(argoexec wait)"] Vol[("/var/run/argo/ctr<br/>shared emptyDir<br/>init/combined · main/combined · wait/combined")] Init -- tee --> Vol Main -- emissary tee --> Vol Wait -- tee --> Vol Vol -- "wait reads back (SaveLogs)" --> Wait end Wait -- upload --> Store["Artifact storage<br/>main-logs · init-logs (new) · wait-logs (new)"]flowchart TD subgraph Pod2["Workflow Pod (init-less layout)"] direction TB Sup["supervisor container<br/>(argoexec supervisor)"] Main2["main container<br/>(user cmd via emissary)"] Vol2[("/var/run/argo/ctr<br/>shared emptyDir<br/>supervisor/combined · main/combined")] Sup -- tee --> Vol2 Main2 -- emissary tee --> Vol2 Vol2 -- "supervisor reads back (SaveLogs)" --> Sup end Sup -- upload --> Store2["Artifact storage<br/>main-logs · supervisor-logs (new)"]pkg/apis/workflow/v1alpha1/): added theArchiveSystemContainerLogsfield toArtifactRepositoryand the workflow/template types, plus aSaveSystemContainerLogsAsArtifact()helper mirroring the existingSaveLogsAsArtifact().workflow/controller/workflowpod.go): addedIsArchiveSystemContainerLogs(tmpl), a faithful copy ofIsArchiveLogswith the samecontroller(on) > template > workflow > controller(off)priority.cmd/argoexec/commands/auxiliary.go: the tee lives inrunAuxiliaryContainer, the lifecycle scaffolding shared by the legacywaitcommand and the init-lesssupervisorcommand, so both layouts are covered in one place. The legacy-onlyinitcommand (init.go) sets up its own tee, since it does not go throughrunAuxiliaryContainer. A tee setup failure is treated as a log-archive failure and does not block the container's primary work.util/logging/slog.go: addedTeeLoggerand amultiHandlerso a logger can fan out to its original destination and the combined file, preserving fields, format, and hooks.workflow/executor/executor.go:SaveLogsderives the system container names from the pod layout (common.IsInitlessPod()) and uploads their combined files as<container>-logsartifacts when the flag is set. The auxiliary container's own log is saved last so it includes the preceding save operations; a missing combined file is skipped viafs.ErrNotExistas a safety net for tee setup failure.ui/src/shared/services/workflows-service.ts): dropped thecontainer === 'main'restriction so archived logs of non-main containers can be viewed after the Pod is gone.Verification
workflow/executor/executor_test.go—TestSaveLogs: covers each flag combination, theErrNotExistskip, that the auxiliary container is saved last, and the init-less cases (t.Setenv(common.EnvVarInitlessPod, "true")switches the targets tosupervisor).cmd/argoexec/commands/logs_test.go—Test_teeContainerLogs: combined file creation and write tee-through.fixtures.AuxContainerName()/E2E_INITLESS, exercised by both the regular and theinitless: trueCI dimensions):TestArchiveSystemContainerLogs— verifiesmain-logsand the per-layout system log artifacts exist on a succeeded Pod.TestArchiveSystemContainerLogsWhenArtifactLoadFails— legacy: a failinginitcontainer still ends cleanly in theWorkflowErrorphase (regression guard for the tee added ininit.go); init-less:supervisor-logsis archived even though pre-main failed.make pre-commit -Bpasses (codegen + lint + docs + features-validate), andgo build ./...is clean.Documentation
docs/configure-archive-logs.md, including an example manifest and the init-less behavior..features/pending/archive-system-container-logs.md.initcontainer is not captured, because thewaitcontainer never starts to archive it (the init-less layout does capture this case).AI
Claude Code was used to help review the implementation, write tests, and draft the docs, commit messages, and this PR description. All changes were reviewed by the author.
Summary by CodeRabbit
Summary by CodeRabbit
archiveSystemContainerLogsto opt-in archiving of Argo system container logs (init/wait in legacy pods, supervisor in init-less) asinit-logs,wait-logs, orsupervisor-logs.archiveLogs.