[1839] Fix: Truncate status.deploy.stdout as it can exceed etcd's limit on large clusters, causing reconciliation delay

[himsngh]

What this PR does / why we need it:

On large clusters (>~250 nodes), a kapp deploy can produce several MiB of change output (one line per resource per node). kapp-controller writes this verbatim into status.deploy.stdout and then calls UpdateStatus on the App CR. etcd rejects objects larger than its hard 2 MiB gRPC message limit, causing the status write to fail repeatedly. The deployment itself has already succeeded, only the status update is failing but the App stays in ReconcileFailed until a subsequent no-op reconcile produces a small enough diff to fit.

This PR fixes the issue by truncating individual status output fields to 1 MiB before they are written into the status struct. When truncation occurs the field is prefixed with [output truncated]\n so operators immediately know the field is incomplete rather than receiving silently clipped output. The tail of the output is kept (not the head) because the most actionable content, the final resource summary and any error lines, always appears at the end of kapp output.

Fields covered: status.deploy.stdout, status.deploy.stderr, status.fetch.stdout, status.fetch.stderr, status.inspect.stdout, status.inspect.stderr, status.usefulErrorMessage.

Which issue(s) this PR fixes:

Fixes #1839

Does this PR introduce a user-facing change?

NA

Additional Notes for your reviewer:

Review Checklist:

• Follows the developer guidelines
• Relevant tests are added or updated
• [-] Relevant docs in this repo added or updated
• [-] Relevant carvel.dev docs added or updated in a separate PR and there's
  a link to that PR
• Code is at least as readable and maintainable as it was before this
  change

Additional documentation e.g., Proposal, usage docs, etc.:

[aroradaman]

I see we already have WithFriendlyYAMLStrings which returns modified CmdRunResult. Can we follow the same pattern here?, may be add WithTruncatedStrings(size).

(ref:

kapp-controller/pkg/exec/cmd_run_result.go

Lines 53 to 63 in be1faef

	func (r CmdRunResult) WithFriendlyYAMLStrings() CmdRunResult {
	// YAML can format muliline strings nicely
	// if they do not have trailing spaces right before newlines
	return CmdRunResult{
	Stdout: trailingSpace.ReplaceAllString(strings.TrimSpace(r.Stdout), "\n"),
	Stderr: trailingSpace.ReplaceAllString(strings.TrimSpace(r.Stderr), "\n"),
	ExitCode: r.ExitCode,
	Error: r.Error,
	Finished: r.Finished,
	}
	}

)

[himsngh]

I see we already have WithFriendlyYAMLStrings which returns modified CmdRunResult. Can we follow the same pattern here?, may be add WithTruncatedStrings(size).

(ref:

kapp-controller/pkg/exec/cmd_run_result.go

Lines 53 to 63 in be1faef

	func (r CmdRunResult) WithFriendlyYAMLStrings() CmdRunResult {
	// YAML can format muliline strings nicely
	// if they do not have trailing spaces right before newlines
	return CmdRunResult{
	Stdout: trailingSpace.ReplaceAllString(strings.TrimSpace(r.Stdout), "\n"),
	Stderr: trailingSpace.ReplaceAllString(strings.TrimSpace(r.Stderr), "\n"),
	ExitCode: r.ExitCode,
	Error: r.Error,
	Finished: r.Finished,
	}
	}

)

Updated!

[aroradaman]

Review — testing only

Focusing on the test changes here (code changes look good). I reviewed the three test files against the actual source (WithFriendlyYAMLStrings, the NewApp signature, existing e2e conventions), not just the diff.

Overall

Unit-level testing is genuinely strong; the e2e test is the weak link.

What's good

• pkg/exec/cmd_run_result_test.go is excellent. Table-driven and covers the cases that matter for truncateOutput: under-limit, exactly-at-limit, empty, over-limit tail-preservation, and two UTF-8 boundary cases (continuation byte skipped, pure multibyte runes producing valid UTF-8). It also correctly asserts the marker is not counted toward maxBytes. This is the core logic and it's well locked down.
• app_status_truncate_test.go exercises the real methods (updateLastDeploy, setUsefulErrorMessage) through a clean helper, and the tail-preservation test correctly accounts for the WithFriendlyYAMLStrings → strings.TrimSpace interaction (TrimSpace strips the trailing \n, so the comment is accurate and Contains is robust either way).
• Using a configurable Opts.MaxStatusOutputBytes so tests run with a limit of 50 instead of generating multi-MiB strings — fast and deterministic.

Gaps / issues (roughly by importance)

1. Fetch and inspect truncation paths are untested. The diff adds .WithTruncatedStrings(...) in three status paths — deploy, fetch (reconcileFetchTemplateDeploy), and inspect (reconcileInspect) — but the tests only cover deploy and usefulErrorMessage. A regression that dropped the WithTruncatedStrings call from the fetch or inspect path would pass the entire suite. Please add assertions on Status.Fetch.Stdout/Stderr and Status.Inspect.Stdout/Stderr.

2. The e2e test never exercises truncation. It deploys 30 ConfigMaps (~KB), so strings.HasPrefix(stdout, marker) is always false and the "bounded" assertion is trivially true. It's really a happy-path capture test that overlaps existing deploy e2e coverage — the name ..._CapturedAndBounded oversells what it verifies. The comment acknowledges generating >1 MiB in-cluster is impractical, which is fair — but the truncation limit isn't runtime-configurable (only via Opts in code), so there's no way to drive this path from e2e today. Options: rename to reflect it's a capture test, or make the cap configurable via a controller flag so e2e can set a tiny limit and assert the marker appears. As-is its incremental value over existing tests is low.

3. The e2e encodes an incorrect invariant. The test asserts len(stdout) < 1 MiB with the comment "must never exceed the 1 MiB cap." But truncateOutput returns TruncationMarker + last maxBytes, so a truncated field is up to 1 MiB + len(marker) (≈1 MiB + 19 bytes). The assertion only passes today because output is tiny; if truncation ever actually occurred, this assertion would fail. The real bound is len(marker) + maxBytes.

4. Default-limit fallback branch has no coverage. Every test passes an explicit Opts.MaxStatusOutputBytes, so the == 0 → defaultMaxStatusOutputBytes branch of maxOutputBytes() is never hit. A one-liner asserting maxOutputBytes() returns 1 MiB when Opts is zero closes it.

5. e2e indexes cr.Status.Conditions[0] without a non-empty guard. If conditions were empty this panics (index out of range) instead of failing cleanly — prefer require.NotEmpty(...) first. Also, existing e2e tests (app_status_test.go) assert the full condition (Type/Status/Message); asserting only .Type is slightly less thorough but consistent enough.

6. Minor: Test_WithTruncatedStrings only covers the both-fields-over-limit case. A mixed case (one field under the limit passed through verbatim, the other truncated) would better lock per-field behavior. Low priority since truncateOutput's passthrough is already tested.

Bottom line

Unit tests for the truncation primitive are thorough and correct. Before merge I'd want: (1) fetch/inspect truncation assertions, (3) the incorrect 1 MiB invariant fixed, and (4) the default-fallback covered. The e2e test (2) is the judgment call — it's currently a capture test wearing a truncation-test name.