[Feature] Webhooks support

[yurinnick]

As part of the effort to enhance Kernel testing I am working with KernelCI to implement Patchwork support on their end (kernelci/kernelci-api#307). To make it happen I'd like to propose webhook feature for Patchwork, which will allow us to enable testing patches on KernelCI infrastructure and publish results and Patchwork checks.

I'd like to add webhook logic that will POST patch/series-related metadata [1] into specified URL from per-project webhook config [2]. Config will specify destination tree and branch, and events that should trigger the webhook.

[1] Example metadata

{
  "event": "patch-created",
  "revision": {
    "tree": "mainline",
    "url": "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git",
    "branch": "master"
  },
  "submitter": {
        "id": 1,
        "url": "<URL>",
        "name": "John Doe",
        "email": "example@example.com"
  },
  "patches": [
    {
      "id": 1,
      "hash": "<HASH>",
      "web_url": "<URL>",
      "date": "2023-07-25T05:12:34.478Z",
      "mbox": "<URL>"
    }
  ]
}

[2] Example config

{
  "webhook": {
    "url": "<URL>",
    "auth": {
      "bearer_token": "<TOKEN>"
    }
  },
  "revision": {
    "tree": "mainline",
    "url": "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git",
    "branch": "master"
  },
  "events": [
    "patch-created"
  ]
}

This is not finalized design and I am open to tweak any part of it to fit your requirements. Please let me know what you guys think. Thanks!

[stephenfin]

This has long been on my TODO list, so if you are willing to do it then I am happy to review.

Could I ask that you annotate the example config you provided? In particular, I am curious as to the meaning and significance of the revision field in both. We do not have the concept of a "branch" in a Patchwork project. The closest you can probably get is subject prefixes (e.g. [stable/2.2] Fix foo) but those are human-written and therefore subject to typos, being forgotten, etc. As such, I'd be concerned that this could be misleading.

One other concern is how to implement the hooks. This is a bit solution space'y, and I suspect you may have already though about this, but I'd like to ensure the sending of the webhooks is not handled in the main server processes. Doing so with a large number of receivers or a receiver that is slow to respond will cause these to hang. Instead, I suspect we'd want a queue (Redis?) with a separate process to pull queued webhook events and send them?

[stephenfin]

This has long been on my TODO list, so if you are willing to do it then I am happy to review.

Could I ask that you annotate the example config you provided? In particular, I am curious as to the meaning and significance of the revision field in both. We do not have the concept of a "branch" in a Patchwork project. The closest you can probably get is subject prefixes (e.g. [stable/2.2] Fix foo) but those are human-written and therefore subject to typos, being forgotten, etc. As such, I'd be concerned that this could be misleading.

Thinking on it more, I'm guessing your receiver will be handling webhooks for multiple projects. Could you just send a project object, like we do for /events? In fact, could we just use the /events resource representation wholesale?

[yurinnick]

Could I ask that you annotate the example config you provided? In particular, I am curious as to the meaning and significance of the revision field in both. We do not have the concept of a "branch" in a Patchwork project. The closest you can probably get is subject prefixes (e.g. [stable/2.2] Fix foo) but those are human-written and therefore subject to typos, being forgotten, etc. As such, I'd be concerned that this could be misleading.

So the revision concept is for specifying base tree and branch for a project. For example, so subsystems would like to apply patches on top of mainline tree master branch, so would prefer next. BPF system has it's own kernel tree and uses bpf-next branch for CI. So I'd like to make it flexibly for those kind of things.

One other concern is how to implement the hooks. This is a bit solution space'y, and I suspect you may have already though about this, but I'd like to ensure the sending of the webhooks is not handled in the main server processes. Doing so with a large number of receivers or a receiver that is slow to respond will cause these to hang. Instead, I suspect we'd want a queue (Redis?) with a separate process to pull queued webhook events and send them?

Tbh I didn't look into implementation details of it, but everything you said makes sense for me.

Thinking on it more, I'm guessing your receiver will be handling webhooks for multiple projects. Could you just send a project object, like we do for /events? In fact, could we just use the /events resource representation wholesale?

/events covers most things that I'd like to pass except a few pieces of data. First, base tree/branch/revision information which I described above. Second, I'd like to pass current patch + ordered list of dependent patches. Current implementation only supports passing all patches in the series, so, if I want to test patch 3/5 I have to query all patches for the series, find patch 3/5 and then get list of dependent patches.

[stephenfin]

Could I ask that you annotate the example config you provided? In particular, I am curious as to the meaning and significance of the revision field in both. We do not have the concept of a "branch" in a Patchwork project. The closest you can probably get is subject prefixes (e.g. [stable/2.2] Fix foo) but those are human-written and therefore subject to typos, being forgotten, etc. As such, I'd be concerned that this could be misleading.

So the revision concept is for specifying base tree and branch for a project. For example, so subsystems would like to apply patches on top of mainline tree master branch, so would prefer next. BPF system has it's own kernel tree and uses bpf-next branch for CI. So I'd like to make it flexibly for those kind of things.

And there isn't somewhere else that this would be better placed? I mean, all that Patchwork is providing here is notifications about new changes to test and the ability to fetch those changes: the actual configuration of the test (how to run tests, what test to run, etc.) is going to live somewhere else. Could this "somewhere else" not include branch and base tree information?

One other concern is how to implement the hooks. This is a bit solution space'y, and I suspect you may have already though about this, but I'd like to ensure the sending of the webhooks is not handled in the main server processes. Doing so with a large number of receivers or a receiver that is slow to respond will cause these to hang. Instead, I suspect we'd want a queue (Redis?) with a separate process to pull queued webhook events and send them?

Tbh I didn't look into implementation details of it, but everything you said makes sense for me.

Thinking on it more, I'm guessing your receiver will be handling webhooks for multiple projects. Could you just send a project object, like we do for /events? In fact, could we just use the /events resource representation wholesale?

/events covers most things that I'd like to pass except a few pieces of data. First, base tree/branch/revision information which I described above. Second, I'd like to pass current patch + ordered list of dependent patches. Current implementation only supports passing all patches in the series, so, if I want to test patch 3/5 I have to query all patches for the series, find patch 3/5 and then get list of dependent patches.

Not necessarily. If you watch for the patch-created event, you can use the /mbox URL with the ?series=* querystring parameter to downloaded the patch mbox with all dependencies included. Would that achieve what you need?

[yurinnick]

And there isn't somewhere else that this would be better placed? I mean, all that Patchwork is providing here is notifications about new changes to test and the ability to fetch those changes: the actual configuration of the test (how to run tests, what test to run, etc.) is going to live somewhere else. Could this "somewhere else" not include branch and base tree information?

I think you're right, and this can be done on KernelCI side by configuring these parameters for each project. So Patchwork will send project name and KernelCI will decide appropriate tree/branch for it.

Not necessarily. If you watch for the patch-created event, you can use the /mbox URL with the ?series=* querystring parameter to downloaded the patch mbox with all dependencies included. Would that achieve what you need?

Is it a part of the REST API? I can't find any mentions of /mbox in the documentation. Could you provide more details about this API please?

[yurinnick]

Btw, can we add /events API as HTTP Stream? It would be so much easier to handle subscribing and querying that way.

[stephenfin]

And there isn't somewhere else that this would be better placed? I mean, all that Patchwork is providing here is notifications about new changes to test and the ability to fetch those changes: the actual configuration of the test (how to run tests, what test to run, etc.) is going to live somewhere else. Could this "somewhere else" not include branch and base tree information?

I think you're right, and this can be done on KernelCI side by configuring these parameters for each project. So Patchwork will send project name and KernelCI will decide appropriate tree/branch for it.

👍

Not necessarily. If you watch for the patch-created event, you can use the /mbox URL with the ?series=* querystring parameter to downloaded the patch mbox with all dependencies included. Would that achieve what you need?

Is it a part of the REST API? I can't find any mentions of /mbox in the documentation. Could you provide more details about this API please?

Sort of. This is the URL that's exposed via the mbox attribute of a patch object at /patches/{patchID}. We provide this URL in the REST API to allow downloading of the mbox. We also expose it in the web UX (via the download button on a patch page) to allow same. You're right that we don't appear to document the series parameter for this though. I'll work on that.

[stephenfin]

Is it a part of the REST API? I can't find any mentions of /mbox in the documentation. Could you provide more details about this API please?

You're right that we don't appear to document the series parameter for this though. I'll work on that.

Done in c42d9b0

[stephenfin]

Btw, can we add /events API as HTTP Stream? It would be so much easier to handle subscribing and querying that way.

There should be nothing preventing this in Patchwork itself, save for some of Django's middleware. This would presumably need configuration from the reverse proxy?

[yurinnick]

Sort of. This is the URL that's exposed via the mbox attribute of a patch object at /patches/{patchID}. We provide this URL in the REST API to allow downloading of the mbox. We also expose it in the web UX (via the download button on a patch page) to allow same. You're right that we don't appear to document the series parameter for this though. I'll work on that.

Is it specific to API v1.3? I tried to query a few patches from patchwork.kernel.org and it doesn't seem to work.

There should be nothing preventing this in Patchwork itself, save for some of Django's middleware. This would presumably need configuration from the reverse proxy?

I believe so, more about it here: NGINX as a Reverse Stream Proxy

[stephenfin]

Sort of. This is the URL that's exposed via the mbox attribute of a patch object at /patches/{patchID}. We provide this URL in the REST API to allow downloading of the mbox. We also expose it in the web UX (via the download button on a patch page) to allow same. You're right that we don't appear to document the series parameter for this though. I'll work on that.

Is it specific to API v1.3? I tried to query a few patches from patchwork.kernel.org and it doesn't seem to work.

No, it's nothing to do with the API version. The API exposes an mbox parameter in the GET /patch/{patchID} response which is a URL to download the patch in mbox format. If you add ?series=* to this URL, the mbox with include the patch dependencies. The code that does that lives here.

For example, this patch shows:

{
    "id": 10539039,
    "url": "https://patchwork.kernel.org/api/patches/10539039/",
    "mbox": "https://patchwork.kernel.org/project/linux-iio/patch/20180721194049.23265-3-martin.blumenstingl@googlemail.com/mbox/"
}

If you use https://patchwork.kernel.org/project/linux-iio/patch/20180721194049.23265-3-martin.blumenstingl@googlemail.com/mbox/?series=* then the mbox will include the patch's series dependencies.

There should be nothing preventing this in Patchwork itself, save for some of Django's middleware. This would presumably need configuration from the reverse proxy?

I believe so, more about it here: NGINX as a Reverse Stream Proxy

Sweet. Happy to document this but as I don't administer a deployment (you want @jk-ozlabs for the ozlabs instance) I can't really test this myself.

[yurinnick]

If we can implement HTTP Stream endpoint for events, and we don't want to manage base revision configurations for projects , I think we can live without any additional functionality on Patchwork side. KernelCI Patchwork service will subscribe to the stream to look for patch-created events for configured projects, and it will trigger CI based on the information from the event.

I am wondering if it aligns with your vision, or you believe it would be beneficial for Patchwork to implement webhook in a more conventional way. Let me know what do you think.

[stephenfin]

If we can implement HTTP Stream endpoint for events, and we don't want to manage base revision configurations for projects , I think we can live without any additional functionality on Patchwork side. KernelCI Patchwork service will subscribe to the stream to look for patch-created events for configured projects, and it will trigger CI based on the information from the event.

I am wondering if it aligns with your vision, or you believe it would be beneficial for Patchwork to implement webhook in a more conventional way. Let me know what do you think.

So I've been looking into this in more detail, and I'm starting to think there is actually quite a bit of work to be done here. From what I can tell, the first big step would be updating the /events API view to use HTTPStreamingResponse instead of the standard HTTPResponse. There's an example of this here and a quick local test suggests that approach would work, but that example doesn't even touch how you would deploy this, which brings us to the other big thing. I note the following in the documentation:

There are performance considerations when doing this, though. Django, under WSGI, is designed for short-lived requests. Streaming responses will tie a worker process for the entire duration of the response. This may result in poor performance.

Seeing as we're deploying everything with WSGI currently rather than ASGI, this could be turning out to be a much larger change that we envisioned. If we can get HTTP streaming working as expected for your use case, then I certainly couldn't ask you to do any further work. I would imagine this approach would allow for a much simpler client side implementation (no need to expose a HTTP endpoint to receive webhooks requests). However, asking everyone to retool their stack completely will be a big ask.

[yurinnick]

From what I can tell, the first big step would be updating the /events API view to use HTTPStreamingResponse instead of the standard HTTPResponse.

To be clear, I am in favor of replacing /events api, but rather adding additional HTTP Stream endpoint. In this case there would be no need to redesign anything.

I would imagine this approach would allow for a much simpler client side implementation (no need to expose a HTTP endpoint to receive webhooks requests). However, asking everyone to retool their stack completely will be a big ask.

Would it be the case if we keep /events endpoint untouched?

So I guess, my main question is, would be adding additional HTTP Stream endpoint complicated and dealbreaking? If so, I think we can stick to my original webhook proposal.