From 05175c9029882ec1ad51b9527092644884b303a6 Mon Sep 17 00:00:00 2001 From: wuhuizuo Date: Mon, 6 Jan 2025 11:23:40 +0000 Subject: [PATCH 1/2] refactor(pkg/jenkins): change the state update logic - keep the prow job be `triggered` state until the jenkins build is running. - only set the prow job be `pending` state when the jenkins build is running rather then the jenkins build enqueued. Signed-off-by: wuhuizuo --- pkg/jenkins/controller.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/pkg/jenkins/controller.go b/pkg/jenkins/controller.go index df83d7427..af312d2d1 100644 --- a/pkg/jenkins/controller.go +++ b/pkg/jenkins/controller.go @@ -365,7 +365,7 @@ func (c *Controller) syncPendingJob(pj prowapi.ProwJob, reports chan<- prowapi.P case jb.IsRunning(): // Build still going. c.incrementNumPendingJobs(pj.Spec.Job) - if pj.Status.Description == "Jenkins job running." { + if pj.Status.Description == "Jenkins job running." && pj.Status.URL != "" { return nil } pj.Status.Description = "Jenkins job running." @@ -429,7 +429,7 @@ func (c *Controller) syncTriggeredJob(pj prowapi.ProwJob, reports chan<- prowapi // Record last known state so we can patch prevPJ := pj.DeepCopy() - if _, jbExists := jbs[pj.ObjectMeta.Name]; !jbExists { + if jb, jbExists := jbs[pj.ObjectMeta.Name]; !jbExists { // Do not start more jobs than specified. if !c.canExecuteConcurrently(&pj) { return nil @@ -446,20 +446,30 @@ func (c *Controller) syncTriggeredJob(pj prowapi.ProwJob, reports chan<- prowapi pj.Status.URL = c.cfg().StatusErrorLink pj.Status.Description = "Error starting Jenkins job." } else { - now := metav1.NewTime(c.clock.Now()) - pj.Status.PendingTime = &now - pj.Status.State = prowapi.PendingState pj.Status.Description = "Jenkins job enqueued." } } else { - // If a Jenkins build already exists for this job, advance the ProwJob to Pending and - // it should be handled by syncPendingJob in the next sync. - if pj.Status.PendingTime == nil { - now := metav1.NewTime(c.clock.Now()) - pj.Status.PendingTime = &now + if jb.IsRunning() { + // If a Jenkins build already exists for this job, advance the ProwJob to Pending and + // it should be handled by syncPendingJob in the next sync. + if pj.Status.PendingTime == nil { + now := metav1.NewTime(c.clock.Now()) + pj.Status.PendingTime = &now + } + pj.Status.State = prowapi.PendingState + pj.Status.Description = "Jenkins job running." + + // Construct the status URL that will be used in reports. + pj.Status.PodName = pj.ObjectMeta.Name + pj.Status.BuildID = jb.BuildID() + pj.Status.JenkinsBuildID = strconv.Itoa(jb.Number) + var b bytes.Buffer + if err := c.config().JobURLTemplate.Execute(&b, &pj); err != nil { + c.log.WithFields(pjutil.ProwJobFields(&pj)).Errorf("error executing URL template: %v", err) + } else { + pj.Status.URL = b.String() + } } - pj.Status.State = prowapi.PendingState - pj.Status.Description = "Jenkins job enqueued." } // Report to GitHub. reports <- pj From 31cd7b02a61f5b7310ff61eabb4a67d4994d2c46 Mon Sep 17 00:00:00 2001 From: wuhuizuo Date: Mon, 20 Jan 2025 15:27:17 +0800 Subject: [PATCH 2/2] test(pkg/jenkins): fix unit tests Signed-off-by: wuhuizuo --- pkg/jenkins/controller.go | 41 +++++++++++++++++----------------- pkg/jenkins/controller_test.go | 19 ++++++++-------- pkg/jenkins/jenkins.go | 2 +- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/pkg/jenkins/controller.go b/pkg/jenkins/controller.go index af312d2d1..e14f00dc4 100644 --- a/pkg/jenkins/controller.go +++ b/pkg/jenkins/controller.go @@ -448,27 +448,28 @@ func (c *Controller) syncTriggeredJob(pj prowapi.ProwJob, reports chan<- prowapi } else { pj.Status.Description = "Jenkins job enqueued." } - } else { - if jb.IsRunning() { - // If a Jenkins build already exists for this job, advance the ProwJob to Pending and - // it should be handled by syncPendingJob in the next sync. - if pj.Status.PendingTime == nil { - now := metav1.NewTime(c.clock.Now()) - pj.Status.PendingTime = &now - } - pj.Status.State = prowapi.PendingState - pj.Status.Description = "Jenkins job running." + } else if jb.IsEnqueued() { + // Still in queue. + pj.Status.Description = "Jenkins job enqueued." + } else if jb.IsRunning() { + // If a Jenkins build already exists for this job, advance the ProwJob to Pending and + // it should be handled by syncPendingJob in the next sync. + if pj.Status.PendingTime == nil { + now := metav1.NewTime(c.clock.Now()) + pj.Status.PendingTime = &now + } + pj.Status.State = prowapi.PendingState + pj.Status.Description = "Jenkins job running." - // Construct the status URL that will be used in reports. - pj.Status.PodName = pj.ObjectMeta.Name - pj.Status.BuildID = jb.BuildID() - pj.Status.JenkinsBuildID = strconv.Itoa(jb.Number) - var b bytes.Buffer - if err := c.config().JobURLTemplate.Execute(&b, &pj); err != nil { - c.log.WithFields(pjutil.ProwJobFields(&pj)).Errorf("error executing URL template: %v", err) - } else { - pj.Status.URL = b.String() - } + // Construct the status URL that will be used in reports. + pj.Status.PodName = pj.ObjectMeta.Name + pj.Status.BuildID = jb.BuildID() + pj.Status.JenkinsBuildID = strconv.Itoa(jb.Number) + var b bytes.Buffer + if err := c.config().JobURLTemplate.Execute(&b, &pj); err != nil { + c.log.WithFields(pjutil.ProwJobFields(&pj)).Errorf("error executing URL template: %v", err) + } else { + pj.Status.URL = b.String() } } // Report to GitHub. diff --git a/pkg/jenkins/controller_test.go b/pkg/jenkins/controller_test.go index 8d64188ec..0fdf669a6 100644 --- a/pkg/jenkins/controller_test.go +++ b/pkg/jenkins/controller_test.go @@ -189,7 +189,6 @@ func (f *fghc) EditCommentWithContext(_ context.Context, org, repo string, ID in func TestSyncTriggeredJobs(t *testing.T) { fakeClock := clocktesting.NewFakeClock(time.Now().Truncate(1 * time.Second)) - pendingTime := metav1.NewTime(fakeClock.Now()) var testcases = []struct { name string @@ -223,9 +222,9 @@ func TestSyncTriggeredJobs(t *testing.T) { }, expectedBuild: true, expectedReport: true, - expectedState: prowapi.PendingState, + expectedState: prowapi.TriggeredState, expectedEnqueued: true, - expectedPendingTime: &pendingTime, + expectedPendingTime: nil, }, { name: "start new job, error", @@ -292,9 +291,9 @@ func TestSyncTriggeredJobs(t *testing.T) { maxConcurrency: 21, expectedBuild: true, expectedReport: true, - expectedState: prowapi.PendingState, + expectedState: prowapi.TriggeredState, expectedEnqueued: true, - expectedPendingTime: &pendingTime, + expectedPendingTime: nil, }, } for _, tc := range testcases { @@ -398,14 +397,14 @@ func TestSyncPendingJobs(t *testing.T) { Job: "test-job", }, Status: prowapi.ProwJobStatus{ - State: prowapi.PendingState, + State: prowapi.TriggeredState, Description: "Jenkins job enqueued.", }, }, builds: map[string]Build{ "foofoo": {enqueued: true, Number: 10}, }, - expectedState: prowapi.PendingState, + expectedState: prowapi.TriggeredState, expectedEnqueued: true, }, { @@ -420,7 +419,7 @@ func TestSyncPendingJobs(t *testing.T) { }, Status: prowapi.ProwJobStatus{ State: prowapi.PendingState, - Description: "Jenkins job enqueued.", + Description: "Jenkins job running.", }, }, builds: map[string]Build{ @@ -635,7 +634,7 @@ func TestBatch(t *testing.T) { fakeProwJobClient := fake.NewSimpleClientset(&pj) jc := &fjc{ builds: map[string]Build{ - "known_name": { /* Running */ }, + "known_name": {enqueued: true}, }, } totServ := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -661,7 +660,7 @@ func TestBatch(t *testing.T) { if err != nil { t.Fatalf("failed to get prowjob from client: %v", err) } - if afterFirstSync.Status.State != prowapi.PendingState { + if afterFirstSync.Status.State != prowapi.TriggeredState { t.Fatalf("Wrong state: %v", afterFirstSync.Status.State) } if afterFirstSync.Status.Description != "Jenkins job enqueued." { diff --git a/pkg/jenkins/jenkins.go b/pkg/jenkins/jenkins.go index 64fac6d87..2cdafdce1 100644 --- a/pkg/jenkins/jenkins.go +++ b/pkg/jenkins/jenkins.go @@ -123,7 +123,7 @@ type JobInfo struct { // IsRunning means the job started but has not finished. func (jb *Build) IsRunning() bool { - return jb.Result == nil + return jb.Result == nil && !jb.enqueued } // IsSuccess means the job passed