You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "imply-elliott (via GitHub)" <gi...@apache.org> on 2023/03/15 19:24:48 UTC
[GitHub] [druid] imply-elliott opened a new pull request, #13940: Clean up docker prior to steps that might expect a clean docker works…
imply-elliott opened a new pull request, #13940:
URL: https://github.com/apache/druid/pull/13940
#### We may sometimes run into dirty docker workspaces, clean up completely first just in case.
This PR has:
- [x] been self-reviewed.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-elliott commented on a diff in pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "imply-elliott (via GitHub)" <gi...@apache.org>.
imply-elliott commented on code in PR #13940:
URL: https://github.com/apache/druid/pull/13940#discussion_r1158897031
##########
.github/workflows/reusable-revised-its.yml:
##########
@@ -72,6 +72,15 @@ jobs:
path: ./**/target
key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-targets-${{ github.sha }}
+ - name: Clean up docker
Review Comment:
Maybe it would be more simple to just force stop and prune any containers matching the ones we know we'll be creating with the new ITs/`it.sh`?
The files not owned by root is a direct result of running in containers, files created in them will often be created as `root`, and thus the runner user (ubuntu in this case) doesn't have permission to clean them, and as a result, the GHA runner can't clean up properly and artifacts get left around. I threw it in as a good measure, since it doesn't really add any time or risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Clean up docker prior to steps that might expect a clean docker works… (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13940:
URL: https://github.com/apache/druid/pull/13940#issuecomment-1984824536
This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Clean up docker prior to steps that might expect a clean docker works… (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
URL: https://github.com/apache/druid/pull/13940
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-elliott commented on pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "imply-elliott (via GitHub)" <gi...@apache.org>.
imply-elliott commented on PR #13940:
URL: https://github.com/apache/druid/pull/13940#issuecomment-1497963983
> Tried a version of this in another PR. It cleaned up not only our Druid items, but a bunch of others that seemed to be published images cached locally. So, I think the approach to doing the prune is a bit too aggressive.
It is, however it's also quick and effective. Most of what it's cleaning up are pre-cached containers on the runners that we don't even use. So to take the time to engineer a solution to only prune stuff we care about belongs in the `it.sh` script, i agree. I'd say we use this heavy handed approach for now, as it's quick and effective for as long as the standard ITs need em.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-elliott commented on a diff in pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "imply-elliott (via GitHub)" <gi...@apache.org>.
imply-elliott commented on code in PR #13940:
URL: https://github.com/apache/druid/pull/13940#discussion_r1158897920
##########
.github/workflows/reusable-revised-its.yml:
##########
@@ -72,6 +72,15 @@ jobs:
path: ./**/target
key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-targets-${{ github.sha }}
+ - name: Clean up docker
Review Comment:
There's some options for running containers as a user, but they're not really available to us in the hosted runners, as we don't have control over the base image. We could install our own docker, running in the user env, but that'd cost both time and effort that simply cleaning up like this solves easily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on a diff in pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13940:
URL: https://github.com/apache/druid/pull/13940#discussion_r1139411707
##########
.github/workflows/reusable-revised-its.yml:
##########
@@ -72,6 +72,15 @@ jobs:
path: ./**/target
key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-targets-${{ github.sha }}
+ - name: Clean up docker
Review Comment:
For the "new ITs", the `it.sh` script itself should clean up. There is a bug that will be fixed in another PR, but once that is fixed, we want the script to do the work. We need it to work for developers, so we want it to work for builds also.
Given that, if there are any containers to stop in this script block, then that indicates a bug in our `it.sh` script. Thus, it would be good to
* Count the number of running containers.
* If more than 0:
* Echo the list of running containers.
* Do the force shutdown steps
Another question is the set of files not owned by `$USER_NAME`. What might cause that to occur? There are several possible cases:
* The cached files grabbed earlier are owned by another user. (This was the case on an older proprietary Jenkins setup.)
* The Docker containers created files owned by another user.
Both cases are bugs. Having mixed ownership will cause all manner of problems if it results in an inability to delete files, overwrite files or create directories. Thus, rather than simply clean up, we should log such files so we can fix them. This comment applies only to the "new IT" case: those run by `it.sh`. The old test may do all manner of horrible things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Clean up docker prior to steps that might expect a clean docker works… (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13940:
URL: https://github.com/apache/druid/pull/13940#issuecomment-1935131147
This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on a diff in pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13940:
URL: https://github.com/apache/druid/pull/13940#discussion_r1139429935
##########
.github/workflows/reusable-revised-its.yml:
##########
@@ -72,6 +72,15 @@ jobs:
path: ./**/target
key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-targets-${{ github.sha }}
+ - name: Clean up docker
+ run: |
+ USER_NAME=$(whoami)
+ echo "Force stopping all containers and pruning"
+ docker ps -aq | xargs -r docker rm -f
+ docker system prune -af --volumes
+ echo "Deleting files in workspace not owned by $USER_NAME"
+ sudo find $GITHUB_WORKSPACE ! -uid $(id -ru $USER_NAME) -delete -print
Review Comment:
Another thought is that this might want to move into `it.sh` as, say, `it.sh clean` so we can reuse it rather than doing copy/paste. There seems nothing here that uses GHA-specific commands: all are shell commands, aren't they? In PR #13877, there now commands for two of the tasks:
* `it.sh prune-containers` will print any still-running containers, then prune them.
* `it.sh prune-volumes` does what it says
Both were created/modified to mimic that lines you have here.
Also, `it.sh` has a `github` option to run the test. Cleanup should probably be done there if it is GHA-specific. Went ahead and added the above to the list of commands we already run.
This leaves only the owned-by-someone-else file cleanup, which is a case I don't fully understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #13940: Clean up docker prior to steps that might expect a clean docker works…
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13940:
URL: https://github.com/apache/druid/pull/13940#issuecomment-1474196021
Tried a version of this in another PR. It cleaned up not only our Druid items, but a bunch of others that seemed to be published images cached locally. So, I think the approach to doing the `prune` is a bit too aggressive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org