From 632260e2ae98c4892f1c88707568344360f8aabf Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 05:53:52 +0000 Subject: [PATCH 1/7] test(e2e): time the bi-directional loop, and print it as numbers The bi-directional specs prove what the loop does and only bound how long it may take. bi_directional_timing_e2e_test.go (label timing, in the corner) times it on a shared folder, for Flux and Argo CD: API edit to commit, commit to apply after the quiet period, apply to held again, and a Git-side push through the receiver to the live object. Each sample goes into a gmeasure experiment, printed as an N/min/median/max table and kept in the JSON report; each has a loose budget that fails the spec when a path falls back to the steady tick. The Flux API-edit round trip is asserted to take at least the quiet period. ginkgo-allure now attaches a spec's report entries as "Measurements", so the tables show up in task allure-e2e-report too. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/spec/e2e-bi-directional-corner.md | 21 ++ test/e2e/bi_directional_timing_e2e_test.go | 406 +++++++++++++++++++++ test/e2e/tools/ginkgo-allure/main.go | 52 ++- test/e2e/tools/ginkgo-allure/main_test.go | 55 ++- 4 files changed, 527 insertions(+), 7 deletions(-) create mode 100644 test/e2e/bi_directional_timing_e2e_test.go diff --git a/docs/spec/e2e-bi-directional-corner.md b/docs/spec/e2e-bi-directional-corner.md index ddb801bc7..3843f6db0 100644 --- a/docs/spec/e2e-bi-directional-corner.md +++ b/docs/spec/e2e-bi-directional-corner.md @@ -178,6 +178,25 @@ dedicated CI leg: | Task | `task test-e2e-bi-directional` | | CI leg | `bi-directional` | +### The timing spec: `bi_directional_timing_e2e_test.go` + +The other specs prove what the loop does and only bound how long it may take. +[`bi_directional_timing_e2e_test.go`](../../test/e2e/bi_directional_timing_e2e_test.go) +(label `timing`) measures it. It mirrors a labeled ConfigMap into the folder a Flux +Kustomization or an Argo CD Application applies, then times each sample from the stimulus +(an API edit, or a Git-side push landing on Gitea) to the effect an operator would look for: + +- the commit on the branch; +- Flux applying it after the default 30s quiet period; +- the trigger holding its applier again; +- the live object changing after a Git-side push through the receiver. + +Each duration goes into a `gmeasure` experiment, which Ginkgo prints as an N/min/median/max +table and writes into the JSON report. `task allure-e2e-report` attaches it to the spec as +"Measurements". Every sample also has a budget, and a sample over it fails the spec. The +budgets catch a path that fell back to the 5-minute steady tick, not a slow runner. +`E2E_TIMING_SAMPLES` sets the sample count (default 3). + ### The third spec: `provenance_markers_e2e_test.go` The corner hosts a third spec, and it is the reason the corner is worth more than the @@ -605,4 +624,6 @@ namespaces and resource names, and none is `Serial`. - [`test/e2e/flux_bi_directional_e2e_test.go`](../../test/e2e/flux_bi_directional_e2e_test.go) and [`test/e2e/argocd_bi_directional_e2e_test.go`](../../test/e2e/argocd_bi_directional_e2e_test.go): the two specs in this corner +- [`test/e2e/bi_directional_timing_e2e_test.go`](../../test/e2e/bi_directional_timing_e2e_test.go): + the loop's latency, as numbers - [`internal/sanitize/types.go`](../../internal/sanitize/types.go): the strip lists diff --git a/test/e2e/bi_directional_timing_e2e_test.go b/test/e2e/bi_directional_timing_e2e_test.go new file mode 100644 index 000000000..42c91fa82 --- /dev/null +++ b/test/e2e/bi_directional_timing_e2e_test.go @@ -0,0 +1,406 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "os" + "regexp" + "strconv" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/onsi/gomega/gmeasure" +) + +// How long the bi-directional loop takes, as numbers. +// +// The other bi-directional specs prove WHAT happens and only bound how long it may take. These +// time it: each sample starts at the stimulus (an API edit, or a Git-side push landing on the +// server) and stops at the effect an operator would look for, and every duration is recorded in +// a gmeasure experiment that Ginkgo prints as a table (N, min, median, mean, stddev, max) and +// writes into the JSON report. +// +// Each duration also has a budget, and a sample over it fails the spec. The budgets are loose on +// purpose: they catch a path that silently fell back to a periodic tick (minutes), not a slow CI +// runner (seconds). The table is the measurement; the budget is the guarantee. +// +// The folder is SHARED, which is the case a trigger exists for: GitOps Reverser mirrors a +// labeled ConfigMap into the folder the applier applies. The WatchRule selects only that label, +// so the namespace's kube-root-ca.crt never reaches the folder and the applier applies exactly one +// object. +// +// Resolution: the probes poll every timingPoll and each probe is a kubectl or git round trip, so +// a sample reads about a second long at worst. E2E_TIMING_SAMPLES sets the sample count. + +const ( + timingPoll = 500 * time.Millisecond + + // An API edit is on the remote branch. The e2e commit window is 0s, so this is the watch + // event, the commit and the push. + timingCommitBudget = 45 * time.Second + // A Git-side push the receiver hears of has reached the live object, through the applier. + timingReceiverBudget = 90 * time.Second + // An apply the trigger started has finished, and the trigger holds its applier again. + timingHeldBudget = 60 * time.Second + + timingDefaultSamples = 3 + timingLabelKey = "e2e.configbutler.ai/timing" + timingConfigMap = "bi-timing" +) + +// Measurement names, shared by both appliers so the two tables read side by side. +const ( + measureAPIToCommit = "API edit → commit on the branch" + measureCommitToApplied = "commit on the branch → Flux applied it" + measureAPIToApplied = "API edit → Flux applied it (includes the quiet period)" + measureAppliedToHeld = "applied → trigger holds the applier again" + measurePushToLive = "Git-side push → live object (receiver)" + measurePushToSyncDone = "Git-side push → Argo CD sync finished (receiver)" + measurePushToFluxHeld = "Git-side push → Flux applied it and is held again (receiver)" + timingSampleValueFormat = "%s-%d" +) + +// timingSamples is how many samples each measurement takes. +func timingSamples() int { + if raw := strings.TrimSpace(os.Getenv("E2E_TIMING_SAMPLES")); raw != "" { + n, err := strconv.Atoi(raw) + if err == nil && n > 0 { + return n + } + } + return timingDefaultSamples +} + +// timedUntil polls probe until it passes, at most budget after start, records the time from start +// to the first passing probe under name, and returns that duration. +func timedUntil( + experiment *gmeasure.Experiment, name string, start time.Time, budget time.Duration, probe func(g Gomega), +) time.Duration { + GinkgoHelper() + remaining := budget - time.Since(start) + Expect(remaining).To(BeNumerically(">", 0), "%s: the budget of %s was spent before the probe began", name, budget) + Eventually(probe, remaining, timingPoll).Should(Succeed(), "%s: over its budget of %s", name, budget) + elapsed := time.Since(start) + experiment.RecordDuration(name, elapsed, gmeasure.Precision(100*time.Millisecond)) + return elapsed +} + +// timingPipeline is one GitTarget mirroring the labeled ConfigMap into the folder an applier +// applies. +type timingPipeline struct { + testNs string + repo *RepoArtifacts + providerName string + targetName string + ruleName string + gitPath string + receiverName string + hookID int64 +} + +func newTimingPipeline(testNs, prefix string, repo *RepoArtifacts) *timingPipeline { + return &timingPipeline{ + testNs: testNs, + repo: repo, + providerName: prefix + "-provider", + targetName: prefix + "-target", + ruleName: prefix + "-rule", + gitPath: "timing/" + prefix, + receiverName: prefix + "-receiver", + } +} + +// filePath is where the GitTarget writes the ConfigMap, relative to the repository root. +func (p *timingPipeline) filePath() string { + return fmt.Sprintf("%s/%s/configmaps/%s.yaml", p.gitPath, p.testNs, timingConfigMap) +} + +// start brings up GitProvider (with a receiver Gitea pushes to), GitTarget and WatchRule, creates +// the ConfigMap, and waits for its first commit, so the folder exists before an applier names it. +func (p *timingPipeline) start() { + GinkgoHelper() + _, err := kubectlRunInNamespace(p.testNs, "apply", "-f", p.repo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to the test namespace") + applySOPSAgeKeyToNamespace(p.testNs) + configureRepoOriginWithCredentials(p.repo, p.testNs) + + createGitProviderWithURLInNamespace(p.providerName, p.testNs, p.repo.GitSecretHTTP, p.repo.RepoURLHTTP) + verifyResourceStatus("gitprovider", p.providerName, p.testNs, "True", "Succeeded", "") + p.hookID = registerOperatorReceiver(p.testNs, p.providerName, p.receiverName, p.repo.RepoName) + + createGitTarget(p.targetName, p.testNs, p.providerName, p.gitPath, "main") + _, err = kubectlRunWithStdin(p.testNs, fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: %s + namespace: %s +spec: + gitTargetRef: + name: %s + rules: + - resources: ["configmaps"] + objectSelector: + matchLabels: + %s: "true" +`, p.ruleName, p.testNs, p.targetName, timingLabelKey), "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "failed to apply the WatchRule") + verifyResourceStatus("gittarget", p.targetName, p.testNs, "True", "Succeeded", "") + waitForStreamsRunning(p.targetName, p.testNs) + + _, err = kubectlRunWithStdin(p.testNs, fmt.Sprintf(`apiVersion: v1 +kind: ConfigMap +metadata: + name: %s + namespace: %s + labels: + %s: "true" +data: + value: seed +`, timingConfigMap, p.testNs, timingLabelKey), "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "failed to create the timed ConfigMap") + Eventually(func(g Gomega) { + g.Expect(p.committedValue(g)).To(Equal("seed")) + }, timingCommitBudget, timingPoll).Should(Succeed(), "the ConfigMap must be in Git before an applier applies it") +} + +func (p *timingPipeline) cleanup() { + deleteRepoHook(p.repo.RepoName, p.hookID) + cleanupWatchRule(p.ruleName, p.testNs) + cleanupGitTarget(p.targetName, p.testNs) + cleanupNamespacedResource(p.testNs, "gitprovider", p.providerName) + cleanupNamespacedResource(p.testNs, "secret", p.receiverName) +} + +var timingValueLine = regexp.MustCompile(`(?m)^([ \t]+value:[ \t]*).*$`) + +// committedFile is the ConfigMap's file on the remote branch. +func (p *timingPipeline) committedFile(g Gomega) string { + _, err := gitRun(p.repo.CheckoutDir, "fetch", "origin", "main") + g.Expect(err).NotTo(HaveOccurred()) + out, err := gitRun(p.repo.CheckoutDir, "show", "origin/main:"+p.filePath()) + g.Expect(err).NotTo(HaveOccurred(), "the ConfigMap is not at %s on main", p.filePath()) + return out +} + +// committedValue is data.value of the ConfigMap as the remote branch holds it. +func (p *timingPipeline) committedValue(g Gomega) string { + match := timingValueLine.FindStringSubmatch(p.committedFile(g)) + g.Expect(match).NotTo(BeNil(), "the committed ConfigMap has no data.value") + return strings.TrimSpace(strings.TrimPrefix(match[0], match[1])) +} + +// liveValue is data.value of the live ConfigMap. +func (p *timingPipeline) liveValue(g Gomega) string { + out, err := kubectlRunInNamespace(p.testNs, "get", "configmap", timingConfigMap, "-o", "jsonpath={.data.value}") + g.Expect(err).NotTo(HaveOccurred()) + return strings.TrimSpace(out) +} + +// editThroughAPI sets data.value through the Kubernetes API and returns when the request returned. +func (p *timingPipeline) editThroughAPI(value string) time.Time { + GinkgoHelper() + start := time.Now() + _, err := kubectlRunInNamespace(p.testNs, "patch", "configmap", timingConfigMap, "--type=merge", "-p", + fmt.Sprintf(`{"data":{"value":%q}}`, value)) + Expect(err).NotTo(HaveOccurred(), "failed to edit the ConfigMap through the API") + return start +} + +// pushFromGitSide rewrites data.value in the committed file, byte for byte otherwise, so the +// applied object mirrors back as no change, and pushes it as another writer would. It returns the +// commit and the moment the push returned, which is when the commit is on the server. +func (p *timingPipeline) pushFromGitSide(value string) (string, time.Time) { + GinkgoHelper() + content := timingValueLine.ReplaceAllString(p.committedFile(Default), "${1}"+value) + head := pushFileFromOutside(p.repo, p.filePath(), content, "e2e: a Git-side timing change to "+value) + return head, time.Now() +} + +// remoteBranchHead is where main is on the server. +func (p *timingPipeline) remoteBranchHead(g Gomega) string { + _, err := gitRun(p.repo.CheckoutDir, "fetch", "origin", "main") + g.Expect(err).NotTo(HaveOccurred()) + out, err := gitRun(p.repo.CheckoutDir, "rev-parse", "origin/main") + g.Expect(err).NotTo(HaveOccurred()) + return strings.TrimSpace(out) +} + +var _ = Describe("Bi Directional timing (Flux)", Label("bi-directional", "flux", "timing"), Ordered, + SpecPriority(10), func() { + var ( + testNs, fluxName string + pipeline *timingPipeline + ) + + BeforeAll(func() { + skipUnlessBiDirectionalEnabled() + allowCrossNamespaceAppliers() + testNs = testNamespaceFor("bi-timing-flux") + _, _ = kubectlRun("create", "namespace", testNs) + fluxName = "bi-timing-" + testNs + + repo := SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-bi-timing-flux-%d", GinkgoRandomSeed())) + pipeline = newTimingPipeline(testNs, "flux-timing", repo) + pipeline.start() + + By("letting Flux apply the shared folder once, then naming a trigger with the default quiet period") + applyFluxApplier(testNs, repo, fluxName, fluxName+"-git", "main", "./"+pipeline.gitPath) + createFluxTrigger(testNs, fluxName, fluxName, "") + nameApplyTrigger(testNs, pipeline.targetName, "FluxTrigger", fluxName, "") + verifyResourceCondition("gittarget", pipeline.targetName, testNs, + "ApplyTriggerReady", "True", "Succeeded", "") + verifyResourceCondition(fluxTriggerResource, fluxName, testNs, "Held", "True", "Suspended", "") + }) + + AfterAll(func() { + if !biDirectionalEnabled() { + return + } + pipeline.cleanup() + cleanupNamespacedResource(testNs, fluxTriggerResource, fluxName) + cleanupNamespacedResource("flux-system", "kustomization", fluxName) + cleanupNamespacedResource("flux-system", "gitrepository", fluxName) + cleanupNamespacedResource("flux-system", "secret", fluxName+"-git") + cleanupNamespace(testNs) + }) + + // heldAgain passes once the trigger reports the operation for commit Succeeded, with the + // Kustomization suspended again. + heldAgain := func(g Gomega, commit string) { + operation := readTriggerOperation(g, testNs, fluxTriggerResource, fluxName) + g.Expect(operation.Commit).To(Equal(commit)) + g.Expect(operation.Phase).To(Equal("Succeeded")) + g.Expect(kustomizationSuspended(g, fluxName)).To(BeTrue()) + } + + // The API side: an edit is mirrored, the branch goes quiet, and the trigger asks Flux to catch + // up. The quiet period is the default, because that is the latency a user gets; it is part of + // the total by design, so the spec also proves it is honored: no apply lands before it ends. + It("times an API edit through Git to a Flux apply", func() { + experiment := gmeasure.NewExperiment("Flux: API edit round trip") + AddReportEntry(experiment.Name, experiment) + quiet := 30 * time.Second // DefaultApplierQuietPeriod; the trigger leaves spec.quietPeriod empty. + experiment.RecordNote(fmt.Sprintf("quietPeriod %s (the default), commit window 0s", quiet)) + + for i := range timingSamples() { + value := fmt.Sprintf(timingSampleValueFormat, "api", i) + By("sample " + strconv.Itoa(i) + ": editing the ConfigMap through the API") + start := pipeline.editThroughAPI(value) + + var head string + timedUntil(experiment, measureAPIToCommit, start, timingCommitBudget, func(g Gomega) { + g.Expect(pipeline.committedValue(g)).To(Equal(value)) + head = pipeline.remoteBranchHead(g) + }) + committedAt := time.Now() + + applied := timedUntil(experiment, measureAPIToApplied, start, timingCommitBudget+quiet+timingHeldBudget, + func(g Gomega) { + g.Expect(fluxLastAppliedRevision(g, fluxName)).To(ContainSubstring(head)) + }) + experiment.RecordDuration(measureCommitToApplied, time.Since(committedAt), + gmeasure.Precision(100*time.Millisecond)) + Expect(applied).To(BeNumerically(">=", quiet), + "Flux applied %s after the edit, before the branch had been quiet for %s", applied, quiet) + appliedAt := time.Now() + + timedUntil(experiment, measureAppliedToHeld, appliedAt, timingHeldBudget, func(g Gomega) { + heldAgain(g, head) + }) + Expect(pipeline.liveValue(Default)).To(Equal(value), "the catch-up must not revert the edit it applies") + } + }) + + // The Git side: another writer pushes, Gitea tells the receiver, and the trigger asks Flux to + // apply that commit. A ForeignCommit waits for no quiet period. + It("times a Git-side push through the receiver to the live object", func() { + experiment := gmeasure.NewExperiment("Flux: Git-side push") + AddReportEntry(experiment.Name, experiment) + + for i := range timingSamples() { + value := fmt.Sprintf(timingSampleValueFormat, "git", i) + By("sample " + strconv.Itoa(i) + ": pushing a Git-side change") + head, pushed := pipeline.pushFromGitSide(value) + + timedUntil(experiment, measurePushToLive, pushed, timingReceiverBudget, func(g Gomega) { + g.Expect(pipeline.liveValue(g)).To(Equal(value)) + }) + timedUntil(experiment, measurePushToFluxHeld, pushed, timingReceiverBudget+timingHeldBudget, + func(g Gomega) { + g.Expect(fluxLastAppliedRevision(g, fluxName)).To(ContainSubstring(head)) + heldAgain(g, head) + }) + } + }) + }) + +var _ = Describe("Bi Directional timing (Argo CD)", Label("bi-directional", "argocd", "timing"), Ordered, + SpecPriority(10), func() { + var ( + testNs string + pipeline *timingPipeline + run argoBiDirectionalRun + ) + + BeforeAll(func() { + skipUnlessBiDirectionalEnabled() + requireArgoCDInstalled() + allowCrossNamespaceAppliers() + testNs = testNamespaceFor("bi-timing-argo") + _, _ = kubectlRun("create", "namespace", testNs) + + repo := SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-bi-timing-argo-%d", GinkgoRandomSeed())) + pipeline = newTimingPipeline(testNs, "argo-timing", repo) + run = newArgoBiDirectionalRun(testNs, repo, "argo-timing") + pipeline.start() + + By("registering the repository with Argo CD and naming the Application, automated sync off") + run.applyArgoRepoSecret() + cfg := run.baseAppConfig() + cfg.Path = pipeline.gitPath + run.applyArgoApp(cfg) + createArgoCDTrigger(testNs, run.appName, run.argoNs, run.appName) + nameApplyTrigger(testNs, pipeline.targetName, "ArgoCDTrigger", run.appName, "") + verifyResourceCondition("gittarget", pipeline.targetName, testNs, + "ApplyTriggerReady", "True", "Succeeded", "") + }) + + AfterAll(func() { + if !biDirectionalEnabled() { + return + } + cleanupNamespacedResource(run.argoNs, "application.argoproj.io", run.appName) + cleanupNamespacedResource(run.argoNs, "secret", run.repoSecretName) + cleanupNamespacedResource(testNs, argoCDTriggerResource, run.appName) + pipeline.cleanup() + cleanupNamespace(testNs) + }) + + // Argo CD is never asked to catch up with GitOps Reverser's own commits, so the API side has + // no apply to time: its latency is the commit, which the Flux table already measures. The Git + // side is a ForeignCommit sync pinned to the pushed commit. + It("times a Git-side push through the receiver to the live object", func() { + experiment := gmeasure.NewExperiment("Argo CD: Git-side push") + AddReportEntry(experiment.Name, experiment) + + for i := range timingSamples() { + value := fmt.Sprintf(timingSampleValueFormat, "git", i) + By("sample " + strconv.Itoa(i) + ": pushing a Git-side change") + head, pushed := pipeline.pushFromGitSide(value) + + timedUntil(experiment, measurePushToLive, pushed, timingReceiverBudget, func(g Gomega) { + g.Expect(pipeline.liveValue(g)).To(Equal(value)) + }) + timedUntil(experiment, measurePushToSyncDone, pushed, timingReceiverBudget+timingHeldBudget, + func(g Gomega) { + operation := run.triggerOperation(g) + g.Expect(operation.Reason).To(Equal("ForeignCommit")) + g.Expect(operation.Commit).To(Equal(head)) + g.Expect(operation.Phase).To(Equal("Succeeded")) + }) + } + }) + }) diff --git a/test/e2e/tools/ginkgo-allure/main.go b/test/e2e/tools/ginkgo-allure/main.go index e76f11a72..139a9ae9a 100644 --- a/test/e2e/tools/ginkgo-allure/main.go +++ b/test/e2e/tools/ginkgo-allure/main.go @@ -12,6 +12,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strconv" "strings" "time" @@ -116,7 +117,10 @@ func convertReports(outputDir, path string, reports []types.Report) (int, error) continue } result := convertSpec(path, reportIndex, specIndex, report, spec) - if err := writeResult(outputDir, result, spec.CombinedOutput()); err != nil { + if err := writeResult(outputDir, result, + textAttachment{name: "Ginkgo output", suffix: "attachment", body: spec.CombinedOutput()}, + textAttachment{name: "Measurements", suffix: "measurements", body: measurements(spec)}, + ); err != nil { return count, err } count++ @@ -169,14 +173,50 @@ func convertSpec(path string, reportIndex, specIndex int, report types.Report, s return result } -func writeResult(outputDir string, result allureResult, combinedOutput string) error { - if strings.TrimSpace(combinedOutput) != "" { - source := result.UUID + "-attachment.txt" - if err := os.WriteFile(filepath.Join(outputDir, source), []byte(combinedOutput), 0o600); err != nil { +// textAttachment is a plain-text file attached to an Allure result; an empty body attaches nothing. +type textAttachment struct { + name string + suffix string + body string +} + +// ginkgoStyle matches the {{bold}}-style console markup Ginkgo keeps in a report entry's text. +var ginkgoStyle = regexp.MustCompile(`\{\{[^}]*\}\}`) + +// measurements renders a spec's report entries (AddReportEntry, such as a gmeasure experiment's +// table) as plain text, so the numbers a spec records sit beside its result in Allure. +func measurements(spec types.SpecReport) string { + var out strings.Builder + for _, entry := range spec.ReportEntries { + // A report read back from JSON keeps the text Ginkgo rendered when it wrote the file in + // Representation; StringRepresentation is the in-process fallback. + text := entry.Value.Representation + if text == "" { + text = entry.StringRepresentation() + } + text = strings.TrimSpace(ginkgoStyle.ReplaceAllString(text, "")) + if text == "" { + continue + } + if !strings.HasPrefix(text, entry.Name) { + out.WriteString(entry.Name + "\n") + } + out.WriteString(text + "\n\n") + } + return out.String() +} + +func writeResult(outputDir string, result allureResult, attachments ...textAttachment) error { + for _, attachment := range attachments { + if strings.TrimSpace(attachment.body) == "" { + continue + } + source := result.UUID + "-" + attachment.suffix + ".txt" + if err := os.WriteFile(filepath.Join(outputDir, source), []byte(attachment.body), 0o600); err != nil { return fmt.Errorf("write attachment %s: %w", source, err) } result.Attachments = append(result.Attachments, allureAttachment{ - Name: "Ginkgo output", + Name: attachment.name, Source: source, Type: "text/plain", }) diff --git a/test/e2e/tools/ginkgo-allure/main_test.go b/test/e2e/tools/ginkgo-allure/main_test.go index 49600f475..80ca791ae 100644 --- a/test/e2e/tools/ginkgo-allure/main_test.go +++ b/test/e2e/tools/ginkgo-allure/main_test.go @@ -320,7 +320,8 @@ func TestWriteResult_NoAttachment(t *testing.T) { Status: "passed", Stage: "finished", } - if err := writeResult(outputDir, result, " \n\t"); err != nil { + blank := textAttachment{name: "Ginkgo output", suffix: "attachment", body: " \n\t"} + if err := writeResult(outputDir, result, blank); err != nil { t.Fatalf("writeResult: %v", err) } @@ -333,6 +334,58 @@ func TestWriteResult_NoAttachment(t *testing.T) { } } +// styledTable stands in for a gmeasure experiment: a value Ginkgo renders through ColorableString. +type styledTable struct{} + +func (styledTable) ColorableString() string { + return "{{bold}}Flux: Git-side push\n{{/}}push → live | 3 | 2.2s | 2.7s" +} + +func (styledTable) String() string { return "unused" } + +func TestRun_AttachesReportEntriesAsMeasurements(t *testing.T) { + reports := fixtureReport() + spec := &reports[0].SpecReports[1] + spec.ReportEntries = []types.ReportEntry{ + {Name: "Flux: Git-side push", Value: types.WrapEntryValue(styledTable{})}, + {Name: "empty", Value: types.WrapEntryValue(nil)}, + {Name: "gitea-commit-verification", Value: types.WrapEntryValue("verified: true")}, + } + reportPath := writeFixtureReport(t, reports) + outputDir := filepath.Join(t.TempDir(), "allure-results") + if _, err := run(outputDir, []string{reportPath}); err != nil { + t.Fatalf("run: %v", err) + } + + result := findResult(t, readAllureResults(t, outputDir), "passes") + var source string + for _, attachment := range result.Attachments { + if attachment.Name == "Measurements" { + source = attachment.Source + } + } + if source == "" { + t.Fatalf("attachments: got %#v, want one named Measurements", result.Attachments) + } + data, err := os.ReadFile(filepath.Join(outputDir, source)) + if err != nil { + t.Fatalf("read measurements: %v", err) + } + want := "Flux: Git-side push\npush → live | 3 | 2.2s | 2.7s\n\n" + + "gitea-commit-verification\nverified: true\n\n" + if string(data) != want { + t.Errorf("measurements:\n got %q\nwant %q", string(data), want) + } + + for _, other := range readAllureResults(t, outputDir) { + for _, attachment := range other.Attachments { + if other.Name != "passes" && attachment.Name == "Measurements" { + t.Errorf("%s: a spec without report entries got a Measurements attachment", other.Name) + } + } + } +} + func TestStatusMappings(t *testing.T) { cases := map[types.SpecState]string{ types.SpecStatePassed: "passed", From f974c923773610514b8174787f8570f849e41f33 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 05:54:00 +0000 Subject: [PATCH 2/7] test(e2e): start the longest manager specs first, and stop waiting on namespace deletion The manager leg's wall clock is set by Remote Refresh (about 674 s, two steady ticks). Where it happened to start moved the suite between 995 s and 1369 s in recent CI runs. SpecPriority on the four longest manager containers starts them at second 0 of the parallel phase. cleanupNamespace no longer waits for the namespace to finalize: 5-10 s per AfterAll, 214 s of process time in the manager leg. Every test namespace is named for the run's seed, so nothing reads one again. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/e2e/flux_applier_e2e_test.go | 2 +- test/e2e/helpers.go | 4 +++- test/e2e/object_selector_e2e_test.go | 2 +- test/e2e/refresh_remote_e2e_test.go | 2 +- test/e2e/refusal_reapply_e2e_test.go | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/test/e2e/flux_applier_e2e_test.go b/test/e2e/flux_applier_e2e_test.go index ec695fb65..4b0b6b480 100644 --- a/test/e2e/flux_applier_e2e_test.go +++ b/test/e2e/flux_applier_e2e_test.go @@ -23,7 +23,7 @@ import ( // spec's API edit or this spec's Git-side push. Once the trigger holds it, the Kustomization is // suspended, and no Flux Receiver is wired to the repository, so any apply inside the spec is one // GitOps Reverser started. -var _ = Describe("Manager Flux Applier", Label("manager", "flux-applier"), Ordered, func() { +var _ = Describe("Manager Flux Applier", Label("manager", "flux-applier"), Ordered, SpecPriority(90), func() { const ( providerName = "flux-applier-provider" destName = "flux-applier-dest" diff --git a/test/e2e/helpers.go b/test/e2e/helpers.go index 81d74f2a2..2aa474cdb 100644 --- a/test/e2e/helpers.go +++ b/test/e2e/helpers.go @@ -356,7 +356,9 @@ func cleanupNamespace(name string) { return } By(fmt.Sprintf("deleting test namespace '%s'", name)) - _, _ = kubectlRun("delete", "namespace", name, "--ignore-not-found=true") + // --wait=false: finalizing a namespace takes 5-10s and nothing reads it again, since every + // test namespace is named for this run's seed. Waiting held the spec's process for it. + _, _ = kubectlRun("delete", "namespace", name, "--ignore-not-found=true", "--wait=false") } func cleanupNamespacedResource(namespace, resource, name string) { diff --git a/test/e2e/object_selector_e2e_test.go b/test/e2e/object_selector_e2e_test.go index 851c454bb..573e64407 100644 --- a/test/e2e/object_selector_e2e_test.go +++ b/test/e2e/object_selector_e2e_test.go @@ -22,7 +22,7 @@ import ( // Two targets watch the same ConfigMaps with the same selector: one Always, one on the default // (OnEvent). Each retention claim is made only after the Always target has visibly acted on the // same snapshot, so "still present" means "decided to keep", not "not processed yet". -var _ = Describe("Manager WatchRule objectSelector", Label("manager"), Ordered, func() { +var _ = Describe("Manager WatchRule objectSelector", Label("manager"), Ordered, SpecPriority(50), func() { const ( providerName = "gitprovider-selector" alwaysTarget = "selector-always-target" diff --git a/test/e2e/refresh_remote_e2e_test.go b/test/e2e/refresh_remote_e2e_test.go index 2937845ef..b6aecb377 100644 --- a/test/e2e/refresh_remote_e2e_test.go +++ b/test/e2e/refresh_remote_e2e_test.go @@ -37,7 +37,7 @@ import ( // // No reconcile-request annotation is used anywhere in this spec: an operator asking by hand is // precisely the case this feature exists to remove. -var _ = Describe("Manager Remote Refresh", Label("manager", "refresh"), Ordered, func() { +var _ = Describe("Manager Remote Refresh", Label("manager", "refresh"), Ordered, SpecPriority(100), func() { var ( testNs string repo *RepoArtifacts diff --git a/test/e2e/refusal_reapply_e2e_test.go b/test/e2e/refusal_reapply_e2e_test.go index 65aae2dc2..5058a582b 100644 --- a/test/e2e/refusal_reapply_e2e_test.go +++ b/test/e2e/refusal_reapply_e2e_test.go @@ -32,7 +32,7 @@ import ( // Flux applies the overlay when its Kustomization is created, and the trigger then holds it // suspended, so a revert of the drifted env var inside this spec is an apply GitOps Reverser started, // and nothing else: a resume of the unchanged commit, which is all a Refusal needs. -var _ = Describe("Manager Refusal Reapply", Label("manager", "refusal-reapply"), Ordered, func() { +var _ = Describe("Manager Refusal Reapply", Label("manager", "refusal-reapply"), Ordered, SpecPriority(80), func() { var ( testNs string repo *RepoArtifacts From 747c14c46a7e4a936144a9c97a20961cdfce5a1f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 05:54:00 +0000 Subject: [PATCH 3/7] test(e2e): move the serial CRD lifecycle spec to the core leg, and assert its no-commit check A Serial container runs alone after every parallel spec, so its 214 s added straight to the manager leg, the longest one. It stays Serial (the registry records it was de-serialized once and re-flaked) and moves to full-core by label, which has minutes to spare. The serial registry now says where a Serial container belongs. Its "no new commit after a status-only patch" check printed HEAD before and after but never compared them, and read "before" from an unpulled checkout. It now holds the remote head with Consistently, replacing a 10 s sleep. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/spec/e2e-serial-registry.md | 9 ++++- test/e2e/crd_lifecycle_e2e_test.go | 64 +++++++++++++----------------- 2 files changed, 34 insertions(+), 39 deletions(-) diff --git a/docs/spec/e2e-serial-registry.md b/docs/spec/e2e-serial-registry.md index fdaa36e84..211524bd5 100644 --- a/docs/spec/e2e-serial-registry.md +++ b/docs/spec/e2e-serial-registry.md @@ -29,17 +29,22 @@ row below. ## Serial containers +A Serial container runs alone, after every parallel spec of its CI leg has finished, so its whole +runtime adds to that leg's wall clock. Place it on a leg with time to spare: `CRD Lifecycle` +carries the `crd-lifecycle` label rather than `manager` for exactly that reason, which puts it in +the `full-core` leg beside the restart specs instead of at the end of the longest leg. + | File | Container | Shared state it touches | Why name isolation can't fix it | |---|---|---|---| | [test/e2e/restart_reconcile_e2e_test.go](../../test/e2e/restart_reconcile_e2e_test.go) | `Restart Reconcile Safety` | Rollout-restarts the controller deployment. | The controller is a singleton; restarting it disrupts in-flight reconciles/commits for every other spec. | | [test/e2e/flux_restart_recovery_e2e_test.go](../../test/e2e/flux_restart_recovery_e2e_test.go) | `Flux Restart Recovery` | Rollout-restarts the controller deployment while a Flux apply runs, and labels a node so that apply's Deployment can schedule. | The controller is a singleton, as for `Restart Reconcile Safety`; a node label is cluster-wide. | | [test/e2e/image_refresh_test.go](../../test/e2e/image_refresh_test.go) | `image refresh dependency chain` | Changes the controller image / redeploys the controller. | Same singleton controller; an image swap perturbs all concurrent specs. | -| [test/e2e/crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go) | `Manager CRD Lifecycle` | Installs/deletes a cluster-scoped CRD and watches all `customresourcedefinitions` cluster-wide, asserting exact Git file presence/absence. | A concurrent CRD install/delete bumps the global discovery-catalog generation and re-resolves every GitTarget's watched-type tables, delaying this spec's reconcile (CRD file appears late / post-delete sweep lags — the 649↔673 flake). A cluster-wide catalog bump cannot be scoped by name. | +| [test/e2e/crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go) | `CRD Lifecycle` | Installs/deletes a cluster-scoped CRD and watches all `customresourcedefinitions` cluster-wide, asserting exact Git file presence/absence. | A concurrent CRD install/delete bumps the global discovery-catalog generation and re-resolves every GitTarget's watched-type tables, delaying this spec's reconcile (CRD file appears late / post-delete sweep lags — the 649↔673 flake). A cluster-wide catalog bump cannot be scoped by name. | | [test/e2e/tilt_playground_e2e_test.go](../../test/e2e/tilt_playground_e2e_test.go) | `playground` | Reusable manual-playground fixture: fixed (non-randomized) `tilt-playground` namespace and `playground` repo/provider names, preserved across runs. | A singleton by construction — its resource names are intentionally stable so `task playground-*` can re-attach, so per-run name isolation does not apply. | ### De-serialized -`Manager CRD Lifecycle` ([crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go)) +`CRD Lifecycle` ([crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go)) **was de-serialized and then re-serialized** — it is in the Serial table above, not here. Finding #2's fix ([manager.go:1191](../../internal/watch/manager.go#L1191)) — resnapshot a target only when its *resolved* plan hash changes — removed the worst diff --git a/test/e2e/crd_lifecycle_e2e_test.go b/test/e2e/crd_lifecycle_e2e_test.go index 6216c56bf..e418bf923 100644 --- a/test/e2e/crd_lifecycle_e2e_test.go +++ b/test/e2e/crd_lifecycle_e2e_test.go @@ -5,7 +5,6 @@ package e2e import ( "fmt" "os" - "os/exec" "path/filepath" "time" @@ -21,7 +20,12 @@ import ( // GitTarget's watched-type tables, delaying this spec's reconcile so the CRD file appeared late // (verifyFileExists) or its post-delete sweep lagged (verifyFileDeleted) — the 649↔673 flake. // Serial pins it to the isolated serial phase, where no other spec churns cluster-scoped CRD state. -var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, func() { +// +// Not labeled manager: a Serial spec runs alone after every parallel spec has finished, so its +// whole runtime (about 3.5 min) adds to its CI leg's wall clock. The manager leg is the longest, +// the core leg has minutes to spare, and restart_reconcile's serial phase there is no CRD churn +// running alongside it. +var _ = Describe("CRD Lifecycle", Label("crd-lifecycle"), Serial, Ordered, func() { var ( testNs string crdLifecycleRepo *RepoArtifacts @@ -334,6 +338,10 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun } Eventually(verifyCRDInstanceExists, 15*time.Second, time.Second).Should(Succeed()) + // Read where the branch is BEFORE the patch, from the remote: a status-only change must not + // move it. Read after the patch, a status commit that landed fast would be the baseline. + headBeforeStatus := remoteHead(Default, crdLifecycleRepo.CheckoutDir) + By("applying status update to the IceCreamOrder CR") statusPatch := `{"status":{"phase":"Preparing","cost":12.5,"message":"Queued for pickup"}}` statusOutput, statusErr := kubectlRunInNamespace( @@ -349,41 +357,23 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun Expect(statusErr).NotTo(HaveOccurred(), "Status subresource patch should succeed") By(fmt.Sprintf("status patched successfully: %s", statusOutput)) - By("getting current git commit hash") - gitRevCmd := exec.Command("git", "rev-parse", "HEAD") - gitRevCmd.Dir = crdLifecycleRepo.CheckoutDir - beforeStatusCommit, _ := gitRevCmd.Output() - - By("waiting to ensure no new commit is created from status update") - time.Sleep(10 * time.Second) - - By("verifying no new commit was created and status is not in Git") - verifyStatusNotCommitted := func(g Gomega) { - pullLatestRepoState(g, crdLifecycleRepo.CheckoutDir) - - // Check that commit hash hasn't changed - gitRevCmd := exec.Command("git", "rev-parse", "HEAD") - gitRevCmd.Dir = crdLifecycleRepo.CheckoutDir - afterStatusCommit, err := gitRevCmd.Output() - g.Expect(err).NotTo(HaveOccurred()) - - By(fmt.Sprintf("Commit before status: %s", string(beforeStatusCommit))) - By(fmt.Sprintf("Commit after status: %s", string(afterStatusCommit))) - - // Read the file again to ensure status is still not present - expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, - "e2e/icecream-test", - iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) - content, readErr := os.ReadFile(expectedFile) - g.Expect(readErr).NotTo(HaveOccurred()) - g.Expect(string(content)).NotTo(ContainSubstring("status:"), - "CRD instance file should still NOT contain status field after status update") - g.Expect(string(content)).NotTo(ContainSubstring("phase:"), - "CRD instance file should NOT contain status phase content") - g.Expect(string(content)).NotTo(ContainSubstring("Queued for pickup"), - "CRD instance file should NOT contain status content") - } - Eventually(verifyStatusNotCommitted).Should(Succeed()) + By("verifying no new commit is created and status is not in Git") + Consistently(func(g Gomega) { + g.Expect(remoteHead(g, crdLifecycleRepo.CheckoutDir)).To(Equal(headBeforeStatus), + "a status-only update must not produce a commit") + }, 10*time.Second, time.Second).Should(Succeed()) + pullLatestRepoState(Default, crdLifecycleRepo.CheckoutDir) + expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, + "e2e/icecream-test", + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) + content, readErr := os.ReadFile(expectedFile) + Expect(readErr).NotTo(HaveOccurred()) + Expect(string(content)).NotTo(ContainSubstring("status:"), + "CRD instance file should still NOT contain status field after status update") + Expect(string(content)).NotTo(ContainSubstring("phase:"), + "CRD instance file should NOT contain status phase content") + Expect(string(content)).NotTo(ContainSubstring("Queued for pickup"), + "CRD instance file should NOT contain status content") By("✅ Status update verified - no Git commit created and status not in file") From 4a64d4ae46f23baaa2c3a567c7f975d51a116947 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 06:37:49 +0000 Subject: [PATCH 4/7] fix(controller): publish a held status.remote answer when the floor lifts requeueForRemoteAnswer brings a target that asked the remote back after RemotePublicationInterval to publish the answer. That requeue was lost whenever the refresh found the branch moved: the worker re-enqueues the GitTarget while the asking reconcile runs, and controller-runtime's priority queue folds a delayed requeue into an item already waiting. The reconcile that then ran held a fresh answer, asked nothing, was stopped by the floor, and returned the steady cadence, so a foreign push reached status.remote a whole tick (5m) late: the "ten minutes after noticing it" the function exists to prevent. publishRemote now reports how long the floor holds news back, and whichever reconcile meets a held answer requeues for when the floor lifts. The e2e install's --git-refresh-interval goes from 30s to 90s, above the 1m floor as the 10m default is. Below it, a reconcile inside the floor refreshed an observation not yet published, so our own push was published as a Fetch (the Remote Refresh failure on main at 519b483e). With both, Remote Refresh publishes our push when the floor lifts (about 1m, was a tick) and the foreign push one requeue after the tick that finds it: 433s locally, from 654-674s in CI, and its first bound drops from 7m to 3m. Co-Authored-By: Claude Opus 5.5 (1M context) --- config/deployment.yaml | 7 ++- internal/controller/gittarget_controller.go | 56 ++++++++++++++------ internal/controller/gittarget_remote.go | 14 +++-- internal/controller/gittarget_remote_test.go | 49 +++++++++++++++-- test/e2e/flux_applier_e2e_test.go | 2 +- test/e2e/refresh_remote_e2e_test.go | 20 ++++--- 6 files changed, 113 insertions(+), 35 deletions(-) diff --git a/config/deployment.yaml b/config/deployment.yaml index 393d912c6..c17b48193 100644 --- a/config/deployment.yaml +++ b/config/deployment.yaml @@ -46,8 +46,11 @@ spec: - --author-attribution-grace=10s # A short refresh interval so the e2e suite can watch an idle GitTarget notice a branch # somebody moved from outside, without waiting out the 10m default. The effective - # granularity is still the 5m control-plane reconcile. - - --git-refresh-interval=30s + # granularity is still the 5m control-plane reconcile. Kept above the 1m status.remote + # publication floor, as the default is: below it, a reconcile inside the floor refreshes + # an observation that has not been published yet, and our own push is published as a + # fetch. + - --git-refresh-interval=90s # The push receiver, so the e2e suite can deliver a Git host's push webhook through the # gitops-reverser-receiver Service (receiver-service.yaml) instead of waiting for a refresh. - --receiver-bind-address=:9292 diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 9bdb80600..2ba1f5501 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -203,9 +203,10 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, providerErr } - // awaitingAnswer: this reconcile is about to ask the remote where the branch is, so the answer - // lands after the status write below. See requeueForRemoteAnswer. - awaitingAnswer := r.publishGitObservations(st, &target, providerNS, repoIdentityOf(gitProvider)) + // remoteWake: whether this reconcile is about to ask the remote where the branch is, so the + // answer lands after the status write below, and how long the floor holds back an answer it + // already has. See requeueForRemoteAnswer. + remoteWake := r.publishGitObservations(st, &target, providerNS, repoIdentityOf(gitProvider)) // Ahead of every gate, for the reason the layout stanza above is: the join series has to carry // the targets that are NOT working. git_pushes_total names a branch that stopped advancing and @@ -304,7 +305,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // requeueAfter shortens the cadence when the status write lost a race: the object then holds // the WINNER's older answer and nothing re-enqueues it, because the For() predicate filters // status-only updates by design. - requeue := st.requeueAfter(requeueForRemoteAnswer(gitTargetRequeue(rd), awaitingAnswer)) + requeue := st.requeueAfter(requeueForRemoteAnswer(gitTargetRequeue(rd), remoteWake)) // TEMPORARY at Info, while Failure A is open. The data plane can converge and this status not // follow it: a run has shown the render gate reaching True and both this GitTarget and every // WatchRule on it still publishing "Rechecking" two minutes later, with no dropped reconcile @@ -921,20 +922,31 @@ func (r *GitTargetReconciler) publishGitObservations( target *configbutleraiv1alpha3.GitTarget, providerNS string, repo git.RepoIdentity, -) bool { +) remoteWake { layout, scanned := r.observeLayout(target) publishLayout(st, target, layout, scanned) now := r.clockNow() remote, remoteSeen := r.observeRemote(target, providerNS) - r.publishRemote(st, target, remote, remoteSeen, repo, now) - // Whether the refresh this reconcile goes on to enqueue will actually reach the remote: the - // worker spends a connection only when what it holds is older than the interval configured. - return r.GitRefreshInterval > 0 && (!remoteSeen || remote.Age(now) >= r.GitRefreshInterval) + heldFor := r.publishRemote(st, target, remote, remoteSeen, repo, now) + return remoteWake{ + // Whether the refresh this reconcile goes on to enqueue will actually reach the remote: the + // worker spends a connection only when what it holds is older than the interval configured. + asked: r.GitRefreshInterval > 0 && (!remoteSeen || remote.Age(now) >= r.GitRefreshInterval), + heldFor: heldFor, + } +} + +// remoteWake is what status.remote needs from the next reconcile's timing. +type remoteWake struct { + // asked: this reconcile asked the remote where the branch is, and the answer lands after it. + asked bool + // heldFor: the floor holds back an answer this target already has, for this long. + heldFor time.Duration } // requeueForRemoteAnswer shortens the cadence for a target that has just ASKED the remote where -// its branch is. +// its branch is, or that holds an answer the floor has not let out yet. // // It is the one piece of scheduling this status field gets, and it exists because the refresh is // enqueued during the reconcile: the worker answers moments later, after the status write, so the @@ -942,13 +954,25 @@ func (r *GitTargetReconciler) publishGitObservations( // spend a connection every interval and publish what it learned a whole tick late — visible to an // operator as a target that follows a foreign push ten minutes after noticing it. // -// Nothing else is threaded anywhere. A reconcile that asked nothing keeps its own cadence, and no -// early return has to carry a deadline. -func requeueForRemoteAnswer(cadence time.Duration, asked bool) time.Duration { - if asked && RemotePublicationInterval < cadence { - return RemotePublicationInterval +// Asking is not enough on its own, because the requeue it returns can be lost. A refresh that +// finds the branch moved re-enqueues the target while the asking reconcile is still running, and +// the queue folds a delayed requeue into an item that is already waiting (controller-runtime's +// priority queue keeps the earlier time). The reconcile that then runs holds a fresh answer, so it +// asks nothing, and the floor stops it publishing: without heldFor it returned the steady cadence, +// and the answer waited the whole tick after all. So whichever reconcile meets the held answer +// comes back when the floor lifts. +// +// Nothing else is threaded anywhere. A reconcile that asked nothing and holds nothing keeps its +// own cadence, and no early return has to carry a deadline. +func requeueForRemoteAnswer(cadence time.Duration, wake remoteWake) time.Duration { + next := cadence + if wake.asked && RemotePublicationInterval < next { + next = RemotePublicationInterval + } + if wake.heldFor > 0 && wake.heldFor < next { + next = wake.heldFor } - return cadence + return next } // ensureEventStream wires this GitTarget to the branch worker for the repository its GitProvider diff --git a/internal/controller/gittarget_remote.go b/internal/controller/gittarget_remote.go index ee9e45f01..41846d1a6 100644 --- a/internal/controller/gittarget_remote.go +++ b/internal/controller/gittarget_remote.go @@ -118,6 +118,9 @@ func (r *GitTargetReconciler) observeRemote( // Two things ignore the floor, because they are correctness rather than throughput: the FIRST // observation, which is the difference between "nothing has looked" and an answer, and any // correction that involves the repository changing under the target. +// +// It returns how long the floor is holding news back, zero when nothing is held. A held answer +// must not wait for whatever reconcile happens to come next: see requeueForRemoteAnswer. func (r *GitTargetReconciler) publishRemote( st *reconcileStatus, target *configbutleraiv1alpha3.GitTarget, @@ -125,7 +128,7 @@ func (r *GitTargetReconciler) publishRemote( seen bool, repo git.RepoIdentity, now time.Time, -) { +) time.Duration { ref := types.NewResourceReference(target.Name, target.Namespace) published, everPublished := r.remotePublications.last(ref, target.UID) // What this target PUBLISHED was proved against a repository it no longer points at: its @@ -137,12 +140,12 @@ func (r *GitTargetReconciler) publishRemote( // published with, and it has to go rather than go stale. target.Status.Remote = nil st.afterPersist(func() { r.remotePublications.forget(ref) }) - return + return 0 } if !seen { // Nothing has looked at this branch since the process started. What is published is the // last thing anyone proved about it, and there is nothing better to replace it with. - return + return 0 } next := &configbutleraiv1alpha3.GitTargetRemoteStatus{ Commit: observed.Commit, @@ -151,11 +154,11 @@ func (r *GitTargetReconciler) publishRemote( Parent: parentStatusOf(observed), } if !remoteStatusIsNews(target.Status.Remote, next) { - return + return 0 } // The floor, skipped for the two corrections above and for the first publication. if everPublished && !staleRepository && now.Sub(published.at) < RemotePublicationInterval { - return + return published.at.Add(RemotePublicationInterval).Sub(now) } target.Status.Remote = next // Only once the write has landed, and dated THEN rather than now. A patch refused by the @@ -166,6 +169,7 @@ func (r *GitTargetReconciler) publishRemote( // so on a slow reconcile a cooldown dated from it can be half spent before the write lands, // and a reconcile queued behind it would publish again straight away. st.afterPersist(func() { r.remotePublications.record(ref, target.UID, r.clockNow(), observed.Repo) }) + return 0 } // withdrawRemote decides whether what is published has to be taken back rather than replaced. diff --git a/internal/controller/gittarget_remote_test.go b/internal/controller/gittarget_remote_test.go index c9b95c0e4..a0da683d2 100644 --- a/internal/controller/gittarget_remote_test.go +++ b/internal/controller/gittarget_remote_test.go @@ -417,12 +417,55 @@ func TestPublishRemote_ADeletedTargetReleasesItsLedgerEntry(t *testing.T) { // it, or an idle one spends a connection every interval and publishes what it learned a whole tick // late. A target that asked nothing keeps its own cadence. func TestRequeueForRemoteAnswer_ShortensOnlyWhenSomethingWasAsked(t *testing.T) { - assert.Equal(t, RemotePublicationInterval, requeueForRemoteAnswer(RequeueSteadyInterval, true)) - assert.Equal(t, RequeueSteadyInterval, requeueForRemoteAnswer(RequeueSteadyInterval, false)) - assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, true), + assert.Equal(t, RemotePublicationInterval, + requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{asked: true})) + assert.Equal(t, RequeueSteadyInterval, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{})) + assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, remoteWake{asked: true}), "a target already on a faster loop is not slowed down to the publication interval") } +// TestRequeueForRemoteAnswer_ComesBackWhenTheFloorLifts is the reconcile after a lost requeue. The +// asking reconcile's one-minute requeue is folded away when the refresh's answer re-enqueues the +// target while it runs; the reconcile that follows holds that answer fresh, asks nothing, and is +// stopped by the floor. It must come back when the floor lifts, not a whole steady tick later. +func TestRequeueForRemoteAnswer_ComesBackWhenTheFloorLifts(t *testing.T) { + held := 59 * time.Second + assert.Equal(t, held, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{heldFor: held})) + assert.Equal(t, held, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{asked: true, heldFor: held}), + "the earlier of the two wakes wins") + assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, remoteWake{heldFor: held}), + "a target already on a faster loop is not slowed down to the floor") +} + +// TestPublishRemote_ReportsHowLongTheFloorHoldsAnAnswer: the floor delays news, and says for how +// long, so the reconcile can come back for it. What is not news, and what is published, holds +// nothing back. +func TestPublishRemote_ReportsHowLongTheFloorHoldsAnAnswer(t *testing.T) { + r := &GitTargetReconciler{} + target := remoteTestTarget() + start := time.Now() + previous := r.clock + r.clock = func() time.Time { return start } + defer func() { r.clock = previous }() + + none := git.RepoIdentity{} + later := start.Add(2 * time.Second) + + st := &reconcileStatus{} + first := observation("aaaa", start, git.ObservedByPush, none) + assert.Zero(t, r.publishRemote(st, target, first, true, none, start), + "the first observation is published, so nothing is held") + st.runPersisted() + + moved := observation("bbbb", later, git.ObservedByFetch, none) + assert.Equal(t, 58*time.Second, r.publishRemote(&reconcileStatus{}, target, moved, true, none, later), + "news two seconds after a write waits out the rest of the floor") + assert.Equal(t, "aaaa", target.Status.Remote.Commit) + + assert.Zero(t, r.publishRemote(&reconcileStatus{}, target, first, true, none, later), + "what is already published is not news, so the floor holds nothing back") +} + // remoteLedgerFixture drives publishRemote through the REAL status session, so what advances the // ledger is a patch the API server accepted rather than a callback a test chose to run. type remoteLedgerFixture struct { diff --git a/test/e2e/flux_applier_e2e_test.go b/test/e2e/flux_applier_e2e_test.go index 4b0b6b480..a9439cac7 100644 --- a/test/e2e/flux_applier_e2e_test.go +++ b/test/e2e/flux_applier_e2e_test.go @@ -178,7 +178,7 @@ var _ = Describe("Manager Flux Applier", Label("manager", "flux-applier"), Order // With no receiver and no API edit, the only thing that can find a Git-side push is the // refresher on the GitTarget's steady tick. This is the guarantee that a missing webhook costs // latency and never an apply (docs/design/apply-trigger.md). - // The e2e install refreshes after 30s, so the wait is the control plane's reconcile cadence of + // The e2e install refreshes after 90s, so the wait is the control plane's reconcile cadence of // about five minutes, as in the refresh-remote spec. It("applies a Git-side push the refresher finds, with no receiver and no API edit", func() { Eventually(func(g Gomega) { diff --git a/test/e2e/refresh_remote_e2e_test.go b/test/e2e/refresh_remote_e2e_test.go index b6aecb377..a56f77fe2 100644 --- a/test/e2e/refresh_remote_e2e_test.go +++ b/test/e2e/refresh_remote_e2e_test.go @@ -26,14 +26,17 @@ import ( // one that writer left behind. // // Timing. The refresh rides the GitTarget reconcile, and a converged target requeues on the -// 5-minute steady interval, so the bound here is one steady tick plus the refresh itself — not -// the 30s --git-refresh-interval config/deployment.yaml sets, which only decides how old an -// observation may be when that tick arrives. +// 5-minute steady interval, so the bound on noticing the move is one steady tick plus the refresh +// itself — not the 90s --git-refresh-interval config/deployment.yaml sets, which only decides how +// old an observation may be when that tick arrives. What the refresh learns is published in the +// same round: the reconcile that asked comes back for the answer, and one the publication floor +// stops comes back when it lifts (requeueForRemoteAnswer). // -// status.remote is SAMPLED onto that same tick: proving where the branch is and publishing it are -// two different rates, and nothing in the data plane wakes a target because a branch moved. So -// every wait on that stanza below is one steady tick wide, including the one after our OWN push. -// That is the contract, not slack: see docs/spec/status-conditions-guide.md. +// status.remote is SAMPLED: proving where the branch is and publishing it are two different +// rates, and nothing in the data plane wakes a target because a branch moved. Our OWN push is +// published once RemotePublicationInterval has passed since the last write, which was the unborn +// branch moments before; a minute, not a tick. That is the contract, not slack: see +// docs/spec/status-conditions-guide.md. // // No reconcile-request annotation is used anywhere in this spec: an operator asking by hand is // precisely the case this feature exists to remove. @@ -104,7 +107,8 @@ var _ = Describe("Manager Remote Refresh", Label("manager", "refresh"), Ordered, g.Expect(pushedCommit).NotTo(BeEmpty(), "a target that has pushed knows where its branch is") g.Expect(gitTargetRemoteField(g, destName, testNs, "verifiedBy")).To(Equal("Push")) g.Expect(pushedCommit).To(Equal(remoteHeadSHA(g, repo.CheckoutDir))) - }, 7*time.Minute, 5*time.Second).Should(Succeed()) + }, 3*time.Minute, 5*time.Second).Should(Succeed(), + "published when the floor lifts, well before a steady tick") verifiedBefore := gitTargetRemoteField(Default, destName, testNs, "lastVerifiedAt") Expect(verifiedBefore).NotTo(BeEmpty(), "the observation is dated, or nothing can report its own staleness") From 852369e3853a609b88cd637e1363162130bc8984 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 06:37:49 +0000 Subject: [PATCH 5/7] test(e2e): let each Git-side timing sample settle before the next Each Git-side sample now holds, for 15s past its apply, that the branch has not moved and the live object keeps the pushed value: the apply mirrored back as no change. That outlasts the apply window's grace, so every sample times one change through a quiet loop. Without it, the Argo CD samples pushed about two seconds after the previous sync, and CI hit a race: the sync's own write to the ConfigMap, still pending in the apply window, was replayed onto the newer push and reverted it in Git (apply_window.go mirrors an applier's write nobody edited since the attempt, on the premise that Git already holds it). That race is reported on the PR, not hidden here: a later push lands as a commit after head and fails the settle. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/e2e/bi_directional_timing_e2e_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/e2e/bi_directional_timing_e2e_test.go b/test/e2e/bi_directional_timing_e2e_test.go index 42c91fa82..eb53ff357 100644 --- a/test/e2e/bi_directional_timing_e2e_test.go +++ b/test/e2e/bi_directional_timing_e2e_test.go @@ -219,6 +219,25 @@ func (p *timingPipeline) pushFromGitSide(value string) (string, time.Time) { return head, time.Now() } +// timingSettle outlasts the apply window's grace after an apply finishes (docs/design/apply-trigger.md), +// the time the applier's own write to the object can still be on its way back through the mirror. +const timingSettle = 15 * time.Second + +// settled holds, for timingSettle, that the branch is still at head and the live object still +// holds value: the apply mirrored back as no change. A sample starts only once the previous one +// has settled, so each times one change through a quiet loop. +// +// It is not only pacing. Pushing again while the previous apply's own write is still pending is +// a race this spec found with Argo CD: that write, replayed onto the newer push, reverted it. Here +// it would show as a commit after head. +func (p *timingPipeline) settled(head, value string) { + GinkgoHelper() + Consistently(func(g Gomega) { + g.Expect(p.remoteBranchHead(g)).To(Equal(head), "the apply must mirror back as no change") + g.Expect(p.liveValue(g)).To(Equal(value)) + }, timingSettle, time.Second).Should(Succeed()) +} + // remoteBranchHead is where main is on the server. func (p *timingPipeline) remoteBranchHead(g Gomega) string { _, err := gitRun(p.repo.CheckoutDir, "fetch", "origin", "main") @@ -333,6 +352,7 @@ var _ = Describe("Bi Directional timing (Flux)", Label("bi-directional", "flux", g.Expect(fluxLastAppliedRevision(g, fluxName)).To(ContainSubstring(head)) heldAgain(g, head) }) + pipeline.settled(head, value) } }) }) @@ -401,6 +421,7 @@ var _ = Describe("Bi Directional timing (Argo CD)", Label("bi-directional", "arg g.Expect(operation.Commit).To(Equal(head)) g.Expect(operation.Phase).To(Equal("Succeeded")) }) + pipeline.settled(head, value) } }) }) From d4aa56d30fea33207fca5901308287b5beebd031 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 06:37:49 +0000 Subject: [PATCH 6/7] test(e2e): wait for kcp to accept the admin credentials before the first workspace A source-cluster run had its first authenticated kcp request refused ("the server has asked for the client to provide credentials") a minute after setup-kcp.sh saw both Deployments rolled out and the admin kubeconfig Secret. The tunnel now waits for an authenticated read through the front-proxy before any spec uses it. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/e2e/kcp_workspace_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/e2e/kcp_workspace_test.go b/test/e2e/kcp_workspace_test.go index e7257cb30..ab5d470ad 100644 --- a/test/e2e/kcp_workspace_test.go +++ b/test/e2e/kcp_workspace_test.go @@ -88,6 +88,17 @@ func startKcpTunnel() *kcpTunnel { t := &kcpTunnel{cmd: cmd, cancel: cancel, admin: admin, workdir: workdir} t.rootKubeconfig = t.writeDerivedKubeconfig("root", fmt.Sprintf("https://127.0.0.1:%d/clusters/root", kcpLocalPort), kcpFrontProxySNI) + + // A port that accepts connections is not a front-proxy that accepts the admin's credentials. + // setup-kcp.sh waits for the Deployments and the admin kubeconfig Secret, and one CI run still + // had its first authenticated request refused ("the server has asked for the client to provide + // credentials") a minute after both. Wait for an authenticated read through the tunnel, so a + // startup that is still settling is not reported as a failing spec. + Eventually(func() error { + _, err := kubectlWithKubeconfig(t.rootKubeconfig, "", "get", "workspaces") + return err + }).WithTimeout(90*time.Second).WithPolling(3*time.Second). + Should(Succeed(), "kcp never accepted the admin credentials through the front-proxy") return t } From 47b171b97d0120df64934184956bf823d4e39f90 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 20 Oct 2026 07:09:20 +0000 Subject: [PATCH 7/7] test(e2e): wait out a same-seed rerun's terminating namespaces, and guard timing cleanup cleanupNamespace no longer waits for a namespace to finalize, which is safe within a run because every namespace is named for the run's seed. A rerun with the same seed (reproducing with --seed) could still find its namespaces terminating, and the specs ignore the create's AlreadyExists. Process 1 now waits for any terminating namespace with this run's seed prefix (or the pinned TESTNAMESPACE) before any spec runs: one wait in the rare case, instead of one per teardown. The timing specs' AfterAll skipped pipeline.cleanup() only by panicking when BeforeAll failed before building it; it is now guarded, so the rest of the cleanup runs and the BeforeAll failure is the only one reported. Both from CodeRabbit's review of #424. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/e2e/bi_directional_timing_e2e_test.go | 10 ++++++++-- test/e2e/e2e_suite_test.go | 23 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/test/e2e/bi_directional_timing_e2e_test.go b/test/e2e/bi_directional_timing_e2e_test.go index eb53ff357..f9f7657d5 100644 --- a/test/e2e/bi_directional_timing_e2e_test.go +++ b/test/e2e/bi_directional_timing_e2e_test.go @@ -278,7 +278,10 @@ var _ = Describe("Bi Directional timing (Flux)", Label("bi-directional", "flux", if !biDirectionalEnabled() { return } - pipeline.cleanup() + if pipeline != nil { + // nil when BeforeAll failed before building it. + pipeline.cleanup() + } cleanupNamespacedResource(testNs, fluxTriggerResource, fluxName) cleanupNamespacedResource("flux-system", "kustomization", fluxName) cleanupNamespacedResource("flux-system", "gitrepository", fluxName) @@ -395,7 +398,10 @@ var _ = Describe("Bi Directional timing (Argo CD)", Label("bi-directional", "arg cleanupNamespacedResource(run.argoNs, "application.argoproj.io", run.appName) cleanupNamespacedResource(run.argoNs, "secret", run.repoSecretName) cleanupNamespacedResource(testNs, argoCDTriggerResource, run.appName) - pipeline.cleanup() + if pipeline != nil { + // nil when BeforeAll failed before building it. + pipeline.cleanup() + } cleanupNamespace(testNs) }) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 7e4526bf7..fc148c309 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -63,6 +63,7 @@ var _ = SynchronizedBeforeSuite(func() []byte { By("preparing e2e cluster prerequisites via Task target") prepareE2EClusterOnce() + waitForPreviousRunNamespaces() // After prepare, while this process still holds the exclusive cluster lock, so every spec runs // against a Prometheus with no retained samples and a controller whose counters start at zero. @@ -72,6 +73,28 @@ var _ = SynchronizedBeforeSuite(func() []byte { configureE2EProcess() }) +// waitForPreviousRunNamespaces waits out test namespaces an earlier run with this seed is still +// deleting. cleanupNamespace does not wait for a namespace to finalize: every namespace is named +// for the run's seed (testNamespaceFor), so within one run nothing reads it again. A rerun with the +// SAME seed (reproducing a failure with --seed) is the exception, and would find its namespaces +// still terminating, where the specs' create ignores AlreadyExists and the first write is refused. +// Within one run there is nothing to wait for yet: this runs before any spec. +func waitForPreviousRunNamespaces() { + out, err := kubectlRun("get", "namespaces", "-o", + `jsonpath={range .items[?(@.status.phase=="Terminating")]}{.metadata.name}{"\n"}{end}`) + Expect(err).NotTo(HaveOccurred(), "failed to list terminating namespaces") + prefix := fmt.Sprintf("%d-test-", GinkgoRandomSeed()) + pinned := strings.TrimSpace(os.Getenv("TESTNAMESPACE")) + for _, name := range strings.Fields(out) { + if !strings.HasPrefix(name, prefix) && name != pinned { + continue + } + By(fmt.Sprintf("waiting for namespace %s, left by an earlier run with this seed, to finish deleting", name)) + _, err := kubectlRun("wait", "--for=delete", "namespace/"+name, "--timeout=180s") + Expect(err).NotTo(HaveOccurred(), "namespace %s is still terminating", name) + } +} + // resetE2EObservability discards Prometheus's retained samples and restarts the controller so its // in-process counters start from zero. Both halves are needed: resetting Prometheus alone leaves // the controller ready to republish its old totals on the very next scrape.