prow: sidecar: allow configuring ignore interrupts #21644

viveksyngh · 2021-04-03T07:08:26Z

Add a field upload_ignores_interrupts to decoration config spec to configure ignore interrupts while performing
upload. It also tries to peform best-effort upload when upload_ignores_interrupts is set to false and interrupt
is received. While performing best-effort upload if main upload is ready it interrupts best-effort upload and switches
to main upload.

Fixes: #21167

k8s-ci-robot · 2021-04-03T07:08:34Z

Welcome @viveksyngh!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-04-03T07:08:34Z

Hi @viveksyngh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-04-03T07:08:39Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

alvaroaleman · 2021-04-07T16:35:11Z

@viveksyngh can you explain the motivation/usecase for this new knob?

viveksyngh · 2021-04-07T16:49:48Z

@alvaroaleman Sorry I didn't link the issue but I have created to fix this issue #21167

I have linked the issue now.

cjwagner

Thanks for working on this. Exposing the ignore_interrupts option is nice, but the fix to the best effort upload logic is even better. With that fix the option hopefully won't be needed at all.

cjwagner · 2021-04-13T00:03:34Z

prow/apis/prowjobs/v1/types.go

+
+	// IgnoreInterrupts causes sidecar to ignore interrupts and
+	// hope that the test process exits cleanly before starting an upload.
+	IgnoreInterrupts *bool `json:"ignore_interrupts,omitempty"`


I think we should rename this to something that indicates that this applies to the job result upload only and that interrupts will still be forwarded to the test process, e.g. upload_ignores_interrupts

Updated it to upload_ignores_interrupts

cjwagner · 2021-04-13T00:08:49Z

prow/pod-utils/decorate/podspec.go

+	var ignoreInterrupts bool
+	if pj.Spec.DecorationConfig.IgnoreInterrupts == nil { // if nil, set it to false
+		ignoreInterrupts = false
+	} else {
+		ignoreInterrupts = *pj.Spec.DecorationConfig.IgnoreInterrupts
+	}


nit: This is a bit simpler

Suggested change

var ignoreInterrupts bool

if pj.Spec.DecorationConfig.IgnoreInterrupts == nil { // if nil, set it to false

ignoreInterrupts = false

} else {

ignoreInterrupts = *pj.Spec.DecorationConfig.IgnoreInterrupts

}

ignoreInterrupts := pj.Spec.DecorationConfig.IgnoreInterrupts != nil && *pj.Spec.DecorationConfig.IgnoreInterrupts

cjwagner · 2021-04-13T00:25:23Z

prow/sidecar/run.go

@@ -108,6 +108,10 @@ func (o Options) Run(ctx context.Context) (int, error) {
 		return 0, fmt.Errorf("could not resolve job spec: %v", err)
 	}

+	entries := o.entries()
+	buildLogs := logReaders(entries)
+	metadata := combineMetadata(entries)


logReaders and combineMetadata cannot be called this early. They need to be called immediately before doUpload so that we don't open and read the files before they've been written.

Move these calls to immediately before doUpload and bestEffortUpload call.

cjwagner · 2021-04-13T00:28:06Z

prow/sidecar/run.go

