hack: don't let artifact tar warnings fail otherwise-green CI runs

[pujitha24]

Summary

Prevent hack/run-with-prow.sh from reporting a CI job as failed when the
post-test artifact-compression step hits a non-fatal tar error caused by
lingering test processes still flushing log/audit files.

What Type of PR Is This?

/kind bug

Related Issue(s)

Fixes #

Release Notes

NONE

Fixes #4343

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign clubanderson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kcp-ci-bot]

Hi @pujitha24. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[gman0]

Why can't we just wait for all processes to exit?

[pujitha24]

Good question, so I actually went and checked. The command run-with-prow.sh runs directly ("${@}", plain go test) already waits properly for its own kcp servers: sdk/testing/server/fixture.go sends SIGTERM and blocks on cmd.Wait() before the test binary returns, so those are fully reaped by the time this script gets to the tar step.

The race comes from the shared/sharded flows (test-e2e-shared-minimal etc.). There, kcp/cache-server/front-proxy are children of cmd/test-server/main.go and cmd/sharded-test-server/main.go, and those start() functions cancel the child processes and return right after GatherMetrics, without ever waiting on the terminatedCh/error channels the shard already exposes. So the wrapper binary (and the Makefile's wait $PID) can exit before the kcp process it just killed has actually finished dying and flushing audit.log — this script has no handle on those grandchild processes to wait on itself.

So "wait for all processes to exit" is the right fix, but it belongs in those two main()s, not here. That's a real change to shutdown ordering across two binaries and I don't want to guess at timeouts/ordering without being able to validate it against real CI. Want me to take a shot at it as a follow-up, or would you rather fold it into this PR?

[mjudeikis]

I would like to see how this could looks like in the main go code. How big is the change

[pujitha24]

Looked into this concretely. For cmd/test-server/main.go it's small — start() already has errCh from WaitForReady, it just never blocks on it after GatherMetrics:

s.GatherMetrics(metricsCtx)

cancel()   // stop the shard
<-errCh    // wait for it to actually exit before we return
return nil

That's ~4 lines.

cmd/sharded-test-server/main.go is bigger. Shards, the cache server, and virtual workspaces already expose a terminatedCh (currently only consumed to detect premature failure, not reused at shutdown), so those just need to be waited on again after shutdownCtx.Done(). But startFrontProxy in frontproxy.go creates its terminatedCh locally and only uses it internally for the readiness-wait loop — it never returns it. To wait for front-proxy on shutdown too, startFrontProxy's signature has to change to return that channel, plus the call site in main.go. So realistically this touches main.go, frontproxy.go, and the shutdown select block — rough guess ~30-50 lines across those files, plus a real CI run to make sure it doesn't introduce a hang if a process doesn't exit cleanly.

I haven't written that patch yet since it's a shutdown-ordering change I want to validate against real CI runs rather than guess at, so I'd rather keep it out of this PR (which is just the tar-warning wrapper) and open it separately. Happy to go do that now if that's preferred over merging this as-is.

[mjudeikis]

So can we do this better? :) over trying to hack bash?

[pujitha24]

Yeah, fair — the tar wrapper is a band-aid, not a fix. I'd rather land the real thing than keep this hack around.

Of the two pieces I scoped out above, the cmd/test-server/main.go side is small and I'm confident in it (call cancel() explicitly, then block on the errCh from WaitForReady before returning, instead of letting the deferred cancel() race with process exit). The cmd/sharded-test-server side is bigger — it needs startFrontProxy to return its terminatedCh too — and I don't want to merge shutdown-ordering changes there without a real CI run backing it up.

Want me to swap this PR for the test-server/main.go fix now and follow up with the sharded-test-server side once I can validate it, or would you rather I close this and open the real fix as a fresh PR?