Runner Entrypoint: fix daemon.json #1409
Conversation
|
Having the same issue... Is there a way to provide daemon.json that is custom to regular summerwind/actions-runner-dind:latest |
|
@UrosCvijan How about modifying |
This overhaul turns it into a shellcheck valid script with explicit error handling for all possible situations I could think of. This change takes #1409 into account and things can be merged in any order. There are a few important changes here to the logic: - The wait logic for checking if docker comes up was fundamentally flawed because it checks for the PID. Docker will always come up and thus become visible in the process list, just to immediately die when it encounters an issue, after which supervisor starts it again. This means that our check so far is flaky due to the `sleep 1` it might encounter a PID, or it might not, and the existence of the PID does not mean anything. The `docker ps` check we have in the `entrypoint.sh` script does not suffer from this as it checks for a feature of docker and not a PID. I thus entirely removed the PID check, and instead I am handing things over to our `entrypoint.sh` script by setting the environment variables correctly. - This change has an influence on the `docker0` interface MTU configuration, because the interface might or might not exist after we started docker. Hence, I changed this to a time boxed loop that tries for one minute to set up the interface's MTU. In case the command fails we log an error and continue with the run. - I changed the entire MTU handling by validating its value before configuring it, logging an error and continuing without if it is set incorrectly. This ensures that we are not going to send our users on a bug hunt. - The way we started supervisord did not make much sense to me. It sends itself into the background automatically, there is no need for us to do so with Bash. The decision to not fail on errors but continue is a deliberate choice, because I believe that running a build is more important than having a perfectly configured system. However, this strategy might also hide issues for all users who are not properly checking their logs. It also makes testing harder. Hence, we could change all error conditions from graceful to panicking. We should then align the exit codes across `startup.sh` and `entrypoint.sh` to ensure that every possible error condition has its own unique error code for easy debugging.
|
@mumoshu Aha, sorry for late reply. My idea was not to touch the "core" of everything that was used to build the docker image. As later something might be added to the startup.sh so I would not touch any of those but just add my daemon, basically do the seamless upgrades. But if there wont be a change like this, I guess this will have to be the way. |
|
@UrosCvijan Thanks for clarifying! I hear you- probably we should merge this and after that you'd be able to provide your own daemon.json in your custom runner image's Dockerfile. Does that sound good to you? @UrosCvijan @Vladyslav-Miletskyi The only reason I was a bit hesitant to improve startup.sh like this is that now you have another vector to break ARC so we'd need to improve our already very lengthy bug report form to also include a question like "Paste your custom docker daemon.json if any". If the only way you can customize it was by modifying start.sh, almost certainly the user is advanced enough so that they don't need basic user support so I won't include such question in the bug report form. Perhaps you'd think it's better to merge this and enhance the bug report form, rather than not merging this at all? If so, I'd go that way anyway! |
|
@mumoshu Yeah, that is what i currently did with the base docker:dind image. As I don't want to "break" the future upgrades and flow, this way, I don't touch any of the mechanics of the startup.sh or entrypoint.sh, I can just use your image and add stuff I need, like packages new deamon.json etc.. |
Do not owerwrite daemon.json if it already exists. Usage: custom images, which are using public image as source.
Co-authored-by: Callum Tait <15716903+toast-gear@users.noreply.github.com>
|
@UrosCvijan Thanks for confirming! Got it. We'll do our best to maintain it :) But beware that we may or may not unexpectedly break this as it currently lacks a unit test. It would be great if you could submit a PR to add that. I remember we had a few tests for entrypoint.sh in https://github.com/actions-runner-controller/actions-runner-controller/tree/master/test/entrypoint. It would be great if you could write something similar for startup.sh. |
|
At least this seems to not break existing behavior. I've verified that by running a few tests with the updated image locally. |
This overhaul turns it into a shellcheck valid script with explicit error handling for all possible situations I could think of. This change takes actions/actions-runner-controller#1409 into account and things can be merged in any order. There are a few important changes here to the logic: - The wait logic for checking if docker comes up was fundamentally flawed because it checks for the PID. Docker will always come up and thus become visible in the process list, just to immediately die when it encounters an issue, after which supervisor starts it again. This means that our check so far is flaky due to the `sleep 1` it might encounter a PID, or it might not, and the existence of the PID does not mean anything. The `docker ps` check we have in the `entrypoint.sh` script does not suffer from this as it checks for a feature of docker and not a PID. I thus entirely removed the PID check, and instead I am handing things over to our `entrypoint.sh` script by setting the environment variables correctly. - This change has an influence on the `docker0` interface MTU configuration, because the interface might or might not exist after we started docker. Hence, I changed this to a time boxed loop that tries for one minute to set up the interface's MTU. In case the command fails we log an error and continue with the run. - I changed the entire MTU handling by validating its value before configuring it, logging an error and continuing without if it is set incorrectly. This ensures that we are not going to send our users on a bug hunt. - The way we started supervisord did not make much sense to me. It sends itself into the background automatically, there is no need for us to do so with Bash. The decision to not fail on errors but continue is a deliberate choice, because I believe that running a build is more important than having a perfectly configured system. However, this strategy might also hide issues for all users who are not properly checking their logs. It also makes testing harder. Hence, we could change all error conditions from graceful to panicking. We should then align the exit codes across `startup.sh` and `entrypoint.sh` to ensure that every possible error condition has its own unique error code for easy debugging.
This overhaul turns it into a shellcheck valid script with explicit error handling for all possible situations I could think of. This change takes actions/actions-runner-controller#1409 into account and things can be merged in any order. There are a few important changes here to the logic: - The wait logic for checking if docker comes up was fundamentally flawed because it checks for the PID. Docker will always come up and thus become visible in the process list, just to immediately die when it encounters an issue, after which supervisor starts it again. This means that our check so far is flaky due to the `sleep 1` it might encounter a PID, or it might not, and the existence of the PID does not mean anything. The `docker ps` check we have in the `entrypoint.sh` script does not suffer from this as it checks for a feature of docker and not a PID. I thus entirely removed the PID check, and instead I am handing things over to our `entrypoint.sh` script by setting the environment variables correctly. - This change has an influence on the `docker0` interface MTU configuration, because the interface might or might not exist after we started docker. Hence, I changed this to a time boxed loop that tries for one minute to set up the interface's MTU. In case the command fails we log an error and continue with the run. - I changed the entire MTU handling by validating its value before configuring it, logging an error and continuing without if it is set incorrectly. This ensures that we are not going to send our users on a bug hunt. - The way we started supervisord did not make much sense to me. It sends itself into the background automatically, there is no need for us to do so with Bash. The decision to not fail on errors but continue is a deliberate choice, because I believe that running a build is more important than having a perfectly configured system. However, this strategy might also hide issues for all users who are not properly checking their logs. It also makes testing harder. Hence, we could change all error conditions from graceful to panicking. We should then align the exit codes across `startup.sh` and `entrypoint.sh` to ensure that every possible error condition has its own unique error code for easy debugging.
Do not overwrite daemon.json if it already exists.
Usage: custom images, which are using a public image as source.