Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay in git-wait-for-pr Step Execution #3303

Open
3 tasks done
evantissot opened this issue Jan 17, 2025 · 3 comments
Open
3 tasks done

Delay in git-wait-for-pr Step Execution #3303

evantissot opened this issue Jan 17, 2025 · 3 comments

Comments

@evantissot
Copy link

Checklist

  • I've searched the issue queue to verify this is not a duplicate feature request.
  • I've pasted the output of kargo version, if applicable.
  • I've pasted logs, if applicable.

Motivation

I am experiencing an issue with the git-wait-for-pr step in my workflow, which is causing a delay of 5 minutes before it successfully completes. Here is a brief overview of my stage:

  • I have a git-open-pr step. This PR is automerge with a GitHub automation in place.
  • I have a git-wait-for-pr step

Problem:
The git-wait-for-pr step seems to take 5 minutes to succeed. Based on my debugging, I suspect that the issue arises because when the git-wait-for-pr step is first executed, the PR has not yet been merged. As a result, the step is requeued and executed again 5 minutes later, at which point it successfully completes.

Proposed Feature

  • Introduce a dedicated step to sleep or wait before executing the git-wait-for-pr step. This would ensure that the step succeeds on its first run by allowing enough time for the PR to be merged.
  • Allow users to configure the 5-minute requeue interval that is currently hard-coded. This would enable more flexibility in adjusting the wait time according to specific workflow needs.

Suggested Implementation

@krancour
Copy link
Member

I'm not opposed to making the retry interval configurable, but it would provide no guarantee of the step actually being retried at the specified interval. I think the consequences of configuring this poorly (increased back-pressure on the queue with "busy work," for instance) may make it better suited for adjustment at the system-level by an operator than at the Promotion or step level by a user.

i.e. I don't know how well it solves your particular problem.

Introduce a dedicated step to sleep or wait...

Controllers support a finite number of concurrent reconciliations, so a step of this nature can become a major bottleneck. This is the same reason that the git-wait-for-pr step (and the http step, too) doesn't use its own, internal retry loop. Imagine a whole slew of pending Promotions being fully blocked because a small handful of Promotions are just waiting for a PR to be merged.

A more reliable solution would be something like #647 so that a Promotion can be "jostled" after a PR it's waiting on has been merged.

Short term workaround, in case it helps even a little:

Reconciling the Stage (by refreshing) will put its active Promotion back on the queue immediately.

@hiddeco
Copy link
Contributor

hiddeco commented Jan 30, 2025

Imagine a whole slew of pending Promotions being fully blocked because a small handful of Promotions are just waiting for a PR to be merged.

An interesting development I want to take note of, is that controller-runtime now has builtin (experimental) support for priority queues (also see the release notes), which may actually allow us to prioritize/organize the promotion queue (and i.e., push retries further down on the queue than newly created promotions).

@krancour
Copy link
Member

@hiddeco that is indeed an interesting development that we ought to follow closely. Thanks for bringing it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants