Implement Async Pipeline Upload Flow #1927

triarius · 2023-01-23T23:51:34Z

This is a new method of uploading pipelines introduced in the agent API
which will reduce long held connections between client and server. It works
by first uploading the Pipeline with a client generated UUID to the same
route as before, but with the query parameter async=true set.

Unlike waiting up to 15 seconds as before, this will return status 202 immediately
with a Location header set to where to check the status of the upload.
The API will also set a Retry-After header to inform the client to sleep for this duration.
The agent will then poll the route returned as the Location header until the status of the upload is "applied".

If the API does not support the Async Upload Flow, it will return 200 for the initial pipeline upload instead of 202. Then the agent will revert to its previous behaviour and poll the pipeline upload route with a new pipeline upload each time (although with the same UUID so the API will not create new steps).

triarius · 2023-01-24T00:47:52Z

This requires functionality in the API to be deployed. So I am leaving it as draft until then

DrJosh9000

Looking good, but thought this deserved some comments...

clicommand/pipeline_upload.go

agent/pipeline_uploader_test.go

DrJosh9000 · 2023-02-08T04:47:40Z

agent/pipeline_uploader.go

+			// 422 responses will always fail no need to retry
+			if apierr := new(
+				api.ErrorResponse,
+			); errors.As(err, &apierr) && apierr.Response.StatusCode == http.StatusUnprocessableEntity {


I know I used new with errors.As like this previously, but that was really only because I wanted to do it on one line. If you're splitting it over multiple lines, then the suggested form in the errors package makes more sense to me:

var apierr *api.ErrorResponse if errors.As(err, &apierr) && ... {

I've changed this a bit. See my commit message for my reasoning.

DrJosh9000 · 2023-02-08T04:50:50Z

agent/pipeline_uploader.go

+
+var (
+	locationRegex = regexp.MustCompile(`jobs/(?P<jobID>[^/]+)/pipelines/(?P<uploadUUID>[^/]+)`)
+	ErrRegex      = func(s string, r *regexp.Regexp) error { return fmt.Errorf("regex %s failed to match %s", r, s) }


Three questions:

is it necessary for it to be a var and not simply a func? I don't see it being changed anywhere

is it necessary for it to be exported? I don't see it being used outside of the package

is "ErrRegex" a good name? That makes it sound like it is a *regexp.Regexp when it is actually a func

Apologies. I've made a better error now. I decided not to pass up the regex, just the location, as well.

agent/pipeline_uploader_test.go

agent/pipeline_uploader.go

Type assertions will fail if error wrapping is used

This is a very common use case, but the error unwrapping pattern is pretty tedious to write each time, and the resulting line of code is usually very long. The pattern itself is also easy to get wrong as you need to pass a pointer to a pointer to the second argument of `errors.As`, but it accepts a value of `any` type. The combination of a very long line and code that needs to be checked carefully is precarious, so I think abstracting this pattern is worth it.

Co-authored-by: Josh Deprez <jd@buildkite.com>

DrJosh9000

Looks good! 😎

I have a few more comments, but consider them all non-blocking.

DrJosh9000 · 2023-02-22T03:44:58Z

agent/pipeline_uploader.go

+
+var locationRegex = regexp.MustCompile(`jobs/(?P<jobID>[^/]+)/pipelines/(?P<uploadUUID>[^/]+)`)
+
+type PipelineUploader struct {


It would be unfair of me to ask Benno to add doc comments to all exported top-level identifiers, while failing to ask you to do the same...

I've added this (and everything else) in #1972

DrJosh9000 · 2023-02-22T04:27:14Z

agent/pipeline_uploader.go

+
+	if jobIDFromResponse != u.JobID {
+		return fmt.Errorf(
+			"JobID from API: %s does not match request: %s",


This seems like a job for %q instead of %s - if the two strings mismatch at this point, there's a bug, and so they could be anything, which %q is ideal for. The same goes for UUID below.

Nice, I'll use that.

DrJosh9000 · 2023-02-22T04:41:05Z

agent/pipeline_uploader.go

+	})
+}
+
+type ErrLocationParse struct {


If the error type isn't used / referred to elsewhere, should it be unexported instead?

But I am keen for more error types in general 😄 (not necessarily in this PR)

We should refer to it in tests, but we haven't tested this error so 🤷

DrJosh9000 · 2023-02-22T04:47:37Z

agent/pipeline_uploader_test.go

+	ctx := context.Background()
+	l := clicommand.CreateLogger(&clicommand.PipelineUploadConfig{LogLevel: "notice"})
+
+	type Test struct {


It's not wrong to start identifiers within a func with a capital, but it seems weird to see something that looks like an exported identifier that can't possibly be used like one...

It shouldn't be necessary to avoid conflicting with test as the loop variable below - the loop variable would shadow the type (e.g. https://go.dev/play/p/o8cOdW5oh2q)

But it does conflict with the argument to the closure used to avoid loop variable shenanigans: https://go.dev/play/p/VmzVE01PLdl

The type identifier does not need to exist. It's only because of the loop variable shenanigans that we're even having this conversation. I guess just reassigning it avoids all this, so I'll do that.

DrJosh9000 · 2023-02-22T04:52:26Z

agent/pipeline_uploader_test.go

+	}
+
+	for _, test := range tests {
+		func(test Test) {


The loop variable semantics changes can't land quickly enough, IMO.

For the record, I'm not opposed to either of the two approaches to solving this problem (the other being test := test)

I'm not a huge fan of statements that seem meaningless, like test := test. But as in this case we can avoid a lot of boilerplate with it, I'll do it.

DrJosh9000 · 2023-02-22T05:14:47Z

api/pipelines.go

+	return c.doRequest(req, nil)
+}
+
+// UploadPipelineAsync uploads the pipeline to the Buildkite Agent API. It does not wait for the upload to complete.


Surely it has to complete the upload (sending bytes to the server), but the point is to avoid waiting on "processing"? (I think all I'm saying is that "upload" is ambiguous.)

It's called an StepUpload in the API even after the HTTP upload finish. I agree that this is confusing. I've fixed the comment.

DrJosh9000 · 2023-02-22T05:17:51Z

agent/pipeline_uploader.go

+	RetrySleepFunc func(time.Duration)
+}
+
+func (u *PipelineUploader) AsyncUploadFlow(ctx context.Context, l logger.Logger) error {


From the perspective of the other package, this is the only way to upload a pipeline with the new PipelineUploader (is that correct?) So I wonder if it should be named Upload. The inner workings make use of an async approach, but the whole method isn't exactly "async".

Yep, the name was a holdover from my first attempt, where it did not fall back.

triarius · 2023-02-23T22:33:51Z

Thanks @DrJosh9000 I'm going to make a followup PR with your suggestions. This one has been floating around for a while and I don't want to have to rebase again.

See #1927 (comment)

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

…aded See #1927 (comment)

The asynchronicity is hidden from the consumer, and may not even happen, as observed in #1927 (comment)

See #1927 (comment)

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

…aded See #1927 (comment)

The asynchronicity is hidden from the consumer, and may not even happen, as observed in #1927 (comment)

triarius force-pushed the pdp-467-use-async-pipeline-upload-in-agent branch 7 times, most recently from 795dc59 to 69a1fb8 Compare February 4, 2023 06:08

triarius force-pushed the pdp-467-use-async-pipeline-upload-in-agent branch 2 times, most recently from fe8b0bf to 81cbb6f Compare February 5, 2023 12:12

triarius marked this pull request as ready for review February 5, 2023 12:29

triarius requested a review from a team February 5, 2023 12:29

triarius mentioned this pull request Feb 5, 2023

Enable async pipeline upload flow buildkite/cli#139

Merged

DrJosh9000 reviewed Feb 8, 2023

View reviewed changes

triarius force-pushed the pdp-467-use-async-pipeline-upload-in-agent branch 4 times, most recently from 46ba455 to d946116 Compare February 9, 2023 08:27

triarius requested a review from DrJosh9000 February 19, 2023 21:41

triarius mentioned this pull request Feb 21, 2023

Check that a pipeline upload error actually comes from the JSON marshaller and exit early #1963

Merged

triarius added 8 commits February 21, 2023 17:29

Fix status code magic number and error type assertions

54a253b

Type assertions will fail if error wrapping is used

Implement Async Pipeline Upload Flow

fce5356

Test Async Pipeline Upload Flow

a903a49

Fix bk/cli test

74fa945

Use artifact pipeline

7320517

Use right error reporting format

84995ac

Return an unwrappable error when location fails to parse

7e36f94

triarius force-pushed the pdp-467-use-async-pipeline-upload-in-agent branch 2 times, most recently from 46369fb to dbbe0d7 Compare February 21, 2023 11:00

Update agent/pipeline_uploader.go

a967d3b

Co-authored-by: Josh Deprez <jd@buildkite.com>

triarius force-pushed the pdp-467-use-async-pipeline-upload-in-agent branch from dbbe0d7 to a967d3b Compare February 21, 2023 11:04

DrJosh9000 approved these changes Feb 22, 2023

View reviewed changes

triarius merged commit b60066d into main Feb 23, 2023

triarius deleted the pdp-467-use-async-pipeline-upload-in-agent branch February 23, 2023 22:33

triarius mentioned this pull request Feb 24, 2023

Refactor pipeline upload code in response to a review #1972

Merged

triarius added a commit that referenced this pull request Feb 24, 2023

Add doc comment on type

239b9af

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 24, 2023

Quote values returned by the API on when them in an error message

d685015

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 24, 2023

Make the error type private

76a2cd0

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 24, 2023

Eliminate a named type and a loop closure in tests

d2f79e1

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

triarius added a commit that referenced this pull request Feb 24, 2023

Clarify the we are waiting for the pipeline to be processed, not uplo…

a4e8d40

…aded See #1927 (comment)

triarius added a commit that referenced this pull request Feb 24, 2023

Rename method to Upload

9f1ee2d

The asynchronicity is hidden from the consumer, and may not even happen, as observed in #1927 (comment)

triarius mentioned this pull request Feb 24, 2023

Fix extra pipeline uploaded in fallback upload flow #1973

Merged

triarius added a commit that referenced this pull request Feb 27, 2023

Add doc comment on type

6fa02db

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 27, 2023

Quote values returned by the API on when them in an error message

c710af6

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 27, 2023

Make the error type private

bd33b3f

See #1927 (comment)

triarius added a commit that referenced this pull request Feb 27, 2023

Eliminate a named type and a loop closure in tests

77e6b92

See #1927 (comment) We can remove the reassignment after [this experiment](golang/go#57969) is concluded.

triarius added a commit that referenced this pull request Feb 27, 2023

Clarify the we are waiting for the pipeline to be processed, not uplo…

bc03e70

…aded See #1927 (comment)

triarius added a commit that referenced this pull request Feb 27, 2023

Rename method to Upload

a2837cd

The asynchronicity is hidden from the consumer, and may not even happen, as observed in #1927 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Async Pipeline Upload Flow #1927

Implement Async Pipeline Upload Flow #1927

triarius commented Jan 23, 2023 •

edited

triarius commented Jan 24, 2023

DrJosh9000 left a comment

DrJosh9000 Feb 8, 2023

triarius Feb 9, 2023 •

edited

DrJosh9000 Feb 8, 2023

triarius Feb 9, 2023

DrJosh9000 left a comment

DrJosh9000 Feb 22, 2023

triarius Feb 25, 2023

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023 •

edited

DrJosh9000 Feb 22, 2023

triarius Feb 24, 2023

triarius commented Feb 23, 2023


		var locationRegex = regexp.MustCompile(`jobs/(?P<jobID>[^/]+)/pipelines/(?P<uploadUUID>[^/]+)`)

		type PipelineUploader struct {

Implement Async Pipeline Upload Flow #1927

Implement Async Pipeline Upload Flow #1927

Conversation

triarius commented Jan 23, 2023 • edited

triarius commented Jan 24, 2023

DrJosh9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius Feb 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a 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

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

triarius Feb 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triarius commented Feb 23, 2023

triarius commented Jan 23, 2023 •

edited

triarius Feb 9, 2023 •

edited

triarius Feb 24, 2023 •

edited