Separate build ID from ProwJob ID for Jenkins jobs

It is the intent in the `initupload` and `sidecar` container utilities
to use the full information provided by Prow to the job in `$JOB_SPEC`
to determine where in GCS artifacts from the job should be pushed. Right
now, this is possible and useful for jobs triggered using `plank` but
not so for those triggered using `jenkins-operator`. The largest issue
is that for Jenkins builds, the `$BUILD_ID` parameter was being used
as an identifier to allow Prow to correlate builds with ProwJobs,
whereas in `plank` pods the parameter was a monotonically increasing
identifier vended by `tot` used for e.g. GCS. This patch introduces the
`$PROW_JOB_ID` variable and field to the job spec to separate these two
concerns and allow for them to be fulfilled together. We also thread
through links so that user-facing pages use the `$BUILD_ID` identifier
and not the `$BUILD_NUMBER` identifier that Jenkins produces. This also
handily solves issues with loss of state or Jenkins inconsistencies by
allowing the source of truth for job build state to live in `tot`.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

experiment/manual-trigger/manual-trigger.go

@@ -178,9 +178,9 @@ func main() {
 		MaxConcurrency: 1,
 	}
 
-	if err = jc.BuildFromSpec(&spec, o.jobName); err != nil {
+	if err = jc.BuildFromSpec(&spec, "0", o.jobName); err != nil {
 		log.Println("Submitting the following to Jenkins:")
-		env, _ := pjutil.EnvForSpec(pjutil.NewJobSpec(spec, "0"))
+		env, _ := pjutil.EnvForSpec(pjutil.NewJobSpec(spec, "0", o.jobName))
 		for k, v := range env {
 			log.Printf("  %s=%s\n", k, v)
 		}

prow/README.md

@@ -251,6 +251,7 @@ Variable | Periodic | Postsubmit | Batch | Presubmit | Description | Example
 `JOB_TYPE` | ✓ | ✓ | ✓ | ✓ | Type of job. | `presubmit`
 `JOB_SPEC` | ✓ | ✓ | ✓ | ✓ | JSON-encoded job specification. | see below
 `BUILD_ID` | ✓ | ✓ | ✓ | ✓ | Unique build number for each run. | `12345`
+`PROW_JOB_ID` | ✓ | ✓ | ✓ | ✓ | Unique identifier for the owning Prow Job. | `1ce07fa2-0831-11e8-b07e-0a58ac101036`
 `BUILD_NUMBER` | ✓ | ✓ | ✓ | ✓ | Unique build number for each run. | `12345`
 `REPO_OWNER` | | ✓ | ✓ | ✓ | GitHub org that triggered the job. | `kubernetes`
 `REPO_NAME` | | ✓ | ✓ | ✓ | GitHub repo that triggered the job. | `test-infra`
@@ -270,22 +271,22 @@ job types:
 
 Periodic Job:
 ```json
-{"type":"periodic","job":"job-name","buildid":"0","refs":{}}
+{"type":"periodic","job":"job-name","buildid":"0","prowjobid":"uuid","refs":{}}
 ```
 
 Postsubmit Job:
 ```json
-{"type":"postsubmit","job":"job-name","buildid":"0","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha"}}
+{"type":"postsubmit","job":"job-name","buildid":"0","prowjobid":"uuid","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha"}}
 ```
 
 Presubmit Job:
 ```json
-{"type":"presubmit","job":"job-name","buildid":"0","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha","pulls":[{"number":1,"author":"author-name","sha":"pull-sha"}]}}
+{"type":"presubmit","job":"job-name","buildid":"0","prowjobid":"uuid","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha","pulls":[{"number":1,"author":"author-name","sha":"pull-sha"}]}}
 ```
 
 Batch Job:
 ```json
-{"type":"batch","job":"job-name","buildid":"0","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha","pulls":[{"number":1,"author":"author-name","sha":"pull-sha"},{"number":2,"author":"other-author-name","sha":"second-pull-sha"}]}}
+{"type":"batch","job":"job-name","buildid":"0","prowjobid":"uuid","refs":{"org":"org-name","repo":"repo-name","base_ref":"base-ref","base_sha":"base-sha","pulls":[{"number":1,"author":"author-name","sha":"pull-sha"},{"number":2,"author":"other-author-name","sha":"second-pull-sha"}]}}
 ```
 
 ## Bots home

prow/jenkins/BUILD.bazel

@@ -22,6 +22,7 @@ go_library(
         "//prow/kube:go_default_library",
         "//prow/pjutil:go_default_library",
         "//prow/report:go_default_library",
+        "//vendor/github.com/bwmarrin/snowflake:go_default_library",
         "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
         "//vendor/github.com/sirupsen/logrus:go_default_library",
     ],

prow/jenkins/controller.go

@@ -22,6 +22,7 @@ import (
 	"strconv"
 	"sync"
 
+	"github.com/bwmarrin/snowflake"
 	"github.com/sirupsen/logrus"
 
 	"k8s.io/test-infra/prow/config"
@@ -42,7 +43,7 @@ type kubeClient interface {
 }
 
 type jenkinsClient interface {
-	Build(*kube.ProwJob) error
+	Build(*kube.ProwJob, string) error
 	ListBuilds(jobs []string) (map[string]JenkinsBuild, error)
 	Abort(job string, build *JenkinsBuild) error
 }
@@ -65,11 +66,13 @@ type syncFn func(kube.ProwJob, chan<- kube.ProwJob, map[string]JenkinsBuild) err
 
 // Controller manages ProwJobs.
 type Controller struct {
-	kc  kubeClient
-	jc  jenkinsClient
-	ghc githubClient
-	log *logrus.Entry
-	ca  configAgent
+	kc     kubeClient
+	jc     jenkinsClient
+	ghc    githubClient
+	log    *logrus.Entry
+	ca     configAgent
+	node   *snowflake.Node
+	totURL string
 	// selector that will be applied on prowjobs.
 	selector string
 
@@ -379,8 +382,9 @@ func (c *Controller) syncPendingJob(pj kube.ProwJob, reports chan<- kube.ProwJob
 			pj.Status.Description = "Jenkins job aborted."
 		}
 		// Construct the status URL that will be used in reports.
-		pj.Status.PodName = fmt.Sprintf("%s-%d", pj.Spec.Job, jb.Number)
-		pj.Status.BuildID = strconv.Itoa(jb.Number)
+		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 {
 			return fmt.Errorf("error executing URL template: %v", err)
@@ -407,8 +411,12 @@ func (c *Controller) syncTriggeredJob(pj kube.ProwJob, reports chan<- kube.ProwJ
 		if !c.canExecuteConcurrently(&pj) {
 			return nil
 		}
+		buildID, err := c.getBuildID(pj.Spec.Job)
+		if err != nil {
+			return fmt.Errorf("error getting build ID: %v", err)
+		}
 		// Start the Jenkins job.
-		if err := c.jc.Build(&pj); err != nil {
+		if err := c.jc.Build(&pj, buildID); err != nil {
 			c.log.WithError(err).WithFields(pjutil.ProwJobFields(&pj)).Warn("Cannot start Jenkins build")
 			pj.SetComplete()
 			pj.Status.State = kube.ErrorState
@@ -436,6 +444,13 @@ func (c *Controller) syncTriggeredJob(pj kube.ProwJob, reports chan<- kube.ProwJ
 	return err
 }
 
+func (c *Controller) getBuildID(name string) (string, error) {
+	if c.totURL == "" {
+		return c.node.Generate().String(), nil
+	}
+	return pjutil.GetBuildID(name, c.totURL)
+}
+
 // RunAfterSuccessCanRun returns whether a child job (specified as run_after_success in the
 // prow config) can run once its parent job succeeds. The only case we will not run a child job
 // is when it is a presubmit job and has a run_if_changed regural expression specified which does

prow/jenkins/controller_test.go

@@ -19,6 +19,8 @@ package jenkins
 import (
 	"errors"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"reflect"
 	"sync"
 	"testing"
@@ -132,7 +134,7 @@ type fjc struct {
 	builds map[string]JenkinsBuild
 }
 
-func (f *fjc) Build(pj *kube.ProwJob) error {
+func (f *fjc) Build(pj *kube.ProwJob, buildId string) error {
 	f.Lock()
 	defer f.Unlock()
 	if f.err != nil {
@@ -290,6 +292,10 @@ func TestSyncTriggeredJobs(t *testing.T) {
 		},
 	}
 	for _, tc := range testcases {
+		totServ := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprint(w, "42")
+		}))
+		defer totServ.Close()
 		t.Logf("scenario %q", tc.name)
 		fjc := &fjc{
 			err: tc.err,
@@ -303,6 +309,7 @@ func TestSyncTriggeredJobs(t *testing.T) {
 			jc:          fjc,
 			log:         logrus.NewEntry(logrus.StandardLogger()),
 			ca:          newFakeConfigAgent(t, tc.maxConcurrency, nil),
+			totURL:      totServ.URL,
 			lock:        sync.RWMutex{},
 			pendingJobs: make(map[string]int),
 		}
@@ -402,7 +409,7 @@ func TestSyncPendingJobs(t *testing.T) {
 			builds: map[string]JenkinsBuild{
 				"boing": {enqueued: false, Number: 10},
 			},
-			expectedURL:      "test-job-10/pending",
+			expectedURL:      "boing/pending",
 			expectedState:    kube.PendingState,
 			expectedEnqueued: false,
 			expectedReport:   true,
@@ -423,7 +430,7 @@ func TestSyncPendingJobs(t *testing.T) {
 			builds: map[string]JenkinsBuild{
 				"firstoutthetrenches": {enqueued: false, Number: 10},
 			},
-			expectedURL:    "test-job-10/pending",
+			expectedURL:    "firstoutthetrenches/pending",
 			expectedState:  kube.PendingState,
 			expectedReport: true,
 		},
@@ -473,7 +480,7 @@ func TestSyncPendingJobs(t *testing.T) {
 			builds: map[string]JenkinsBuild{
 				"winwin": {Result: pState(Succeess), Number: 11},
 			},
-			expectedURL:      "test-job-11/success",
+			expectedURL:      "winwin/success",
 			expectedState:    kube.SuccessState,
 			expectedComplete: true,
 			expectedReport:   true,
@@ -494,14 +501,18 @@ func TestSyncPendingJobs(t *testing.T) {
 			builds: map[string]JenkinsBuild{
 				"whatapity": {Result: pState(Failure), Number: 12},
 			},
-			expectedURL:      "test-job-12/failure",
+			expectedURL:      "whatapity/failure",
 			expectedState:    kube.FailureState,
 			expectedComplete: true,
 			expectedReport:   true,
 		},
 	}
 	for _, tc := range testcases {
 		t.Logf("scenario %q", tc.name)
+		totServ := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprint(w, "42")
+		}))
+		defer totServ.Close()
 		fjc := &fjc{
 			err: tc.err,
 		}
@@ -514,6 +525,7 @@ func TestSyncPendingJobs(t *testing.T) {
 			jc:          fjc,
 			log:         logrus.NewEntry(logrus.StandardLogger()),
 			ca:          newFakeConfigAgent(t, 0, nil),
+			totURL:      totServ.URL,
 			lock:        sync.RWMutex{},
 			pendingJobs: make(map[string]int),
 		}
@@ -596,11 +608,16 @@ func TestBatch(t *testing.T) {
 			"known_name": { /* Running */ },
 		},
 	}
+	totServ := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "42")
+	}))
+	defer totServ.Close()
 	c := Controller{
 		kc:          fc,
 		jc:          jc,
 		log:         logrus.NewEntry(logrus.StandardLogger()),
 		ca:          newFakeConfigAgent(t, 0, nil),
+		totURL:      totServ.URL,
 		pendingJobs: make(map[string]int),
 		lock:        sync.RWMutex{},
 	}
@@ -621,7 +638,7 @@ func TestBatch(t *testing.T) {
 	if fc.prowjobs[0].Status.Description != "Jenkins job running." {
 		t.Fatalf("Expected description %q, got %q.", "Jenkins job running.", fc.prowjobs[0].Status.Description)
 	}
-	if fc.prowjobs[0].Status.PodName != "pr-some-job-42" {
+	if fc.prowjobs[0].Status.PodName != "known_name" {
 		t.Fatalf("Wrong PodName: %s", fc.prowjobs[0].Status.PodName)
 	}
 	jc.builds["known_name"] = JenkinsBuild{Result: pState(Succeess)}
@@ -837,6 +854,10 @@ func TestMaxConcurrencyWithNewlyTriggeredJobs(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		totServ := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprint(w, "42")
+		}))
+		defer totServ.Close()
 		jobs := make(chan kube.ProwJob, len(test.pjs))
 		for _, pj := range test.pjs {
 			jobs <- pj
@@ -852,6 +873,7 @@ func TestMaxConcurrencyWithNewlyTriggeredJobs(t *testing.T) {
 			jc:          fjc,
 			log:         logrus.NewEntry(logrus.StandardLogger()),
 			ca:          newFakeConfigAgent(t, 0, nil),
+			totURL:      totServ.URL,
 			pendingJobs: test.pendingJobs,
 		}
 

prow/jenkins/jenkins.go

@@ -44,6 +44,7 @@ const (
 	buildID = "buildId"
 	// Key for unique build number across Jenkins builds.
 	newBuildID = "BUILD_ID"
+	prowJobID  = "PROW_JOB_ID"
 )
 
 const (
@@ -110,20 +111,54 @@ func (jb *JenkinsBuild) IsEnqueued() bool {
 	return jb.enqueued
 }
 
+// ProwJobID extracts the ProwJob identifier for the
+// Jenkins build. We prefer the PROW_JOB_ID parameter
+// if present but fall back to legacy parameters if
+// not.
+func (jb *JenkinsBuild) ProwJobID() string {
+	for _, parameter := range []string{prowJobID, newBuildID, buildID} {
+		for _, action := range jb.Actions {
+			for _, p := range action.Parameters {
+				if p.Name == parameter {
+					value, ok := p.Value.(string)
+					if !ok {
+						logrus.Errorf("Cannot determine %s value for %#v", p.Name, jb)
+						continue
+					}
+					return value
+				}
+			}
+		}
+	}
+	return ""
+}
+
+// BuildID extracts the `tot` identifier for the
+// Jenkins build. We return an empty string if
+// we are dealing with a build that does not have
+// the ProwJobID set explicitly, as in that case
+// the build identifier is not from `tot`.
 func (jb *JenkinsBuild) BuildID() string {
+	var buildID string
+	hasProwJobID := false
 	for _, action := range jb.Actions {
 		for _, p := range action.Parameters {
-			if p.Name == buildID || p.Name == newBuildID {
+			hasProwJobID = hasProwJobID || p.Name == prowJobID
+			if p.Name == newBuildID {
 				value, ok := p.Value.(string)
 				if !ok {
 					logrus.Errorf("Cannot determine %s value for %#v", p.Name, jb)
 					continue
 				}
-				return value
+				buildID = value
 			}
 		}
 	}
-	return ""
+
+	if !hasProwJobID {
+		return ""
+	}
+	return buildID
 }
 
 type Client struct {
@@ -321,17 +356,17 @@ func (c *Client) doRequest(method, path string) (*http.Response, error) {
 }
 
 // Build triggers a Jenkins build for the provided ProwJob. The name of
-// the ProwJob is going to be used as the buildId parameter that will help
-// us track the build before it's scheduled by Jenkins.
-func (c *Client) Build(pj *kube.ProwJob) error {
+// the ProwJob is going to be used as the Prow Job ID parameter that will
+// help us track the build before it's scheduled by Jenkins.
+func (c *Client) Build(pj *kube.ProwJob, buildId string) error {
 	c.logger.WithFields(pjutil.ProwJobFields(pj)).Info("Build")
-	return c.BuildFromSpec(&pj.Spec, pj.ObjectMeta.Name)
+	return c.BuildFromSpec(&pj.Spec, buildId, pj.ObjectMeta.Name)
 }
 
 // BuildFromSpec triggers a Jenkins build for the provided ProwJobSpec.
-// buildId helps us track the build before it's scheduled by Jenkins.
-func (c *Client) BuildFromSpec(spec *kube.ProwJobSpec, buildId string) error {
-	env, err := pjutil.EnvForSpec(pjutil.NewJobSpec(*spec, buildId))
+// prowJobId helps us track the build before it's scheduled by Jenkins.
+func (c *Client) BuildFromSpec(spec *kube.ProwJobSpec, buildId, prowJobId string) error {
+	env, err := pjutil.EnvForSpec(pjutil.NewJobSpec(*spec, buildId, prowJobId))
 	if err != nil {
 		return err
 	}
@@ -415,9 +450,9 @@ func (c *Client) GetEnqueuedBuilds(jobs []string) (map[string]JenkinsBuild, erro
 	}
 	jenkinsBuilds := make(map[string]JenkinsBuild)
 	for _, jb := range page.QueuedBuilds {
-		buildID := jb.BuildID()
+		prowJobID := jb.ProwJobID()
 		// Ignore builds with missing buildId parameters.
-		if buildID == "" {
+		if prowJobID == "" {
 			continue
 		}
 		// Ignore builds for jobs we didn't ask for.
@@ -432,7 +467,7 @@ func (c *Client) GetEnqueuedBuilds(jobs []string) (map[string]JenkinsBuild, erro
 			continue
 		}
 		jb.enqueued = true
-		jenkinsBuilds[buildID] = jb
+		jenkinsBuilds[prowJobID] = jb
 	}
 	return jenkinsBuilds, nil
 }
@@ -460,12 +495,12 @@ func (c *Client) GetBuilds(job string) (map[string]JenkinsBuild, error) {
 	}
 	jenkinsBuilds := make(map[string]JenkinsBuild)
 	for _, jb := range page.Builds {
-		buildID := jb.BuildID()
+		prowJobID := jb.ProwJobID()
 		// Ignore builds with missing buildId parameters.
-		if buildID == "" {
+		if prowJobID == "" {
 			continue
 		}
-		jenkinsBuilds[buildID] = jb
+		jenkinsBuilds[prowJobID] = jb
 	}
 	return jenkinsBuilds, nil
 }