-
Notifications
You must be signed in to change notification settings - Fork 20
Create complete recipes for pip-compile #92
Conversation
compose/Makefile
Outdated
|
||
reset-permissions-to-user: | ||
@echo "Running recipe 'reset-permissions-to-user' and setting ownership to ${CALLER_UID} -- if eq 1000 this should be your USER" | ||
find ../src/ -user root -path "*/requirements/*" -exec sudo chmod 644 '{}' + |
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.
Would this require a sudo
password when this make rule is executed?
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.
@replaceafill that's right, yep. If it needs to change the permissions. You can sudo
the recipe, or supply it during its execution. The root:root
ownership happens when compile is run via the containers on docker-compose (maybe not in all configurations?)
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.
It'd be nice if we never changed the permissions in the first place. In compile-requirements-*, we can use docker-compose run --user=${CALLER_UID}
instead of --user=root
. That way we wouldn't need to reset permissions? Well I guess users already running am where we have already messed up with their permissions will have to reset them one last time.
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 have replaced this recipe with one that does the work in the container instead which immediately I think is better than asking the Makeflle to do something at the system level. Regarding the compile step then using the calling ID would be nice, but "tmpfs:/pip-cache:rw"
seems to needs root access from my investigations today, and docker-compose doesn't yet support "tmpfs:/pip-cache:rw,uid=${myid}"
which is a shame. But maybe you see an alternative? I did spend some significant time on this thinking it was possible but I didn't get there. So perhaps what we've got is good enough for now? I feel it one-step further on than previous.
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 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.
But maybe you see an alternative?
Actually this works for me, in case you are still interested (but we can also tackle later):
docker-compose run --workdir /src/requirements \
-e XDG_CACHE_HOME=/tmp/pip-cache \
--rm \
--no-deps \
--user=$(CALLER_UID) \
--entrypoint bash archivematica-storage-service \
-c "make clean && make all"
I guess that tmpfs
makes things faster but it doesn't seem to be significant.
See 7ad4017.
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.
@sevein 7ad4017 works. On the face of it, quite superior in how much code it removes - but I think we may be approaching this with different intentions.
When you say works, are the results the same? For example, once I've used the updated recipe, my .txt
permissions are ${USER}:root
.
My approach to this PR has been to minimize how invasive it is - granted to differing quality? success?
So:
Revision 1: Uses system level calls to reset ownership to ${USER}:${USER}
. (Captures all users running docker in the docker group, or all users running docker as sudo) but it doesn't quite feel right to me either.
Revision 2: The compromise is that, users must run docker with their user in the group, but the ownership is reset to what the user expects. (there's no explaining to do to anyone / i leave things how I found them). i.e. ${USER}:${USER}
.
Revision 3: 7ad4017 is fine, but as mentioned, it does seem to leave that slightly niggly group ownership behind, which doesn't sit right personally, but that's because I know we have a simple command to put things back to how we found them.
Given that, at the very least, I'd like to keep the reset recipes. But if we do that, then we might as well keep the tmpfs
too which does make things slightly quicker like you say.
Which would leave me to change GID back to UID - that was me misunderstanding some terminology.
Does sticking with revision 2 work with the variable change, or do you need 7ad4017 or another variant of that which can reset group? I can't see a way to reset group but maybe you have another idea still given the problem statement?
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.
When you say works, are the results the same? For example, once I've used the updated recipe, my .txt permissions are ${USER}:root.
Do you think ${USER}:root
is a consequence of your sudo docker
workflow? It doesn't happen to me. But I can see how you'd want the reset recipes, that's fine.
But if we do that, then we might as well keep the tmpfs too which does make things slightly quicker like you say.
Cool, that makes sense.
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.
Oh I see what's going on with ${USER}:root
. When we use docker-compose --user=1000
we're only telling Docker the uid we want to use but the gid is set to zero automatically. If we did --user=$(CALLER_UID):$(CALLER_GID)
then it would be set properly.
$ docker-compose run --no-deps --entrypoint=id --rm --user=1000 archivematica-storage-service -g
0
$ docker-compose run --no-deps --entrypoint=id --rm --user=1000:1000 archivematica-storage-service -g
1000
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.
If we did --user=$(CALLER_UID):$(CALLER_GID) then it would be set properly.
That sounds good, Let me give it a whirl and get the PR updated.
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.
Thanks for doing this. I think that there is a better way to share code using the MAKE variable. Here's an example:
action-1:
@echo "action-1 executed"
@$(MAKE) --no-print-directory post-shared-action
action-2:
@echo "action-2 executed"
@$(MAKE) --no-print-directory post-shared-action
post-shared-action:
@echo "post-shared-action executed"
That way we wouldn't need both compile-requirements-am
and compile-am-requirements
.
compose/Makefile
Outdated
docker-compose run --workdir /src/MCPClient/requirements --volume "tmpfs:/pip-cache:rw" -e XDG_CACHE_HOME=/pip-cache --rm --no-deps --user=root --entrypoint make archivematica-mcp-client all | ||
docker-compose run --workdir /src/dashboard/src/requirements --volume "tmpfs:/pip-cache:rw" -e XDG_CACHE_HOME=/pip-cache --rm --no-deps --user=root --entrypoint make archivematica-dashboard all | ||
docker-compose run --workdir /src/requirements --volume "tmpfs:/pip-cache:rw" -e XDG_CACHE_HOME=/pip-cache --rm --no-deps --user=root --entrypoint make archivematica-storage-service all | ||
CALLER_GID=$(shell id -u) |
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.
Should we use both uid and gid? It's definitely possible to have different values, e.g. the staff
group (gid 20) is macOS is usually the main group of users with different uids.
CALLER_UID=$(shell id -u)
CALLER_GID=$(shell id -g)
This commit introduces a storage service command for pip-compile. For both the Storage Service and Archivematica, 'clean' and compile 'all' is run so that they are compiled fresh every run. This costs a little extra time but ensures we're always up-to-date with what pip-tools tells us. Ownership of requirements output from Docker are set to UID:GID of the caller. Co-authored-by: Jesús García Crespo <[email protected]>
c88f15b
to
204d121
Compare
@sevein I see you've approved this, but there was one last point of note. The |
Perhaps that's because you have docker set as your primary group?
|
This commit introduces a storage service command for pip-compile and
ensures that for both Archivematica and the Storage Service
permissions are updated so that they remain usable by the caller,
i.e. permissions are reset to 0644 and ownership reset to the
user-space UID for the current login and not root for sudo callers.
For usability the two commands to test are:
reset-permissions-to-user-am
reset-permissions-to-user-ss
With
-am
and-ss
at the end of the command, they are easier to exchange vs. mid-string.The last three commits today (Aug 5) should be the ones reviewed here and will make up the re-base.
One additional point of note is that previously I thought
all
meant a clean and compile. I hadn't studied the compile Makefiles too carefully. Without a clean step this has tripped us up a few times, either testing this particular PR, or other related compile PRs. I've therefore implemented a clean-and-compile approach to this which, while it takes longer, is better practice in the long-run. I'd like us to stick with those given the option.Connected to archivematica/Issues#1039
Requires artefactual/archivematica-storage-service#540 (see notes)