Closed taldcroft closed 1 year ago
Great and quick idea. It seems to me that the concerns are speed and security. Speed seems reasonable. For security, I like that it "only allow functions in the public API of starcheck module". Do we also need to either limit access to the server with key or obscurity? And/or run the Python in a sandbox or container, or as unprivileged user?
Maybe I'm being naive, but I think an attack would need this:
starcheck
package.starcheck.utils
are exposed.It also just occurred to me that the starcheck script could generate a random key and pass that to the server on creation and then require that key for commands, so that might pretty much make this lock tight.
Yeah, that's what I meant by "key" as the first choice. It wouldn't need to be SSL (it would just be a startup generated "password") . And you'd add obscurity by using an available port instead of the hardcoded one (which you mention as the third box in the todo items anyway).
I also wasn't sure about IPC vs TCP for this kind of app.
And despite my "It wouldn't need to be SSL" comment, I don't know how much slower this would be if it used standard key security via SSL/TLS.
OK, now the communication between client and server is secured by a shared random key that generated by starcheck.pl
at the beginning. I think this addresses any credible security issues.
Also the port is now randomly selected by the OS.
@jeanconn - apart from details like logging, this is ready for real review. In order to improve performance I made some code changes that you need to review carefully.
Along the way I reformatted sections of code that I was touching since I just couldn't see the logic otherwise. It might just be worth running perltidy
at some point.
I think this is really great, but common usage for me with starcheck has been to "ctrl-c" all the time when I realize I've run with the wrong options or whatever. I just had 8 of these python servers going. So it seems like the python server needs to check regularly if the parent process is alive and also have some kind of reasonable timeout. And/or the perl could handle this signal and do the cleanup?
(well, and also tell the user not to ctrl-c but it should be robust to that).
I spent time playing around this morning as well. My first realization is that using this fork trick is not apparently required. There was a point in the development when I had to do that, but I think the implementation has changed in the meantime.
With 79c351f, the processes all seem to get cleaned up in good order. This included running to completion, Ctrl-C, and kill -9
of the sandbox.
@jeanconn - can you try checking out 2502469 and see if you can manage to start up starcheck and get orphaned child processes hanging around? I could not (on MacOS of course).
So I guess you didn't see my comments before starting your work. I'd hoped to save time but. Your current implementation matches what I had except I'd moved to using signal alarm instead of trying to figure out how to actually have handle_timeout do something.
Your comments did not indicate having solved the problem.
It solved the problem but removed the extra perl fork. I was trying to figure out why the extra perl fork was there but that seems lost to history.
Bummer, guess I wasted my entire morning. Your comments were off track, the suggestion would not have worked (applied literally), and you didn't push any commits or say that you had something which solved the problem.
My comments were asking why there was a perl fork before I removed it.
Sorry about the suggestions. I have not tried those before so I cut and paste I guess that's not how it works.
I also thought ball was in my court so had just reached out trying to make sure I didn't go down a path (removing perl fork) that had different problems.
Well anyway the commits I have pushed should work and finish this up. Actually I think the first commit alone is enough.
Oh well. My lesson learned is just to push the commits even if possibly controversial. And it was a little bit of a tricky problem so the independent solutions arriving at the same place is a good sign.
Update to what I said. When I start the sandbox you have processes like so:
7848 ttys006 0:00.00 /bin/bash ./sandbox_starcheck -dir /Users/aldcroft/ska/data/mpcrit1/mplogs/2022/DEC1922/
7854 ttys006 0:00.09 perl -I ./starcheck/src/lib ./starcheck/src/starcheck.pl -dir /Users/aldcroft/ska/data/m
7855 ttys006 0:04.31 python -m starcheck.server
If I kill the perl (7854) then that leaves an orphaned python -m starcheck.server
. If I will the wrapper 7848
then that cleans up the python.
Well at least say out loud that you have something which is working. Your questions and suggestion gave the impression you were still lost.
I also will try more github suggestion practice. I'd highlighted the whole code block to replace but apparently that didn't take.
And with the server timeout commit 11fe68c then the Python cleans up even with a kill -9 of the other perl processes. I had the timeout set short 2 seconds for testing but changed it to 15 seconds for this test.
But yes, my conclusion was that if the extra perl fork was not needed by something else then removing it had the two advantages of letting primary process kill the python server in "nominal operations" and having everything apparently die on ctrl-c.
Anyway you can do more testing and start down the road to making this a production PR. Push early and often. :smile: I'll step away.
With the handle_request / handle_timeout timeout strategy (so the timeout applies after N seconds of no requests to the server) is the 2sec timeout basically fine for production? Or should this be something like 2 minutes?
I think the timeout should be the latter, at least 2 minutes. Basically we don't want any chance it can time out for normal operation.
Sounds good. I couldn't think of anything that starcheck would do at this point that wouldn't need Python within a couple of seconds, but it is best to just set the timeout in minutes instead of doing a full assessment of the "time between python calls".
For the action item of " Proper logging not print statements" it is hard to know what makes sense. The Perl side is a mess of print statements. The new python code could have better logging, but would we really want to use utils.config_logging for this? One can't use it to start logging the server startup, for example, because one needs to start up the Python server to do things like configure a logger. I'm not sure how much value it would add here.
About logging, just whatever fits with the existing standards for starcheck is fine. I didn't really look at what the rest of the Perl code is doing. For the Python code you could just make a logger with ska_helpers.logger.basic_logger
, with a default log level of INFO
and then all logger.debug(..)
statements. The main point is have code in there to make it easy to debug in a dev environment if something goes wrong. In theory you could have the main command line logging level control this but probably not worth it initially.
I am not going into the details of this PR, just following the comments, and I wanted to say that I just used it with this weeks loads without trouble. And by that I mean:
starcheck.txt
matched the current flight version.My testing is not done, but I've updated the PR with some linux functional and regression test notes.
The testing looks quite thorough, thanks!
About the decode
's, good catch. Did you search for any outstanding decode()
calls?
Was that code actually being run? I would assume that it would fail with an exception always if it ran.
Yes, I searched for other decodes and found none. The characteristics date code with the decode that was hanging around is only called for some historical schedules (so I fixed it so more regression weeks would run to completion). We could perhaps cut it at this point and define a will-run-only-on-schedules-after date for starcheck and document. But I figured not for this release.
As noted above I just found a tiny defect in a change of mine, but I still think this is fine to go. I just don't want to merge as some of this is my code with only (recorded) self-review.
Description
Driven by problems getting
Inline::Python
to work with ska3-prime, this PR replaces all the inline Python calls with a call to a new lightweight server process that runs Python function calls. This is basically the idea that @javierggt for an API server, but it users an even lighter framework (than e.g. flask) for performance. The server gets called at least thousands of times currently, though there are probably opportunities to reduce that with some code changes.Current testing status is that it runs to completion on the DEC1922 loads and produces output that looks about right.
Process management for the Python function server (
starcheck/server.py
) took a little time to work out but it seems to be working, more or less. There might still be room for improvement.The new server includes reasonable exception handling.
To do:
get_fid_actions
sub in parse CM. I think this might be broken for running loads in the far past.Interface impacts
Testing
Unit tests
Functional tests
Output comparison
Ran starcheck for DEC1922A loads in this branch and in master:
Then:
On ska3-flight HEAD linux
Functional and regression tests
Ran these in https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr402/
To functionally test the change in the bad pixel check, I introduced a bad pixel near a guide star and compared outputs vs master.
https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr402/badpixcheck/jan0923_master_badpix.html#obsid25550
https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck-pr402/badpixcheck/jan0923_noinline_badpix.html#obsid25550
Check is still functional.
Run of full regression test set revealed latent issue with #389 fixed in 5be93b4. I compared this PR code to master + that same fix and the regression outputs show only two tiny, understood, and acceptable changes related to the PR update to the bad pixel test.
Performance
This branch runs about 50% slower. For DEC1922A this means about 90 seconds in this branch vs. 60 seconds in master.