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

Reverting #2261 #2265

Merged
merged 1 commit into from Nov 9, 2021
Merged

Reverting #2261 #2265

merged 1 commit into from Nov 9, 2021

Conversation

zroubalik
Copy link
Member

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

Reverting #2261 because concurrency has been already set on the second job in the worklflow:

concurrency: e2e-tests

Setting it once again on the workflow level causes problems like: https://github.com/kedacore/keda/actions/runs/1439326562


Execute e2e tests
Canceling since a higher priority waiting request for 'e2e-tests' exists

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@tomkerkhove
Copy link
Member

/run-e2e

@JorTurFer
Copy link
Member

/run-e2e

until we merge this PR, the workload it's blocking to itself 😆 .
The comment is processed in the main branch

@zroubalik
Copy link
Member Author

Yeah let's merge it

@zroubalik zroubalik merged commit f23c6d6 into kedacore:main Nov 9, 2021
@ahmelsayed
Copy link
Contributor

I think it cancels all the pending requests running the latest one that matches the concurrency tag. I think it's still better than clobbering 2 runs, but the downside is that an e2e run might get skipped and needs to be rescheduled vs having the person requesting a run know whether another workflow is in progress.

@ahmelsayed
Copy link
Contributor

Actually now that I think more about it, we can just have a different cluster for main vs PRs, then maintain the concurrency for each separately. How does that sound? we'll still have to be careful not to queue 2 PR E2Es at the same time as one will get cancelled.

@JorTurFer
Copy link
Member

are you sure about it? During the development, I saw that if you trigger another, the second will wait till the first one ending.
I have to improve the workload to add the execution, I will take a look at it then.
Related with having a second cluster, if it's possible it would be nice, at least from my pov, but if the concurrency works without cancelling others it could be enough

@JorTurFer
Copy link
Member

/run-e2e cron*

2 similar comments
@JorTurFer
Copy link
Member

/run-e2e cron*

@JorTurFer
Copy link
Member

/run-e2e cron*

@JorTurFer
Copy link
Member

JorTurFer commented Nov 9, 2021

You are right @ahmelsayed , GitHub only keep 1 pending job and cancel the others
image

Basically they keep the current executing job and another waiting

@ahmelsayed
Copy link
Contributor

ahmelsayed commented Nov 9, 2021

The behavior I believe I see is:

runN refers to a run than needs the e2e-tests lock regardless of the workflow

run1: Running -> holds e2e-tests lock
run2: Pending -> waiting for e2e-test lock

then github will wait for run1 to finish and then start run2

run1: Running -> holds e2e-tests lock
run2: Pending -> waiting for e2e-test lock
run3: Pending -> waiting for e2e-test lock
run4: Pending -> waiting for e2e-test lock

then it'll evaluate to

run1: Running -> holds e2e-tests lock
run2: Cancelled
run3: Cancelled
run4: Pending -> waiting for e2e-test lock

@ahmelsayed
Copy link
Contributor

Basically they keep the current executing job and another waiting

The only other knob is cancel-in-progress: true

https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#concurrency

@ahmelsayed
Copy link
Contributor

I'll setup another cluster for PR e2e vs main e2e

@JorTurFer
Copy link
Member

image

@zroubalik
Copy link
Member Author

Interesting, I didn't know about the limitation on number of pending jobs. Good catch @ahmelsayed !

@zroubalik
Copy link
Member Author

Having another cluster for pr e2e tests would be great 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants