-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
refactor: move autoremove into the jobexecutor #1463
Conversation
breaking: docker container are removed after job exit
@ChristopherHX this pull request has failed checks 🛠 |
@ChristopherHX this pull request has failed checks 🛠 |
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
+ Coverage 57.50% 60.59% +3.08%
==========================================
Files 32 44 +12
Lines 4594 6991 +2397
==========================================
+ Hits 2642 4236 +1594
- Misses 1729 2451 +722
- Partials 223 304 +81
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
🦙 MegaLinter status: ✅ SUCCESS
See errors details in artifact MegaLinter reports on CI Job page |
logger.WithField("jobResult", "success").Infof("\U0001F3C1 Job succeeded") | ||
var err error | ||
if rc.Config.AutoRemove || jobError == nil { | ||
err = info.stopContainer()(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail if the job was canceled before (e.g. by os signal).
Please make sure that a fallback context is used in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code to allow docker 1min for closing / removing the runner. Should be safe now.
Additionally I now reuse the existing joblogger instead of creating one. I always want to use my own logger provided to the RunContext executor.
Do we want to add a forceCancel context as field to the context, so we can force quit act by pressing ctrl+c twice? This could be the parent context of the context with a timeout.
Needs to be tested manually, I don't think we have tests for cancelling act.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add a forceCancel context as field to the context, so we can force quit act by pressing ctrl+c twice? This could be the parent context of the context with a timeout.
That was the case before. The BackgroundContext
used as a fallback was cancelable as well. That would then cancel/stop the cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the case before. The BackgroundContext used as a fallback was cancelable as well. That would then cancel/stop the cleanup.
I want both a normal cancel with cleanup and additionally the opportunity to send another cancellation signal to abort cleanup and quit as soon as possible.
Previously we had one of them, but never both at the same time.
For Example this scenario
- Start a long running job
- cancel the normal context it while it executes the main stage
- A new context for the post executors is created
- This job has a long runnng post step
- cancel again this time force cancel context, because I don't want to wait otherwise I have to open the task manager and kill act.
- act stopped before hit 5min
Your CI system could just never fire the forcecancel context and you have an opt out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood and I'm fine with that.
I just thought that it did work before, as a second cancelation was respected by act (if I remember correctly).
… we were cancelled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do some tests before approving, but the code looks fine now for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected
breaking: docker container are removed after job exit
In github-act-runner I directly instantiate the RunContext, but I also want to make use of autoremove due to randomized job container name.