Fix: run same-ID jobs one at a time, in order, without dropping any

BR
1mo ago
+364-84

Description of your changes

Problem

A trigger that watches more than one event type (events: [create, update, delete]) looks like it only acts on one of them.

The events themselves all arrive. Every action job from one trigger shares a job ID (the hash of its action spec), and action jobs disallow concurrency. But the executor checked "is this ID running?" and then "mark it running" as two separate steps, so parallel workers all got past the check and the jobs ran at the same time. bump-application-revision reads publishVersion and patches it to N+1, so overlapping runs overwrite each other and only one bump lands.

Closing that race on its own isn't enough. A job that found its ID busy was requeued through the failure rate limiter, so it:

  • waited on exponential backoff (2s, 4s, 8s... at the default --retry-delay), even with --action-retry off,
  • used up its retry budget while waiting,
  • was dropped for good after --max-retry attempts.

Fix

Each ID gets a waiting line, formed in AddJob:

BeforeAfter
Job added while its ID is heldQueued, then requeued with failure backoff when picked upJoins the back of that ID's line
Job finishesClears the running markerQueues the next job in its ID's line
Same-ID jobs overlapYes, the check was racyNever
OrderArbitraryOrder added
Dropped for waitingAfter --max-retryNever
Failed job retryingLater events could overtake itKeeps its ID, so the line waits for it
Jobs that allow concurrencySet and cleared the shared markerBypass the lines

The queue size limit counts waiting jobs as well.

On shutdown, a job cancelled mid-run releases its ID, and once workers stop, the executor logs how many queued and waiting jobs never started, then resets the lines.

I have:

  • Read and followed KubeVela's pull request process.
  • All commits are signed off (DCO).
  • Related Docs updated properly. Not needed: no user-facing configuration changes.
  • Run make reviewable to ensure this PR is ready for review. (Ran make lint, make checklicense and go mod tidy; not generate, as the local controller-gen differs from the pinned v0.9.0.)
  • Added backport release-x.y labels to auto-backport this PR if necessary. Not needed: kube-trigger has no release branches.

How has this code been tested

Each new test fails on main and passes with this change (-race -count=10):

  • TestSameJobNeverRunsConcurrently: three read-modify-write jobs sharing an ID must each apply.
  • TestSameJobBurstRunsEveryJobInOrder: six same-ID jobs with MaxJobRetries: 0 all run, in order, never overlapping.
  • TestConcurrentJobDoesNotReleaseSharedID: a concurrency-allowed job sharing an ID doesn't free another job's claim.
  • TestRetryKeepsItsPlaceInLine: a job that fails twice still runs before the same-ID job added after it.
  • TestShutdownReleasesAndClearsLines: a job cancelled by shutdown releases its ID, and nothing is left waiting.

TestSameJobRequeuing is removed: it asserted contention retries, which no longer exist, and it discarded the error from its only check.

Not verified against a live cluster; this repo has no e2e suite yet.

Special notes for your reviewer

  • Nothing special: a self-contained executor change.

Summary by cubic

Fixes the executor so triggers watching multiple event types no longer run their jobs concurrently, reorder them, or drop them for waiting on a busy ID.

  • Replaces the racy is-running check plus mark with a waiting line per ID formed in AddJob: the first job holds the ID, later same-ID jobs queue behind it, and releasing the ID starts the next in line.
  • A failed job keeps its ID through retries so later events can't overtake it; jobs that allow concurrency bypass the lines, and the queue size limit counts waiting jobs.
  • Shutdown now releases the ID of a cancelled running job and logs and clears the jobs that never started, queued or waiting. The counts are approximate after a shutdown timeout, and the workqueue is not drained since it shuts down with the process.

Removes TestSameJobRequeuing, which asserted contention retries that no longer exist, and adds tests for the new behavior, including one asserting cancelled jobs release their ID.