Separate build ID from ProwJob ID for Jenkins jobs #6853

stevekuznetsov · 2018-02-15T21:51:52Z

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

/cc @BenTheElder @spxtr
/assign @Kargakis @cjwagner

stevekuznetsov · 2018-02-15T21:52:45Z

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 {


This is a bad API for the manual trigger but it's not super important to tackle right now.

cjwagner · 2018-02-15T22:21:42Z

prow/pjutil/jobspec.go

+// to vend build identifier for the job
+func GetBuildID(name, totURL string) (string, error) {
+	var err error
+	url := totURL + "/vend/" + name


Use path.Join in case totURL is specified with a trailing slash.

cjwagner · 2018-02-15T22:27:38Z

prow/pjutil/jobspec.go

+		if buf, err := ioutil.ReadAll(resp.Body); err == nil {
+			return string(buf), nil
+		}
+		return "", err


If ReadAll fails we won't return an error here because err is redefined instead of just being assigned in the conditional.

cjwagner · 2018-02-15T22:29:09Z

prow/pjutil/jobspec.go

+		}
+		defer resp.Body.Close()
+		if resp.StatusCode != 200 {
+			continue


Set err before continuing. Otherwise if we retry 60 times and always get a valid non-200 response we won't return an error.

cjwagner · 2018-02-15T22:32:25Z

prow/pjutil/jobspec.go

+	url := totURL + "/vend/" + name
+	for retries := 0; retries < 60; retries++ {
+		if retries > 0 {
+			time.Sleep(2 * time.Second)


Exponential backoff and fewer retries might be better than 60 retries made uniformly over 2 minutes.

cjwagner · 2018-02-15T22:34:51Z

prow/plank/controller.go

@@ -439,7 +436,7 @@ func (c *Controller) syncTriggeredJob(pj kube.ProwJob, pm map[string]kube.Pod, r
 	}

 	if pj.Status.State == kube.TriggeredState {
-		// BuildID needs to be set before we execute the job url template.
+		// ProwJobID needs to be set before we execute the job url template.


ProwJobID is not set.

Whoops, bad refactor rename button in IDEA

stevekuznetsov · 2018-02-15T22:59:17Z

@cjwagner made the requested changes to the tot connection code

cjwagner

@cjwagner made the requested changes to the tot connection code

Thanks for cleaning that up!
/lgtm

k8s-ci-robot · 2018-02-15T23:15:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~experiment/manual-trigger/OWNERS~~ [cjwagner,stevekuznetsov]
~~prow/OWNERS~~ [cjwagner,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2018-02-16T04:19:32Z

Test failure looks suspicious, and prow/cmd/jenkins-operator/main.go has a conflict (probably from #6850).

BenTheElder · 2018-02-16T04:20:19Z

Seconding the thanks though 😄
/area prow

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>

stevekuznetsov · 2018-02-16T16:43:42Z

@cjwagner fun:

url := path.Join([]string{"http://yuck:1234", "vend", "pls"})
// url is: 'http:/yuck:1234/vend/pls'
_, err := http.Get(url)
// err is 'Get http:///yuck:1234/vend/pls: http: no Host in request URL'
// see this         ^^^

Handling some cases where `err` asignment would not have happened correctly and the error would have been dropped, as well as using an exponential backoff and reducing the total number of retries done. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

0xmichalis · 2018-02-23T11:23:04Z

prow/kube/prowjob.go

+	// the ProwJob.ObjectMeta.Name field.
+	PodName string `json:"pod_name,omitempty"`
+
+	// ProwJobID is the build identifier vended by `tot`


This is not only vended by tot; there is also the option to use the snowflake library.

0xmichalis · 2018-02-23T11:26:54Z

prow/pjutil/jobspec.go

+	}
+	url.Path = path.Join(url.Path, "vend", name)
+	sleep := 100 * time.Millisecond
+	for retries := 0; retries < 10; retries++ {


We should start using apimachinery's ExponentialBackoff.

stevekuznetsov requested review from cjwagner and 0xmichalis as code owners February 15, 2018 21:51

k8s-ci-robot assigned cjwagner and 0xmichalis Feb 15, 2018

k8s-ci-robot requested review from BenTheElder and spxtr February 15, 2018 21:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2018

stevekuznetsov commented Feb 15, 2018

View reviewed changes

stevekuznetsov force-pushed the skuznets/jenkins-pj-id branch from 742f6fe to 3539019 Compare February 15, 2018 21:55

BenTheElder mentioned this pull request Feb 15, 2018

prow: core: one CRD Spec field fulfilled by many controllers makes Status muddy #6852

Closed

cjwagner reviewed Feb 15, 2018

View reviewed changes

stevekuznetsov force-pushed the skuznets/jenkins-pj-id branch 2 times, most recently from 3e64e4f to e738b08 Compare February 15, 2018 22:52

cjwagner approved these changes Feb 15, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2018

k8s-ci-robot added the area/prow Issues or PRs related to prow label Feb 16, 2018

Improve tot request code

1b9425a

Handling some cases where `err` asignment would not have happened correctly and the error would have been dropped, as well as using an exponential backoff and reducing the total number of retries done. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/jenkins-pj-id branch from 933f3a5 to 1b9425a Compare February 16, 2018 16:49

k8s-ci-robot merged commit 351c416 into kubernetes:master Feb 16, 2018

0xmichalis reviewed Feb 23, 2018

View reviewed changes

0xmichalis mentioned this pull request Feb 26, 2018

Cleanup fallback to BUILD_ID #6996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate build ID from ProwJob ID for Jenkins jobs #6853

Separate build ID from ProwJob ID for Jenkins jobs #6853

stevekuznetsov commented Feb 15, 2018

stevekuznetsov Feb 15, 2018

cjwagner Feb 15, 2018

cjwagner Feb 15, 2018

cjwagner Feb 15, 2018

cjwagner Feb 15, 2018

cjwagner Feb 15, 2018

stevekuznetsov Feb 15, 2018

stevekuznetsov commented Feb 15, 2018

cjwagner left a comment

k8s-ci-robot commented Feb 15, 2018

BenTheElder commented Feb 16, 2018 •

edited

Loading

BenTheElder commented Feb 16, 2018

stevekuznetsov commented Feb 16, 2018

0xmichalis Feb 23, 2018

0xmichalis Feb 23, 2018

Separate build ID from ProwJob ID for Jenkins jobs #6853

Separate build ID from ProwJob ID for Jenkins jobs #6853

Conversation

stevekuznetsov commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Feb 15, 2018

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2018

BenTheElder commented Feb 16, 2018 • edited Loading

BenTheElder commented Feb 16, 2018

stevekuznetsov commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 16, 2018 •

edited

Loading