Skip to content

Commit efd3ed3

Browse files
committed
fix: Annotate PLR when started status is reported
The use of the state label (which is mutable) for deciding when to report to the SCM that the PLR was started is flaky. It was seen that the reconciler get events about PLRs with unexpected value for the state label. For example, after the status is reported to the SCM, and the label value is patched to "started", after serval reconcile iterations the label had the "queued" value again. This can happen because of unsafe patching done by controllers (not just the PAC controllers) which reconciles PLRs. Introduce a new annotation for indicating the the status was reported to the SCM. By adding an annotation which is set once, we remove the risk that its value will get overwritten by other controllers (since maps are merged when patched, values are not getting removed unless explicitly defined in the patch - https://datatracker.ietf.org/doc/html/rfc7386#section-2) In addition, at the start of each reconcile loop, ensure that we operate on the latest version of the PLR and not using a stale value from the cache. Assisted-By: Cursor Signed-off-by: Gal Ben Haim <[email protected]>
1 parent fa877bb commit efd3ed3

File tree

5 files changed

+280
-41
lines changed

5 files changed

+280
-41
lines changed

pkg/apis/pipelinesascode/keys/keys.go

Lines changed: 38 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -23,43 +23,44 @@ import (
2323
)
2424

2525
const (
26-
ControllerInfo = pipelinesascode.GroupName + "/controller-info"
27-
Task = pipelinesascode.GroupName + "/task"
28-
Pipeline = pipelinesascode.GroupName + "/pipeline"
29-
URLOrg = pipelinesascode.GroupName + "/url-org"
30-
URLRepository = pipelinesascode.GroupName + "/url-repository"
31-
SHA = pipelinesascode.GroupName + "/sha"
32-
Sender = pipelinesascode.GroupName + "/sender"
33-
EventType = pipelinesascode.GroupName + "/event-type"
34-
Branch = pipelinesascode.GroupName + "/branch"
35-
SourceBranch = pipelinesascode.GroupName + "/source-branch"
36-
Repository = pipelinesascode.GroupName + "/repository"
37-
GitProvider = pipelinesascode.GroupName + "/git-provider"
38-
State = pipelinesascode.GroupName + "/state"
39-
ShaTitle = pipelinesascode.GroupName + "/sha-title"
40-
ShaURL = pipelinesascode.GroupName + "/sha-url"
41-
RepoURL = pipelinesascode.GroupName + "/repo-url"
42-
SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url"
43-
PullRequest = pipelinesascode.GroupName + "/pull-request"
44-
InstallationID = pipelinesascode.GroupName + "/installation-id"
45-
GHEURL = pipelinesascode.GroupName + "/ghe-url"
46-
SourceProjectID = pipelinesascode.GroupName + "/source-project-id"
47-
TargetProjectID = pipelinesascode.GroupName + "/target-project-id"
48-
OriginalPRName = pipelinesascode.GroupName + "/original-prname"
49-
GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret"
50-
CheckRunID = pipelinesascode.GroupName + "/check-run-id"
51-
OnEvent = pipelinesascode.GroupName + "/on-event"
52-
OnComment = pipelinesascode.GroupName + "/on-comment"
53-
OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch"
54-
OnPathChange = pipelinesascode.GroupName + "/on-path-change"
55-
OnLabel = pipelinesascode.GroupName + "/on-label"
56-
OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore"
57-
OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression"
58-
TargetNamespace = pipelinesascode.GroupName + "/target-namespace"
59-
MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs"
60-
CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress"
61-
LogURL = pipelinesascode.GroupName + "/log-url"
62-
ExecutionOrder = pipelinesascode.GroupName + "/execution-order"
26+
ControllerInfo = pipelinesascode.GroupName + "/controller-info"
27+
Task = pipelinesascode.GroupName + "/task"
28+
Pipeline = pipelinesascode.GroupName + "/pipeline"
29+
URLOrg = pipelinesascode.GroupName + "/url-org"
30+
URLRepository = pipelinesascode.GroupName + "/url-repository"
31+
SHA = pipelinesascode.GroupName + "/sha"
32+
Sender = pipelinesascode.GroupName + "/sender"
33+
EventType = pipelinesascode.GroupName + "/event-type"
34+
Branch = pipelinesascode.GroupName + "/branch"
35+
SourceBranch = pipelinesascode.GroupName + "/source-branch"
36+
Repository = pipelinesascode.GroupName + "/repository"
37+
GitProvider = pipelinesascode.GroupName + "/git-provider"
38+
State = pipelinesascode.GroupName + "/state"
39+
ShaTitle = pipelinesascode.GroupName + "/sha-title"
40+
ShaURL = pipelinesascode.GroupName + "/sha-url"
41+
RepoURL = pipelinesascode.GroupName + "/repo-url"
42+
SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url"
43+
PullRequest = pipelinesascode.GroupName + "/pull-request"
44+
InstallationID = pipelinesascode.GroupName + "/installation-id"
45+
GHEURL = pipelinesascode.GroupName + "/ghe-url"
46+
SourceProjectID = pipelinesascode.GroupName + "/source-project-id"
47+
TargetProjectID = pipelinesascode.GroupName + "/target-project-id"
48+
OriginalPRName = pipelinesascode.GroupName + "/original-prname"
49+
GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret"
50+
CheckRunID = pipelinesascode.GroupName + "/check-run-id"
51+
OnEvent = pipelinesascode.GroupName + "/on-event"
52+
OnComment = pipelinesascode.GroupName + "/on-comment"
53+
OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch"
54+
OnPathChange = pipelinesascode.GroupName + "/on-path-change"
55+
OnLabel = pipelinesascode.GroupName + "/on-label"
56+
OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore"
57+
OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression"
58+
TargetNamespace = pipelinesascode.GroupName + "/target-namespace"
59+
MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs"
60+
CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress"
61+
LogURL = pipelinesascode.GroupName + "/log-url"
62+
ExecutionOrder = pipelinesascode.GroupName + "/execution-order"
63+
SCMReportingPLRStarted = pipelinesascode.GroupName + "/scm-reporting-plr-started"
6364
// PublicGithubAPIURL default is "https://api.github.com" but it can be overridden by X-GitHub-Enterprise-Host header.
6465
PublicGithubAPIURL = "https://api.github.com"
6566
GithubApplicationID = "github-application-id"

pkg/pipelineascode/pipelineascode.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,9 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
257257
whatPatching = "annotations.state and labels.state"
258258
patchAnnotations[keys.State] = kubeinteraction.StateQueued
259259
patchLabels[keys.State] = kubeinteraction.StateQueued
260+
} else {
261+
patchAnnotations[keys.SCMReportingPLRStarted] = "true"
262+
whatPatching = fmt.Sprintf("annotation.%s", keys.SCMReportingPLRStarted)
260263
}
261264

262265
if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil {
@@ -278,6 +281,20 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
278281
// unneeded SIGSEGV's
279282
return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
280283
}
284+
currentReason := ""
285+
if len(pr.Status.GetConditions()) > 0 {
286+
currentReason = pr.Status.GetConditions()[0].GetReason()
287+
}
288+
289+
p.logger.Infof("PipelineRun %s/%s patched successfully - Spec.Status: %s, State annotation: '%s', SCMReportingPLRStarted annotation: '%s', Status reason: '%s', Git provider status: '%s', Patched: %s",
290+
pr.GetNamespace(),
291+
pr.GetName(),
292+
pr.Spec.Status,
293+
pr.GetAnnotations()[keys.State],
294+
pr.GetAnnotations()[keys.SCMReportingPLRStarted],
295+
currentReason,
296+
status.Status,
297+
whatPatching)
281298
}
282299

