Github alert issue: "could not list commit statuses: context canceled"

[artemlive]

Hello. I have an issue with the notification controller that is, I believe, related to this #385 PR.
When the controller tries to dispatch the Kustomization I see the error in the notification-controller logs:

{"level":"error","ts":"2022-08-21T15:15:37.267Z","logger":"event-server","msg":"failed to send notification","reconciler kind":"Kustomization","name":"nginx-ingress","namespace":"flux-system","error":"could not list commit statuses: context canceled"}

As far as I see the client's request is not synchronous and it doesn't wait while the request is processed. So it cancels the context because the incoming request from the client (which is the kustomization controller) had been closed before the notification controller could get the result from GitHub API.

Here is the Alert definition:

kind: Alert
metadata:
  creationTimestamp: "2022-08-21T13:09:12Z"
  generation: 2
  labels:
    kustomize.toolkit.fluxcd.io/name: flux-githib-alert
    kustomize.toolkit.fluxcd.io/namespace: flux-system
    manager: notification-controller
    operation: Update
    subresource: status
    time: "2022-08-21T13:09:12Z"
  name: github-status
  namespace: flux-system
  resourceVersion: "1982008"
  uid: f570eb8d-d72e-41ef-8636-c12884bd9751
spec:
  eventSeverity: info
  eventSources:
  - kind: Kustomization
    name: '*'
    namespace: flux-system
  - kind: GitRepository
    name: '*'
    namespace: flux-system
  providerRef:
    name: flux-system
status:
  conditions:
  - lastTransitionTime: "2022-08-21T13:09:12Z"
    message: Initialized
    observedGeneration: 2
    reason: Succeeded
    status: "True"
    type: Ready
  observedGeneration: 2

The provider definition looks like:

kind: Provider
metadata:
  creationTimestamp: "2022-08-21T12:54:00Z"
  generation: 1
  labels:
    kustomize.toolkit.fluxcd.io/name: flux-githib-alert
    kustomize.toolkit.fluxcd.io/namespace: flux-system
    manager: notification-controller
    operation: Update
    subresource: status
    time: "2022-08-21T13:06:47Z"
  name: flux-system
  namespace: flux-system
  resourceVersion: "1965423"
  uid: 572e565c-3255-4334-b811-28f0be59b504
spec:
  address: https://github.com/artemlive/my-repo
  secretRef:
    name: github
  type: github
status:
  conditions:
  - lastTransitionTime: "2022-08-21T13:06:47Z"
    message: Initialized
    observedGeneration: 1
    reason: Succeeded
    status: "True"
    type: Ready
  observedGeneration: 1

The controller creates the context from the incoming parent's request context and passes it to the notifier.Post() method:

notification-controller/internal/server/event_handlers.go

Line 66 in cef6398

ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second)

I've built my own version of the controller with the context.Background (for test purposes) instead of the ctx from the parent method and it works as expected.

	statuses, _, err := g.Client.Repositories.ListStatuses(context.Background(), g.Owner, g.Repo, rev, opts)
	if err != nil {
		return fmt.Errorf("could not list commit statuses: %v", err)
	}
	if duplicateGithubStatus(statuses, status) {
		return nil
	}

	_, _, err = g.Client.Repositories.CreateStatus(context.Background(), g.Owner, g.Repo, rev, status)

Instead of this.

notification-controller/internal/notifier/github.go

Lines 117 to 125 in cef6398

	statuses, _, err := g.Client.Repositories.ListStatuses(ctx, g.Owner, g.Repo, rev, opts)
	if err != nil {
	return fmt.Errorf("could not list commit statuses: %v", err)
	}
	if duplicateGithubStatus(statuses, status) {
	return nil
	}
	_, _, err = g.Client.Repositories.CreateStatus(ctx, g.Owner, g.Repo, rev, status)

[pjbgf]

Other Flux controllers have configurable Timeouts which allows users to set it according to their needs.
I think the same could be beneficial here.

During the Bug Scrub we discussed about the possibility of having a flag to define Timeouts at the handleEvent() level. And potentially a CRD level Spec.Timeout for the providers. But it would be great to get the views of other maintainers on the matter.

[stefanprodan]

I'm for adding a timeout field to the Provider spec with a default of 15s.

[somtochiama]

+1 on adding timeouts to the provider spec.

[phillebaba]

This is my fault. #408 should fix this by moving the defer to inside of the go routine instead.