OLD: Add prototype Mark 6 selection script#44
Closed
wehimwich wants to merge 1 commit into
Closed
Conversation
dehorsley
reviewed
May 11, 2020
dehorsley
left a comment
dehorsley
left a comment
Member
There was a problem hiding this comment.
I know you're looking for more general feedback, but I made some comments inline that might help make the code a bit more Pythonic and easier to work with, as well as fix some issues with Python 3.
Generally it seems reasonable. I like how it handle changed files conflicts. I don't like the files polluting in the control directory, but I especially liked the idea of turning it into a more general tool and I think the file management stuff factors out fairly well into what I'm calling "fs profiles".
Here's an initial design I worked though today:
fs profiles
A profile is a set of control and proc files that can be swapped into of the operational directories.
- The sets of files names of two profiles are either identical or disjoint. This
restriction could be removed, but it does mean the action of applying a
profile is not commutative, so the "active profile" is the ordered
application of past profiles.
Profiles are stored subdirectories under /usr2/profiles
Eg the mk6_select cases "a" "b", "both" and "none" could be represented by profiles:
/usr2/profiles/mk6a/control/mk6ca.ctl
/usr2/profiles/mk6a/control/mk6cb.ctl (potentially blank)
/usr2/profiles/mk6a/control/mk6in.save_file
/usr2/profiles/mk6b/control/mk6ca.ctl (potentially blank)
/usr2/profiles/mk6b/control/mk6cb.ctl
/usr2/profiles/mk6b/control/mk6in.save_file
etc ...
Activating a profile can be done with the fsprofile command:
fsprofile mk6a
The logic around handing conflicts remains mostly the same as in mk6_select,
though the behaviour of the flags other than -f no longer make sense.
We could also add some flags to fsprofile:
-
--newtakes a name and a series of files, and copies them into the
profiles/<name>/{control,proc}directories. -
--updatetakes a profile name, and updates the files it contains with the
currently active versions
The design work well with the potential feature of fsserver of running multiple
instances of the fs. The server could take a profile name as an optional
argument. In this case, it would do an overlay mount of control, proc and log.
Separate profiles could be run at the same time.
Of course, mk6_select does a bit more than just the file copying thing, but
at least that component can be replaced by fsprofile and the mk6 specific
stuff implemented in the flags -i, -r, and -c can remain.
Comment on lines
+111
to
+127
| name=os.path.basename(os.path.splitext(sys.argv[0])[0]) | ||
| force=False | ||
| create=False | ||
| repair=False | ||
| mk6in=False | ||
|
|
||
| try: | ||
| options, remainder = getopt.getopt( | ||
| sys.argv[1:], | ||
| 'cfhir') | ||
|
|
||
| except getopt.GetoptError as err: | ||
| print("getopt ERROR:", err) | ||
| sys.exit("ry "+name+" -cfhir") | ||
|
|
||
| for opt,arg in options: | ||
| if opt == '-h': |
Member
There was a problem hiding this comment.
I recommend using the standard library's argparse module. It takes a lot of the drudgery out of arg parsing.
Comment on lines
+128
to
+277
| print(name+": select, display, or manage FS Mark 6 configuration") | ||
| print("") | ||
| print("Command line: "+name+" -cfhir a|b|both|none|") | ||
| print("") | ||
| print("Usage:") | ||
| print(" Once setup, this script can be used to change which Mark 6 is selected. e.g.:") | ||
| print(" "+name+" a") | ||
| print(" or:") | ||
| print(" "+name+" b") | ||
| print(" The FS must be restarted to use a changed configuration.") | ||
| print(" You can check what is currently selected with:") | ||
| print(" "+name) | ||
| print(" This script is always non-destructive as long as '-f' is not used.") | ||
| print(" Check 'Setup' below for configuration instructions.") | ||
| print(" Check 'Problem recovery' below for hints for dealing with problems.") | ||
| print(" A detailed description of the script follows.") | ||
| print("") | ||
| print("Arguments: a|b|both|none|") | ||
| print(" a|b|both|none - select that configuration if not already selected") | ||
| print(" All the required files must be present or any selection will be rejected.") | ||
| print(" There will be no change if the current configuration is 'other' unless '-f'") | ||
| print(" is used. See 'Problem recovery' below for advice before using '-f'.") | ||
| print(" The FS must be restarted to use a changed configuration.") | ||
| print(" With no argument, the current configuration is shown. If each file is not") | ||
| print(" consistent with 'a|b|none|both', the overall configuration is 'other'.") | ||
| print("") | ||
| # 12345678901234567890123456789012345678901234567890123456789012345678901234567890 | ||
| print("Options: -cfhir") | ||
| print(" Options '-c', '-i', and '-r' cannot be used together in any combination") | ||
| print(" -c - 'create' needed files in '/usr2/control'") | ||
| print(" 'mk6ca.ctl' and 'mk6cb.ctl' must exist already configured for Mark6a") | ||
| print(" and Mark 6b, respectively; if they are same or if one or both are") | ||
| print(" empty, this option is rejected.") | ||
| print(" 'mk6ca.ctl' will be copied to reference copy 'mk6ca.ctl.select'") | ||
| print(" 'mk6cb.ctl' will be copied to reference copy 'mk6cb.ctl.select'") | ||
| print(" The following files will be created:") | ||
| print(" mk6in.save_file - save_file input for mk6in procedure") | ||
| print(" mk6in.save_file.a.select - reference copy for 'a'") | ||
| print(" mk6in.save_file.b.select - reference copy for 'b'") | ||
| print(" mk6in.save_file.both.select - reference copy for 'both'") | ||
| print(" mk6in.save_file.none.select - reference copy for 'none'") | ||
| print(" All non-'.select' files will be in the 'both' configuration afterwards.") | ||
| print(" -f - 'force':") | ||
| print(" USE WITH CARE! This may delete/destroy information.") | ||
| print(" With a non-blank argument, this will override not changing if the") | ||
| print(" current configuration is 'other'; see 'Problem recovery' below for") | ||
| print(" advice before using in this case.") | ||
| print(" For '-c' this will overwrite existing 'mk6c?.ctl.select' files,") | ||
| print(" overwriting any changes that have been made to them, with the") | ||
| print(" corresponding 'mk6c?.ctl' files.") | ||
| print(" For '-c' and '-i', this will overwrite existing 'mk6in.save_file*'") | ||
| print(" files, returning them to their default state, overwriting any") | ||
| print(" changes that have been made.") | ||
| print(" For '-r' this will overwrite only existing 'mk6c?.ctl.select' files") | ||
| print(" with corresponding 'mk6c?.ctl' files that aren't empty") | ||
| print(" -h - 'help' - print this help information") | ||
| print(" -i - 'in' - like '-c' but will create 'mk6in.save_file*' files only") | ||
| print(" The 'mk6in.save_file' will be in the 'both' configuration afterwards.") | ||
| print(" -r - 'repair' - like '-c' but will only create 'mk6c?.ctl.select' files from") | ||
| print(" 'mk6c?.ctl' that are not empty, only overwriting if '-f' is used") | ||
| print(" This option is rejected if the 'mk6c?.ctl files are the same.") | ||
| print(" This option cannot be used with '-i' and/or '-m'") | ||
| print("") | ||
| print("Setup:") | ||
| print("To enable use of this script, the following steps are needed. Some may already") | ||
| print(" have been done, particularly the earlier ones.") | ||
| print("") | ||
| print("1. 'mk6c?.ctl' setup") | ||
| print(" a. Set 'mk6ca.ctl' for Mark 6a.") | ||
| print(" b. Set 'mk6cb.ctl' for Mark 6b.") | ||
| print("2. 'mk6in' setup") | ||
| print(" a. define aliases 'mark6a' and 'mark6b' in '/etc/hosts'") | ||
| print(" This must be done by 'root', instructions are not provided here.") | ||
| print(" b. set keyed login on Mark 6s for 'oper'. As 'oper' on FS computer:") | ||
| print(" i. Generate key") | ||
| print(" ssh-keygen") | ||
| print(" (press 'Enter' at all prompts)") | ||
| print(" ii. Copy key to 'oper' on Mark 6a:") | ||
| print(" ssh-copy-id oper@mark6a") | ||
| print(" (enter password when prompted)") | ||
| print(" iii. Copy key to 'oper' on Mark 6b:") | ||
| print(" ssh-copy-id oper@mark6b") | ||
| print(" (enter password when prompted)") | ||
| print(" iv. Copy 'mk6in' script to Mark 6a:") | ||
| print(" scp -p /usr2/fs/misc/mk6in.centos mark6a:bin/mk6in") | ||
| print(" (use /usr2/fs/misc/mk6in if Mark 6a is Debian)") | ||
| print(" v. Copy 'mk6in' script to Mark 6b:") | ||
| print(" scp -p /usr2/fs/misc/mk6in.centos mark6b:bin/mk6in") | ||
| print(" (use /usr2/fs/misc/mk6in if Mark 6b is Debian)") | ||
| print("3. 'station' SNAP procedure library setup (don't include leading spaces)") | ||
| print(" a. Create procedure 'mk6in_a' with contents:") | ||
| print(" sy=popen 'ssh oper\@mark6a bin/mk6in 2>&1' -n mk6ina &") | ||
| print(" b. Create procedure 'mk6in_b' with contents:") | ||
| print(" sy=popen 'ssh oper\@mark6b bin/mk6in 2>&1' -n mk6inb &") | ||
| print(" c. Create procedure 'mk6in_both' with contents:") | ||
| print(" mk6in_a") | ||
| print(" mk6in_b") | ||
| print(" d. Create procedure 'mk6in' with contents:") | ||
| print(" save_file=mk6in.save_file") | ||
| print("4. Create '.select' files") | ||
| print(" a. Verify no previous set-up") | ||
| print(" "+name) | ||
| print(" (the output should show seven missing files, none ending in '.ctl')") | ||
| print(" a. Create files") | ||
| print(" "+name+" -c") | ||
| print(" (no news is good news, now files are in the 'both' configuration") | ||
| print("5. Select Mark 6a initially") | ||
| print(" "+name+" a") | ||
| print("") | ||
| print("Problem recovery:") | ||
| print(" This script assumes that the '.select' files contain the reference copies of") | ||
| print(" the corresponding files. In general to make a change to the setup the") | ||
| print(" easiest approach is change the affected '.select' files and then reselect") | ||
| print(" the desired configuration.") | ||
| print("") | ||
| print(" Note that there is a fundamental asymmetry between the 'mk6c?.ctl*' files") | ||
| print(" and 'mk6in.save_files'. For the former, how they are initially defined") | ||
| print(" determines the contents of the '.select' files. For the latter, the default") | ||
| print(" content is generated by this script.") | ||
| print("") | ||
| print(" The most likely problem is having changed a 'mk6c?.ctl' file directly and") | ||
| print(" not having updated the corresponding '.select' file. The change may have") | ||
| print(" been made and after awhile forgotten. Then when trying to change the") | ||
| print(" configuration, the overall configuration is reported as 'other' with one or") | ||
| print(" both 'mk6c?.ctl' files' listed as: not 'a'/'b' and not empty, so 'other'.") | ||
| print("") | ||
| print(" If the changes to the 'mk6?.ctl' files(s) should be preserved, the easiest") | ||
| print(" recovery may be to use the '-rf' options, which will overwrite the") | ||
| print(" 'mk6c?.ctl.select' files with any corresponding non-'.select' files that") | ||
| print(" aren't empty:") | ||
| print(" "+name+" -rf") | ||
| print(" Changing configuration should then work if tried again.") | ||
| print("") | ||
| print(" Alternatively, if the 'mk6c?.ctl' (and/or the 'mk6in.save_file') have been") | ||
| print(" incorrectly changed, you can return to the setup in the '.select' files'") | ||
| print(" by using the '-f' option with configuration you want, e.g., for 'a':") | ||
| print(" "+name+" -f a") | ||
| print(" Note that this also overwrites the 'mk6in.save_file' file as well.") | ||
| print("") | ||
| print(" If the 'mk6in.save_file' has changed and the new version should become") | ||
| print(" permanent, the easiest thing to do may be to copy it over the one it") | ||
| print(" should replace, for example it update the '.select' file for 'a':") | ||
| print(" cd /usr2/control") | ||
| print(" cp mk6in.save_file mk6in.save_file.a.select") | ||
| print("") | ||
| print(" Alternatively, it the mk6in,save_file' should not be preserved, it can be") | ||
| print(" overwritten by the '.select' version by using the '-f' option with") | ||
| print(" configuration you want, e.g., for 'a':") | ||
| print(" "+name+" -f a") | ||
| print(" Note that this also overwrites the 'mk6c?.ctl' files as well.") |
Member
There was a problem hiding this comment.
a multiline string and .format(name) will make this much easier to read and edit.
Comment on lines
+335
to
+348
| okay = True | ||
| okay = there("mk6ca.ctl") and okay | ||
| okay = there("mk6ca.ctl.select") and okay | ||
| okay = there("mk6cb.ctl") and okay | ||
| okay = there("mk6cb.ctl.select") and okay | ||
| okay = there("mk6in.save_file") and okay | ||
| okay = there("mk6in.save_file.a.select") and okay | ||
| okay = there("mk6in.save_file.b.select") and okay | ||
| okay = there("mk6in.save_file.both.select") and okay | ||
| okay = there("mk6in.save_file.none.select") and okay | ||
|
|
||
| if not okay: | ||
| sys.exit("Not all required files are present, see above") | ||
|
|
Member
There was a problem hiding this comment.
Perhaps:
required_files = [
"mk6ca.ctl",
"mk6ca.ctl.select",
"mk6cb.ctl",
"mk6cb.ctl.select",
"mk6in.save_file",
"mk6in.save_file.a.select",
"mk6in.save_file.b.select",
"mk6in.save_file.both.select",
"mk6in.save_file.none.select",
]
if not all(there(f) for f in required_files):
sys.exit("Not all required files are present, see above")
| @@ -0,0 +1,424 @@ | |||
| #!/usr/bin/python | |||
Member
There was a problem hiding this comment.
Should we target just Python 3?
If not, we should at least try to make it work on both. Adding this here will help
from __future__ import print_function, division
|
|
||
| def there(path): | ||
| if not os.path.exists(path): | ||
| print path,"doesn't exist" |
|
|
||
| def same(name1,name2): | ||
| if not subprocess.call(["cmp","-s",name1,name2]): | ||
| print "Error: "+name1+" is the same as "+name2 |
Comment on lines
+282
to
+292
| if repair or mk6in: | ||
| sys.exit("None of '-c', '-i', or '-r' can be used together.") | ||
| create = True | ||
| elif opt == '-i': | ||
| if create or repair: | ||
| sys.exit("None of '-c', '-i', or '-r' can be used together.") | ||
| mk6in = True | ||
| elif opt == '-r': | ||
| if create or mk6in: | ||
| sys.exit("None of '-c', '-i', or '-r' can be used together.") | ||
| repair = True |
Member
There was a problem hiding this comment.
You can do this with argparse something like this
parser = argparse.ArgumentParser(...)
parser.add_argument('--force', '-f', action='store_true')
group = parser.add_mutually_exclusive_group()
group.add_argument('--create', '-c', action='store_true')
group.add_argument('--repair', '-r', action='store_true')
group.add_argument('--in', '-i', action='store_true')
| return False | ||
|
|
||
| try: | ||
| os.write(fd,line) |
Member
There was a problem hiding this comment.
This will throw a TypeError in Python 3 as line is presumably a string not a bytes.
Better to use the built-in open (which you can pass a file descriptor if you really need to use os.open)
| else: | ||
| flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY | ||
| try: | ||
| fd = os.open(dst,flags,0o666) |
Member
There was a problem hiding this comment.
Is os.open needed here (rather than just open)? Ie. do you really need to set the permissions?
| print(" a. Create procedure 'mk6in_a' with contents:") | ||
| print(" sy=popen 'ssh oper\@mark6a bin/mk6in 2>&1' -n mk6ina &") | ||
| print(" b. Create procedure 'mk6in_b' with contents:") | ||
| print(" sy=popen 'ssh oper\@mark6b bin/mk6in 2>&1' -n mk6inb &") |
Member
There was a problem hiding this comment.
Is the \@ in the actual command? Python will try read it as an escape sequence and with warnings on this gives a DeprecationWarning.
Better to use a raw string, ie.
r" sy=popen 'ssh oper\@mark6b bin/mk6in 2>&1' -n mk6inb &"
wehimwich
changed the title
Member
Author
|
This PR was inadvertently closed when the default branch was changed to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a prototype for a script to allow operators to easily pick which Mark 6 (a or b) would be in use, by changing the files in /usr2/control (then there needs to be a FS restart). The files involved are:
The last is read by the SNAP procedure mk6in to execute the right command (via the save_file command) to display the Mark 6 input data rates. The script uses a set of .select files to store the configurations needed for each case: a, b, both, and none:
Note that there is no need for other forms of mk6c?.ctl in this situation since the script makes a .ctl file empty to "deconfigure" the corresponding Mark 6.
The thought is that this might be easier than editing the files by hand. For example the user just types:
to switch to Mark6b, overwriting the non-.select files appropriately. The script enforces integrity checks to make sure that the non-.select files are already consistent with a configuration of existing .select files so that information won't be lost by mindless changing the configuration (for example some one changed a .ctl file without updating the corresponding .select). Nothing can be lost unless the user specifies -f, which might be needed in some unusual cases. There are a few other options for setup (-c) and repair (-i and -r). With no arguments, the script just reports the current configuration. So hopefully easy to use for typical cases, but it provides warnings and tools to help with problems.
I don't expect anyone to read my pidgin Python implementation (but I am sure I would learn something if I got feedback). The easiest thing to read might be the -h output. My interest is mainly to get feedback from @dehorsley and @jfhquick about whether this might be a useful step toward a more general tool for switching FS configurations. I am also wondering if it might be better to change the .select files by:
Removing the .select extensions (for mk6c?.ctl.select, it would be fairly natural to substitute .a and .b so they could be unique), less garish I guess.
Make them them hidden by adding a dot as the first character of the names, thus reducing clutter in /usr2/control.
The script name, mk6_select is a little awkward, too long and hitting tab after the 6 conflicts with mk6cn.
Note that this facility is different than the (planned) active_mk6s SNAP command that will allow a schedule to select between already configured Mark 6s on the fly.