283300
return pr, nil

pkg/pipelineascode/pipelineascode_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,15 @@ func TestRun(t *testing.T) {
722722
state, ok := pr.GetAnnotations()[keys.State]
723723
assert.Assert(t, ok, "State hasn't been set on PR", state)
724724
assert.Equal(t, state, kubeinteraction.StateQueued)
725+
726+
// When PipelineRun is queued, SCMReportingPLRStarted should not be set
727+
_, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted]
728+
assert.Assert(t, !scmStartedExists, "SCMReportingPLRStarted should not be set for queued PipelineRuns")
729+
} else {
730+
// When PipelineRun is not queued, SCMReportingPLRStarted should be set to "true"
731+
scmStarted, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted]
732+
assert.Assert(t, scmStartedExists, "SCMReportingPLRStarted should be set for non-queued PipelineRuns")
733+
assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'")
725734
}
726735
}
727736
}

pkg/reconciler/reconciler.go

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,21 @@ var (
5454
func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun) pkgreconciler.Event {
5555
ctx = info.StoreNS(ctx, system.Namespace())
5656
logger := logging.FromContext(ctx).With("namespace", pr.GetNamespace())
57+
58+
logger.Debugf("reconciling pipelineRun %s/%s", pr.GetNamespace(), pr.GetName())
59+
60+
// Get fresh PipelineRun from API server to avoid informer cache staleness
61+
freshPR, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{})
62+
if err != nil {
63+
logger.Errorf("failed to get fresh pipelineRun %s/%s: %v", pr.GetNamespace(), pr.GetName(), err)
64+
return fmt.Errorf("cannot get fresh PipelineRun: %w", err)
65+
}
66+
67+
if freshPR.GetResourceVersion() != pr.GetResourceVersion() {
68+
logger.Debugf("using fresh pipelineRun data (cached version %s vs fresh version %s)", pr.GetResourceVersion(), freshPR.GetResourceVersion())
69+
pr = freshPR
70+
}
71+
5772
// if pipelineRun is in completed or failed state then return
5873
state, exist := pr.GetAnnotations()[keys.State]
5974
if exist && (state == kubeinteraction.StateCompleted || state == kubeinteraction.StateFailed) {
@@ -69,14 +84,20 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun
6984
// another controller outside PaC). To maintain consistency between the PipelineRun
7085
// status and the Git provider status, we update both the PipelineRun resource and
7186
// the corresponding status on the Git provider here.
72-
if reason == string(tektonv1.PipelineRunReasonRunning) && state == kubeinteraction.StateQueued {
87+
scmReportingPLRStarted, exist := pr.GetAnnotations()[keys.SCMReportingPLRStarted]
88+
startReported := exist && scmReportingPLRStarted == "true"
89+
logger.Debugf("pipelineRun %s/%s scmReportingPLRStarted=%v, exist=%v", pr.GetNamespace(), pr.GetName(), startReported, exist)
90+
91+
if reason == string(tektonv1.PipelineRunReasonRunning) && !startReported {
92+
logger.Infof("pipelineRun %s/%s is running but not yet reported to provider, updating status", pr.GetNamespace(), pr.GetName())
7393
repoName := pr.GetAnnotations()[keys.Repository]
7494
repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName)
7595
if err != nil {
7696
return fmt.Errorf("failed to get repository CR: %w", err)
7797
}
7898
return r.updatePipelineRunToInProgress(ctx, logger, repo, pr)
7999
}
100+
logger.Debugf("pipelineRun %s/%s condition not met: reason='%s', startReported=%v", pr.GetNamespace(), pr.GetName(), reason, startReported)
80101

81102
// if its a GitHub App pipelineRun PR then process only if check run id is added otherwise wait
82103
if _, ok := pr.Annotations[keys.InstallationID]; ok {
@@ -343,16 +364,24 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
343364
}
344365

345366
func (r *Reconciler) updatePipelineRunState(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, state string) (*tektonv1.PipelineRun, error) {
367+
currentState := pr.GetAnnotations()[keys.State]
368+
logger.Infof("updating pipelineRun %v/%v state from %s to %s", pr.GetNamespace(), pr.GetName(), currentState, state)
369+
annotations := map[string]string{
370+
keys.State: state,
371+
}
372+
if state == kubeinteraction.StateStarted {
373+
annotations[keys.SCMReportingPLRStarted] = "true"
374+
}
375+
346376
mergePatch := map[string]any{
347377
"metadata": map[string]any{
348378
"labels": map[string]string{
349379
keys.State: state,
350380
},
351-
"annotations": map[string]string{
352-
keys.State: state,
353-
},
381+
"annotations": annotations,
354382
},
355383
}
384+
356385
// if state is started then remove pipelineRun pending status
357386
if state == kubeinteraction.StateStarted {
358387
mergePatch["spec"] = map[string]any{

pkg/reconciler/reconciler_test.go

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,189 @@ func TestUpdatePipelineRunState(t *testing.T) {
299299

300300
assert.Equal(t, updatedPR.Annotations[keys.State], tt.state)
301301
assert.Equal(t, updatedPR.Spec.Status, tektonv1.PipelineRunSpecStatus(""))
302+
303+
// Test SCMReportingPLRStarted annotation for started state
304+
if tt.state == kubeinteraction.StateStarted {
305+
scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted]
306+
assert.Assert(t, exists, "SCMReportingPLRStarted annotation should exist when state is started")
307+
assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'")
308+
} else {
309+
_, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted]
310+
assert.Assert(t, !exists, "SCMReportingPLRStarted annotation should not exist for non-started states")
311+
}
312+
})
313+
}
314+
}
315+
316+
func TestReconcileKind_SCMReportingLogic(t *testing.T) {
317+
observer, _ := zapobserver.New(zap.InfoLevel)
318+
_ = zap.New(observer).Sugar()
319+
320+
tests := []struct {
321+
name string
322+
pipelineRun *tektonv1.PipelineRun
323+
shouldCallUpdateToInProgress bool
324+
description string
325+
}{
326+
{
327+
name: "Running reason without SCMReportingPLRStarted - should call updatePipelineRunToInProgress",
328+
pipelineRun: &tektonv1.PipelineRun{
329+
ObjectMeta: metav1.ObjectMeta{
330+
Namespace: "test",
331+
Name: "test-pr",
332+
Annotations: map[string]string{
333+
keys.State: kubeinteraction.StateQueued,
334+
keys.Repository: "test-repo",
335+
},
336+
},
337+
Spec: tektonv1.PipelineRunSpec{},
338+
Status: tektonv1.PipelineRunStatus{
339+
Status: knativeduckv1.Status{
340+
Conditions: knativeduckv1.Conditions{
341+
{
342+
Type: knativeapi.ConditionSucceeded,
343+
Status: corev1.ConditionUnknown,
344+
Reason: string(tektonv1.PipelineRunReasonRunning),
345+
},
346+
},
347+
},
348+
},
349+
},
350+
shouldCallUpdateToInProgress: true,
351+
description: "PipelineRun is Running but no SCM reporting done yet",
352+
},
353+
{
354+
name: "Running reason with SCMReportingPLRStarted=true - should NOT call updatePipelineRunToInProgress",
355+
pipelineRun: &tektonv1.PipelineRun{
356+
ObjectMeta: metav1.ObjectMeta{
357+
Namespace: "test",
358+
Name: "test-pr",
359+
Annotations: map[string]string{
360+
keys.State: kubeinteraction.StateStarted,
361+
keys.Repository: "test-repo",
362+
keys.SCMReportingPLRStarted: "true",
363+
},
364+
},
365+
Spec: tektonv1.PipelineRunSpec{},
366+
Status: tektonv1.PipelineRunStatus{
367+
Status: knativeduckv1.Status{
368+
Conditions: knativeduckv1.Conditions{
369+
{
370+
Type: knativeapi.ConditionSucceeded,
371+
Status: corev1.ConditionUnknown,
372+
Reason: string(tektonv1.PipelineRunReasonRunning),
373+
},
374+
},
375+
},
376+
},
377+
},
378+
shouldCallUpdateToInProgress: false,
379+
description: "PipelineRun is Running and SCM reporting already done",
380+
},
381+
{
382+
name: "Non-Running reason - should NOT call updatePipelineRunToInProgress",
383+
pipelineRun: &tektonv1.PipelineRun{
384+
ObjectMeta: metav1.ObjectMeta{
385+
Namespace: "test",
386+
Name: "test-pr",
387+
Annotations: map[string]string{
388+
keys.State: kubeinteraction.StateQueued,
389+
keys.Repository: "test-repo",
390+
},
391+
},
392+
Spec: tektonv1.PipelineRunSpec{
393+
Status: tektonv1.PipelineRunSpecStatusPending,
394+
},
395+
Status: tektonv1.PipelineRunStatus{
396+
Status: knativeduckv1.Status{
397+
Conditions: knativeduckv1.Conditions{
398+
{
399+
Type: knativeapi.ConditionSucceeded,
400+
Status: corev1.ConditionUnknown,
401+
Reason: string(tektonv1.PipelineRunReasonPending),
402+
},
403+
},
404+
},
405+
},
406+
},
407+
shouldCallUpdateToInProgress: false,
408+
description: "PipelineRun is not in Running state",
409+
},
410+
}
411+
412+
for _, tt := range tests {
413+
t.Run(tt.name, func(t *testing.T) {
414+
ctx, _ := rtesting.SetupFakeContext(t)
415+
416+
testRepo := &v1alpha1.Repository{
417+
ObjectMeta: metav1.ObjectMeta{
418+
Name: "test-repo",
419+
Namespace: tt.pipelineRun.GetNamespace(),
420+
},
421+
Spec: v1alpha1.RepositorySpec{
422+
URL: randomURL,
423+
},
424+
}
425+
426+
testData := testclient.Data{
427+
Repositories: []*v1alpha1.Repository{testRepo},
428+
PipelineRuns: []*tektonv1.PipelineRun{tt.pipelineRun},
429+
}
430+
stdata, informers := testclient.SeedTestData(t, ctx, testData)
431+
432+
// Track if updatePipelineRunToInProgress was called by checking state changes
433+
originalState := tt.pipelineRun.GetAnnotations()[keys.State]
434+
435+
r := &Reconciler{
436+
repoLister: informers.Repository.Lister(),
437+
run: &params.Run{
438+
Clients: clients.Clients{
439+
Tekton: stdata.Pipeline,
440+
},
441+
Info: info.Info{
442+
Pac: &info.PacOpts{
443+
Settings: settings.Settings{},
444+
},
445+
},
446+
},
447+
}
448+
449+
err := r.ReconcileKind(ctx, tt.pipelineRun)
450+
451+
// For test cases that should call updatePipelineRunToInProgress,
452+
// we expect no error and the state should be updated
453+
if tt.shouldCallUpdateToInProgress {
454+
assert.NilError(t, err, tt.description)
455+
456+
// Get the updated PipelineRun to check if state was changed
457+
updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{})
458+
assert.NilError(t, getErr)
459+
460+
// Should have been updated to started state with SCMReportingPLRStarted annotation
461+
assert.Equal(t, updatedPR.GetAnnotations()[keys.State], kubeinteraction.StateStarted)
462+
scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted]
463+
assert.Assert(t, exists, "SCMReportingPLRStarted should be set")
464+
assert.Equal(t, scmStarted, "true")
465+
} else {
466+
// For cases that should NOT call updatePipelineRunToInProgress,
467+
// the state should remain unchanged
468+
updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{})
469+
assert.NilError(t, getErr)
470+
471+
// State should be unchanged
472+
assert.Equal(t, updatedPR.GetAnnotations()[keys.State], originalState, tt.description)
473+
474+
// Check SCMReportingPLRStarted annotation based on original state
475+
scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted]
476+
if originalState == kubeinteraction.StateStarted {
477+
// If original state was already 'started', SCMReportingPLRStarted should exist and be "true"
478+
assert.Assert(t, exists, "SCMReportingPLRStarted should exist when original state is started")
479+
assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'")
480+
} else {
481+
// If original state was not 'started', SCMReportingPLRStarted should not exist
482+
assert.Assert(t, !exists, "SCMReportingPLRStarted should not exist when original state is not started")
483+
}
484+
}
302485
})
303486
}
304487
}

0 commit comments

Comments
 (0)