Last modified: 2014-05-27 15:14:59 UTC
While documenting some of these scripts I noticed two things: * Not all of them log a message to the SAL[1] * They duplicate ddsh/rsync parameters and may not be consistent (at least it is not obvious why they wouldn't be the same). Proposal: 1) Update these to use sync-common-file internally (just like sync-file and sync-dir already do). That way they automatically get the following (if they didn't have it already, and if they did, it will now be centralized): - various checks such as for SSH and ddsh - notify dologmsg - notify deploy2graphite * sync-dblist * sync-docroot * sync-wikiversions 2) Update sync-apache to include a notify dologmsg [1] http://wikitech.wikimedia.org/view/Server_admin_log
Also, sync-file and sync-dir are not in version control for some reason. and sync-dir has various silly assumptions about file. For one, it tries to do php-l on it, which is pointless since the line above asserted it was a directory. And then it uses sync-common-file, which should probably be renamed to sync-common-path since it does either files or directories.
Several issues in one ticket... not good. Might be a ticket for a sprint (like Amsterdam), like one of platformeng and one of ops working on it together. 1) Adding to version control. 2) Update the scripts.
Tim's changes in I9d315c4cb9 fix linting.
(In reply to comment #3) > Tim's changes in I9d315c4cb9 fix linting. That would be Ic40757f3f04 instead.
So, checklist then: For each of the sync scrips, ensure: [ ] In version control [ ] Have checks for SSH_AUTH_SOCK and dsh etc. (probably best to centralise in a common function) [ ] Have checks for existence of directory or file [ ] Lint PHP [ ] Have consistency in parameters to dsh,mwdeploy,rsync (perhaps centralise in a common function) [ ] notify dologmsg [ ] notify deploy2graphite I'm assigning to myself. I'd like to do this clean up once Tim's scap-to-tin branch[1] is ready and merged. [1] https://gerrit.wikimedia.org/r/#/q/project:operations/puppet+topic:scap-to-tin,n,z
(In reply to comment #5) > So, checklist then: > > For each of the sync scrips, ensure: > > [X] In version control https://gerrit.wikimedia.org/r/#/c/57854/ > [ ] Have checks for SSH_AUTH_SOCK and dsh etc. > (probably best to centralise in a common function) > [ ] Have checks for existence of directory or file > [ ] Lint PHP > [X] Have consistency in parameters to dsh,mwdeploy,rsync > (perhaps centralise in a common function) https://gerrit.wikimedia.org/r/#/c/57890/ > [ ] notify dologmsg > [ ] notify deploy2graphite > > I'm assigning to myself. I'd like to do this clean up once Tim's scap-to-tin > branch[1] is ready and merged. > > > [1] > https://gerrit.wikimedia.org/r/#/q/project:operations/puppet+topic:scap-to- > tin,n,z
I think the checklist from comment 5 is pretty far along. If someone could reply to comment 5 with an updated list of what's been done, that'd be nice.
Krinkle: Are you still actively working on this/think of it as your task to take? If not, go ahead an unassign yourself.
Go ahead :)
(See also comment #5, comment #6) For each of the sync scrips, ensure: [OK] In version control [ ] Checks SSH_AUTH_SOCK [ ] Checks dsh [ ] Checks existence of file or directory [ ] Recursively lint php files [ ] Have consistency in parameters to dsh and rsync [ ] notify dologmsg [ ] notify deploy2graphite * scap, sync-common-file and sync-wikiversions check SSH_AUTH_SOCK * scap-1skins, scap-2, sync-common-file, sync-dblist, sync-docroot and sync-wikiversions call rsync with parameters * scap, sync-common-file, sync-dblist, sync-docroot and sync-wikiversions call dologmsg * sync-dir and sync-file use sync-common-file internally In I9d315c4cb937389fb8 Tim centralised a lot of information already (though not everything, it did make it a lot easier). That change introduced a similar pattern as Ori proposed in I702777b4ada8706b653, namely "/usr/local/lib/mw-deployment-vars.sh" which sets the variables. Depending on how much variance is needed between all the various scripts, I think it should be possible to do all of the above in 1 common function that takes only a path as argument. Pretty much what sync-common-file is now (except that it is oddly named "-file", where in fact "sync-common-path" would be a better name). Current state: ( scap, * loads vars * checks SSH_AUTH_SOCK * lint-php * !! does not check dsh * dsh -F30 -cM -o -oSetupTimeout=10 * > sync-common > scap-1 > scap-2 * rsync -a --delete --exclude=svn,git --no-perms * notifies dologmsg * notifies deploy2graphite scap-1skins scap-1 scap-2 * used by scap sync-common > scap-1 sync-common-all > scap sync-common-file * loads vars * checks SSH_AUTH_SOCK * checks existence * checks dsh * !! does not lint-php, but callers (sync-dir, sync-file) do * notifies dologmsg, deploy2graphite * dsh -cM -o -oSetupTimeout=30 -F30 * rsync -a --delete --exclude=svn,git --exclude=cache/l10n --no-perms * notifies dologmsg * notifies deploy2graphite sync-dblist * loads vars * !! does not check SSH_AUTH_SOCK * !! does not check dsh * dsh -cM -o -oSetupTimeout=30 -F30 * rsync -a # doesn't pass --delete, --no-perms and --exclude # though --exclude does seem (harmlessly) redundant in this case * notifies dologmsg * !! does not notify deploy2graphite sync-docroot * loads vars * !! does not check SSH_AUTH_SOCK * !! does not check dsh * !! does not lint-php * dsh -cM -o -oSetupTimeout=30 -F30 * rsync -a --no-perms # doesn't pass --delete * notifies dologmsg * notifies deploy2graphite sync-dir * loads vars * checks existence * lint-php * > sync-common-file sync-file * loads vars * checks existence * lint-php * > sync-common-file sync-wikiversions * loads vars * checks SSH_AUTH_SOCK * checks dsh * dsh -cM -o -oSetupTimeout=30 -F30 * rsync -l * notifies dologmsg * notifies deploy2graphite )
Change 57890 merged by Tim Starling: Set common rsync and dsh parameters in mw-deployment-vars https://gerrit.wikimedia.org/r/57890
I think this bug has aged a lot over the past 6 months. The good: I think much of this has been done by Bryan in the migration of scap (and sub/helper commands) from php/shell to python. The bad: It's overly time consuming for me at this point to go through each of these checklists and see what's still needed for scap(py). The basic checklist I believe is complete though: [ ] In version control [ ] Checks SSH_AUTH_SOCK [ ] Checks dsh [ ] Checks existence of file or directory [ ] Recursively lint php files [ ] Have consistency in parameters to dsh and rsync [ ] notify dologmsg [ ] notify deploy2graphite Bryan: Correct me if I'm wrong on those assertions. Unless I hear a "you're wrong" I'll close this bug, but at this point I'd ideally prefer specific actionable bug reports for specific parts of scap/our deployment tooling instead of making ever more sub-checklists here.
(In reply to Greg Grossmeier from comment #13) > Unless I hear a "you're wrong" I'll close this bug, but at this point I'd > ideally prefer specific actionable bug reports for specific parts of > scap/our deployment tooling instead of making ever more sub-checklists here. Today some of Krinkle's analysis is dated due to the scap port to python, but since the sync-* family is still unported some of it remains valid. I'm planning to work on sync-* soon. We should revisit this once that work is done. It should be easy to close then.
I believe that this is resolved since the merge of I08ec8e987225b8e7fb1ae2119a4623383cba4db7. The only scripts left in the scap collection that have not been converted to python at this point are refreshCdbJsonFiles, restart-twemproxy and scap-recompile.