@@ -125,7 +129,12 @@ func (o Options) Run(ctx context.Context) (int, error) {
 				// second upload but we can tolerate this as we'd rather get SOME
 				// data into GCS than attempt to cancel these uploads and get none.
 				logrus.Errorf("Received an interrupt: %s, cancelling...", s)
-				cancel()
+
+				err = o.doUpload(spec, false, true, metadata, buildLogs)


This case skips some of the steps before doUpload is called, in particular checking for the deprecated wrapper options and censoring the data before upload. Please put all this common logic in a function that can be called in both places.

Added a new method preUpload which performs steps required before upload and call it before both upload process.

cjwagner · 2021-04-13T00:34:16Z

prow/sidecar/run.go

@@ -125,7 +129,12 @@ func (o Options) Run(ctx context.Context) (int, error) {
 				// second upload but we can tolerate this as we'd rather get SOME
 				// data into GCS than attempt to cancel these uploads and get none.
 				logrus.Errorf("Received an interrupt: %s, cancelling...", s)
-				cancel()
+
+				err = o.doUpload(spec, false, true, metadata, buildLogs)


We need this preemptive upload to be interrupted if we see that the marker files are written and we're able to begin the main upload.
To achieve this we should instrument the call stack of doUpload with a context that can be used to cancel the upload early and then pass the ctx context here. That way if the wait function returns on line 148 we'll cancel the preemptive upload on line 150 and begin the main upload.

Handled this by adding a new method doBestEffortUpload which uses doUpload but also switches context when main upload is ready.

prow/sidecar/run.go

cjwagner

Sorry for the delayed review.

cjwagner · 2021-04-20T18:56:14Z

prow/sidecar/run.go

+		select {
+		case <-ctx.Done():
+			logrus.Infof("Interrupting best-effort upload as main upload is ready")
+			return


Returning here does not actually interrupt the goroutine created above. o.doUpload() will still run to completion unless we pass ctx as a parameter down its call stack to be used to interrupt the upload. Specifically o.doUpload() above, then

test-infra/prow/sidecar/run.go

Line 252 in 0512a33

if err := o.GcsOptions.Run(spec, uploadTargets); err != nil {

then

test-infra/prow/gcsupload/run.go

Line 60 in 0512a33

if err := gcs.Upload(o.Bucket, o.StorageClientOptions.GCSCredentialsFile, o.StorageClientOptions.S3CredentialsFile, uploadTargets); err != nil {

to replace the background context here

test-infra/prow/pod-utils/gcs/upload.go

Line 56 in 0512a33

ctx := context.Background()

After this change it won't be necessary to call doUpload() from a separate goroutine above.

Since we only need to be able to cancel the best effort doUpload() call, the main doUpload() call can use context.Background() rather than a cancelable context.

Update the functions to pass the context in stack and removed separate go routine

cjwagner · 2021-04-20T19:13:18Z

/ok-to-test

alvaroaleman · 2021-04-23T13:36:36Z

@cjwagner is this change really addressing the issue in question? From your original comment:

I think the comment describing the race is misleading and the logic could be improved here. In particular we don't actually try to perform the upload twice if an interrupt is received, we just immediately begin the upload then terminate. I would expect sidecar to immediately begin the upload, but then check/wait for the marker files to be written, and reupload when the markers are present

cjwagner

Looks good to me except for one thread safety issue.

cjwagner · 2021-04-23T20:59:41Z

prow/sidecar/run.go

 	buildLogs := logReaders(entries)
 	metadata := combineMetadata(entries)
-	return failures, o.doUpload(spec, passed, aborted, metadata, buildLogs)
+	ctx = context.Background()


I don't think changing the value of ctx here is threadsafe. Please use context.Background() directly or define a new variable.

Fixed this.

cjwagner · 2021-04-23T21:06:44Z

@cjwagner is this change really addressing the issue in question?

Yes, I believe so. It isn't very clear from PR title/description, but the main change in this PR is the fix of the best effort upload to actually do what the comment described. The PR also makes the interrupt ignoring behavior configurable which may not be useful if the main fix solves the problem, but it shouldn't hurt though.

Add a field `upload_ignores_interrupts` to decoration config spec to configure ignore interrupts while performing upload. It also tries to peform best-effort upload when `upload_ignores_interrupts` is set to `false` and interrupt is received. While performing best-effort upload if main upload is ready it interrupts best-effort upload and switches to main upload. Signed-off-by: Vivek Singh <[email protected]>

cjwagner

Thank you for the contribution, Vivek!

k8s-ci-robot · 2021-04-26T20:32:32Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~prow/OWNERS~~ [cjwagner]

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

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 3, 2021

k8s-ci-robot requested review from alvaroaleman and cjwagner April 3, 2021 07:08

viveksyngh changed the title ~~Add ignore interrupt to decorator config~~ Add ignore_interrupts field to decorator config Apr 3, 2021

viveksyngh changed the title ~~Add ignore_interrupts field to decorator config~~ Add ignore_interrupts field to decoration config Apr 3, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from 1a9819d to 9cd2d39 Compare April 3, 2021 08:58

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 3, 2021

viveksyngh changed the title ~~Add ignore_interrupts field to decoration config~~ prow: sidecar: allow configuring ignore interrupts Apr 3, 2021

viveksyngh mentioned this pull request Apr 4, 2021

Sidecar doesn't properly handle best effort upload and races with entrypoint. #21167

Closed

viveksyngh force-pushed the ignore-interrupt-flag branch from 9cd2d39 to 7a5619e Compare April 5, 2021 04:49

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 5, 2021

viveksyngh marked this pull request as ready for review April 5, 2021 04:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from 7a5619e to e19dc5d Compare April 7, 2021 16:25

k8s-ci-robot added the area/prow/sidecar Issues or PRs related to prow's sidecar component label Apr 7, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from e19dc5d to 8c875ac Compare April 8, 2021 19:19

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from 8c875ac to cdcc915 Compare April 10, 2021 04:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2021

cjwagner requested changes Apr 13, 2021

View reviewed changes

k8s-ci-robot assigned cjwagner Apr 13, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from cdcc915 to 7a71b58 Compare April 13, 2021 18:55

cjwagner reviewed Apr 20, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 20, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from 7a71b58 to 8e278cc Compare April 23, 2021 08:46

k8s-ci-robot added area/prow/gcsupload Issues or PRs related to prow's gcsupload component area/prow/initupload Issues or PRs related to prow's initupload component labels Apr 23, 2021

viveksyngh force-pushed the ignore-interrupt-flag branch from 8e278cc to 17bf4fb Compare April 23, 2021 09:21

cjwagner requested changes Apr 23, 2021

View reviewed changes

viveksyngh force-pushed the ignore-interrupt-flag branch from 17bf4fb to 102404d Compare April 24, 2021 07:15

cjwagner approved these changes Apr 26, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2021

k8s-ci-robot merged commit 1982976 into kubernetes:master Apr 26, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prow: sidecar: allow configuring ignore interrupts #21644

prow: sidecar: allow configuring ignore interrupts #21644

viveksyngh commented Apr 3, 2021 •

edited

Loading

k8s-ci-robot commented Apr 3, 2021

k8s-ci-robot commented Apr 3, 2021

k8s-ci-robot commented Apr 3, 2021

alvaroaleman commented Apr 7, 2021

viveksyngh commented Apr 7, 2021 •

edited

Loading

cjwagner left a comment

cjwagner Apr 13, 2021

viveksyngh Apr 13, 2021

cjwagner Apr 13, 2021

viveksyngh Apr 13, 2021

cjwagner Apr 13, 2021

viveksyngh Apr 13, 2021

cjwagner Apr 13, 2021

viveksyngh Apr 13, 2021

cjwagner Apr 13, 2021

viveksyngh Apr 13, 2021

cjwagner left a comment

cjwagner Apr 20, 2021

viveksyngh Apr 23, 2021

cjwagner commented Apr 20, 2021

alvaroaleman commented Apr 23, 2021 •

edited

Loading

cjwagner left a comment

cjwagner Apr 23, 2021

viveksyngh Apr 24, 2021

cjwagner commented Apr 23, 2021

cjwagner left a comment

k8s-ci-robot commented Apr 26, 2021

prow: sidecar: allow configuring ignore interrupts #21644

prow: sidecar: allow configuring ignore interrupts #21644

Conversation

viveksyngh commented Apr 3, 2021 • edited Loading

k8s-ci-robot commented Apr 3, 2021

k8s-ci-robot commented Apr 3, 2021

k8s-ci-robot commented Apr 3, 2021

alvaroaleman commented Apr 7, 2021

viveksyngh commented Apr 7, 2021 • edited Loading

cjwagner 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

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner commented Apr 20, 2021

alvaroaleman commented Apr 23, 2021 • edited Loading

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner commented Apr 23, 2021

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 26, 2021

viveksyngh commented Apr 3, 2021 •

edited

Loading

viveksyngh commented Apr 7, 2021 •

edited

Loading

alvaroaleman commented Apr 23, 2021 •

edited

Loading