ENH: add permission handling to ioc-deploy #195
Conversation
ZLLentz
changed the title
|
Marking as ready for review, will ask for reviewers after the holiday weekend |
|
I'm going to do a tiny bit more cleanup here based on self-review |
|
I'm happy with how this behaves now. You'll still be able to undo a release if you'd like to but it won't be possible to do this accidentally now. I think we could discuss a bit about small permutations to this- should we be more protective, and require cross-compiles to unlock the write permissions? Should we mess with the .git directory? Maybe if people have opinions on this in the future we can revisit. |
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
I'm interested in what happens when two different users try to undo each others write permission settings here. I want to give this a closer look but I have some questions to start
scripts/ioc_deploy.py
Outdated
| We will also add write permissions to the top-level directory. | ||
| allow_write=False involves removing the "w" permissions | ||
| from all files, and from directories that contain untracked files. |
There was a problem hiding this comment.
Maybe I'm just slow here, but what's the difference between the files affected when allow_write=True and allow_write=False?
allow_write=True--> recursively applies to everything insidedeploy_direxcept for ".git"allow_write=False--> everything tracked by git and their subdirectories recursively, excepting ".git", but then also things not tracked by git?
Is there something missing from the union of "things tracked by git" and "things not tracked by git"?
There was a problem hiding this comment.
Maybe I haven't internalized what the directory structure here looks like
There was a problem hiding this comment.
This is a good question that might prompt some edits
I think it's normal to expect that the set of files affected by these two is the same- maybe I should edit so that it is.
The perspective here is something like "there are some directory objects that should never be write-protected because the group wants to be ably to casually recompile on a new arch", but there is no such constraint in the other direction.
allow_write=True-> no constraints, (re-)implemented separately in the simplest way without constraint logicallow_write=False-> please allow me to be able to log into the rocky9 build host and create e.g.bin/rhel9_x86_64even if I've already built/protected once e.g.bin/rhel7_x86_64
When implementing the False variant with the above goal, I decided that a simple way to do this is to only write protect directories that contain files generated from make, e.g. those not tracked by git.
Cons of this approach:
- Confusing code
- git safedir thing will pop up when you try to write protect someone else's release
Alternates I considered that maybe you think are better:
- Make it simpler: full write permissions or no write permissions for the entire tree, require using the script to unlock for a new make
- Go for the minimal number of allowed directories: look for folders with OS-names in them, allow only those directories to stay unprotected
I ignored .git because it seemed weird to be messing with it but maybe it's better to mess with it for simplicity's sake
Open to all thoughts and suggestions here
There was a problem hiding this comment.
Easy agree: I think ignoring .git makes perfect sense
Ok so the difference is that when we set allow_write=False, we do not write protect the build artifact directories generated by make build
It looks like the code is gathering build_dir_paths, then also write-protecting them. Do we actually want to make these writable? (are these the bin/rhel_xx_xx directories?)
There was a problem hiding this comment.
The make generates files and folders such as:
bin/rhel7_x86_64/adsIoc
Which is the main executable that needs to continue to exist in this example
The intention is to write protect bin/rhel7_x86_64 without write protecting bin:
── [drwxrwxr-x] bin
│ └── [dr-xr-xr-x] rhel7-x86_64
│ └── [-r-xr-xr-x] adsIoc
So that we can't accidentally remove the folder and delete adsIoc, but we can create a parallel folder in bin for a new arch.
|
The final thing I need to do is to follow Tyler's suggestion of running a real IOC. I'm going to stand up a simple PV notepad test IOC starting from https://github.com/pcdshub/ioc-tst-pvNotepad. I'll go all the way through iocmanager to verify everything looks normal, then clean up my iocData mess. If this doesn't work, I'll investigate why and see if any specific directories need to be writable. |
|
It seemed to work just fine. The PVs are online in the test subnet e.g. |
|
This is ready pending re-review of my commit storm, or pending if we want to try to test or damage |
|
As suspected, I cannot change permissions to this directory: me:~/src/engineering_tools/scripts(enh_ioc_deploy_perms +)$ ./ioc-deploy -n ioc-tst-pvNotepad -r R0.2.0 --allow-write True
INFO ioc-deploy: ioc-deploy: checking inputs
INFO ioc-deploy: Allowing writes to /reg/g/pcds/epics/ioc/tst/pvNotepad/R0.2.0
Confirm target? yes/true or no/false
yes
ERROR ioc-deploy: [Errno 1] Operation not permitted: '/reg/g/pcds/epics/ioc/tst/pvNotepad/R0.2.0'
ERROR ioc-deploy: ioc-deploy errored outI think this error is simple enough to catch and let the user know that there's a permission conflict. If the chmod fails maybe we check who the original owner is and report that the user should contact that person. |
|
I'm going to try to sudo chown my test deployment to Robert to self-test the new messages I just added. |
|
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
I'm pretty happy with this (maybe now that I understand it more fully). I have a bit of phrasing nitpicks, not I'm not too worried.
I do think that it might make more sense to have allow-writes be a subcommand of ioc-deploy, such that you'd invoke it like:
$ ioc-deploy update_perms {RW, RO} -n ioc-tst-thing -r R1.0.0 # runs update perms mode
$ ioc-deploy -n ioc-tst-thing -r R1.0.0 # normal deploy processIt does seem like tacking on --allow-write true as an optional arg would first deploy the IOC, then adjust its permissions.
if that's too much work or you're sick of fussing with this, I think it's totally skippable. This will be useful as it is now
This is good feedback and I think it's pretty easy to implement |
|
argparse is a bit restricting as soon as you want to do anything interesting- it was more annoying than expected to get subcommands to behave as I wanted, but I think I addressed everything in a reasonable or close-to-reasonable way |
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
Yea adding a subparser is always finicky in argparse. We have some rather complicated boilerplate in other places for it. (looking at you atef)
No more quips from me. Well done here 👍
| return subprocess.run(cmd, **kwds) | ||
|
|
||
|
|
||
| def print_help_text_for_readme(): |
There was a problem hiding this comment.
Amazing ✨ I'd advocate for this to be its own script in engineering tools but is anyone else writing thorough argparse docstrings?
There was a problem hiding this comment.
You're right, I considered making a standalone script that just stdin -> stdout but I couldn't conceptualize it quickly and didn't feel like increasing my scope.
The idea would be something like
ioc-deploy --help | readme-ify
I think using help text output in general for the readme is really common even if not everything uses argparse
There was a problem hiding this comment.
I'm going to make this an issue
| Otherwise, the first example here is interpretted as if -p was never passed, | ||
| which could be confusing. |
There was a problem hiding this comment.
This is truly an extra mile QOL addition, but I can't argue with the results. I think normally sub-commands just don't work if you don't specify it as the first argument, and I'd have left it at that haha
3 participants
Description
Changes
ioc-deployupdate-perms)--path-override,-p)--helptext outputDetails
When we deploy a release, we'll also apply write protection. Write protection is defined as:
This was previously more complicated, but now we'll simplify it to make the rules understandable. Users will need to restore the write permissions if they wish to rebuild on a new architecture.
Write permissions can be added to or removed from existing releases with invocations like:
ioc-deploy update-perms rw -p path/to/my/releaseioc-deploy update-perms ro -p path/to/my/releaseAllowing writes is the same as allowing user and group writes to all files and folders.
Motivation and Context
https://jira.slac.stanford.edu/browse/ECS-6122
closes #193
How Has This Been Tested?
make distclean,make clean, andrm -rffail to remove built files after running the deploy script.Where Has This Been Documented?
Updated the readme