Enable noctx lint

[matejvasek]

This improves context propagation which might be useful for cancellation of running processes.

For example a user call some /libpod/ RESTful endpoint, and the handler of the endpoint itself does some other HTTP request.
If the user cancels the HTTP request to the /libpod API (i.e. closes TCP connection), the HTTP request invoked by the handler will be also cancelled now.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matejvasek
To complete the pull request process, please assign edsantiago after the PR has been reviewed.
You can assign the PR to them by writing /assign @edsantiago in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[TomSweeneyRedHat]

@matejvasek
Changes LGTM, but could you add a little description to the PR for historical purposes?

[vrothberg]

The PR is changing a lot of internal interfaces while the value (besides making the linter happy) is rather unclear to me. The biggest benefit of using a Context that I know is that it allows for cancelling go routines (if they support it) but I don't think that's the case in the proposed changes.

In case others feel different, I want to encourage to delay merging this PR for a couple of weeks. v3.0.0 just went out the door and merging the PR is likely causing conflicts when backporting from the main to the v3.0 branch. I think we'll be fixing some more bugs once 3.0 hits more users.

[matejvasek]

The biggest benefit of using a Context that I know is that it allows for cancelling go routines (if they support it) but I don't think that's the case in the proposed changes.

@vrothberg In this case it should allow to cancel some ongoing HTTP requests.
Question is if those HTTP can be so long running it's worth doing it.

[matejvasek]

If we wanted just make linter happy we could simply pass context.TODO().

[matejvasek]

But of course this is not super important and don't not need to be merge soon.

[matejvasek]

@vrothberg Imagine you call /libpod/images/import (and you use the URL parameter instead of body as a source). But then you think "Oh that's not I want to do!" and you cancel the request (close the connection) the download process will continue. I believe I might have even seen that happening. Of course this is not necessarily bad.

[matejvasek]

I believe I might have even see that happening.

Once I cancelled some request but saw server continuing with work. But I am not sure that it was image import operation.

[vrothberg]

@vrothberg Imagine you call /libpod/images/import (and you use the URL parameter instead of body as a source). But then you think "Oh that's not I want to do!" and you cancel the request (close the connection) the download process will continue. I believe I might have even seen that happening. Of course this is not necessarily bad.

(More) cancellable remote calls would be an improvement. In case of import, it would fail immediately since the client uploads the data but there are other cases like pull where it would be nice to have.

[matejvasek]

In case of import, it would fail immediately since the client uploads the data but there are other cases like pull where it would be nice to have.

From source code it looks like client don't have to necessarily upload data. It is possible to use query parameter named URL, if the parameter is used then data will be downloaded from there.

[matejvasek]

Flake again.

[vrothberg]

/hold

Holding the merge since I am still rather objecting to merge. If we want to, please wait a while until v3.0 further stabilized to avoid additional work for backports.

[rhatdan]

Yes let's wait for a while before merging. Would like to get @mheon Thoughts on this as well.

[mheon]

I do think this one is a good inclusion - it complicates the call stack but the API calls for volume plugins can potentially hang until their timeout, independent of the timeout of the actual request to get the volume, or mount it, or remove it. Tying it to the context of the request that spawned it makes a lot of sense.

…

On Mon, Feb 15, 2021 at 06:31 Daniel J Walsh ***@***.***> wrote: Yes let's wait for a while before merging. Would like to get @mheon <https://github.com/mheon> Thoughts on this as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9350 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCHM7Q7ZFOFHPGDWVB3S7EAY5ANCNFSM4XRM45KA> .

[openshift-ci-robot]

@matejvasek: PR needs rebase.

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.

[rhatdan]

@matejvasek Please rebase.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.

[rhatdan]

@matejvasek What do you want to do with this PR?

[rhatdan]

Since we have had no movement on this PR in a month, I am going to close. @matejvasek reopen if you still want to add it.

[matejvasek]

@rhatdan I think that at this point rebase would be difficult anyway, rewrite might be easier if we really want these changes.

[rhatdan]

You tell me if you think they are valuable.