[Webservice] Restructure database to give more meaningful success/failure information

Description

At present, the success/failure information we send to myMdC is unreliable in various important ways - see calibration/planning#104 . In addition, the correction_complete events sent via Kafka are sent per detector, but they don't actually tell you which detector they relate to (they give a detector type, such as JUNGFRAU, but not the Karabo ID of a specific detector). Replacing the webservice still looks like a big enough job that it seemed worth spending a bit of time improving the old one. I hope this might also ease the transition to the new one when it's ready.

The jobs database is completely redesigned, with three tables instead of one: each request from myMdC can have several executions, each of which can have several slurm_jobs. I think these terms line up with what Robert is using in Orca. I haven't tried to do any migration of existing data - we're only using it to track active jobs, so I'm assuming we'll just start with an empty database when deploying this change.

The jobs monitor code is also largely refactored, with the loop broken up into several methods in a JobsMonitor class.

How Has This Been Tested?

Deployed on max-exfl017, and submitted correction & dark requests from the test myMdC instance for the CALLAB proposal.

This revealed a number of previously hidden issues which are (hopefully) specific to CALLAB. The presence of JNGFR01 in the filenames triggers an execution for both FXE_XAD_JF1M and SPB_IRDA_JF4M, at least one of which will fail on any given run (as it can't find its data). Previously, the webservice would hide this, because the failing jobs would fail quickly, and the status would only come from the last jobs running, but with these changes the failures are visible.

Relevant Documents (optional)

https://in.xfel.eu/test_metadata/proposals/259/runs/63612

https://in.xfel.eu/test_metadata/proposals/259/runs/86193

https://in.xfel.eu/test_metadata/proposals/259#proposal-calibration

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Refactor (refactoring code with no functionality changes)

Checklist:

• My code follows the code style of this project.

Reviewers

@ahmedk @roscar @schmidtp

This is the message we're already sending (per execution). I've added the karabo_id field. I think 'Karabo ID' might be just what we call this internally for calibration, so maybe we should find a better external name for it.

The canonical name according to the Karabo naming convention for the first part of a device ID is domain. I wouldn't have hard feelings against Karabo ID if not for the unfortunate similarity to device ID.

So I would propose either "karabo-domain" or something Karabo-agnostic like "detector-installation" or just "installation".

OK, I'll go with detector_installation for now; it's long but I think it's quite clear.

FYI, I am aware that so far we have karabo_id (notebooks), detector_identifier(CALCAT APIs), and domain in the mentioned documentation.

Will we really benefit from adding a new name?

Good point. @schmidtp how does detector_identifier sound to you?

Sure, fine by me.

In all fairness CalCat also has detector_name and uses karabo_id itself in some APIs It's a real mess.

This is a new event for when a request finishes, i.e. all the corrected data for a run has been produced (if success is True).

If query-rid is actually used often, we should probably make an index on mymdc_id in the database to make this lookup more efficient.

added Waiting for review label

changed milestone to %3.6.0

For supervisord to manage the process in a (slightly) nicer way, it may be worth adding something here to catch sigkill/interrupt and explicitly handle it. Maybe adding something like:

    def run(self):
        while True:
            try:
                self.do_updates()
            except KeyboardInterrupt:
                self.stop("`KeyboardInterrupt`")
            except Exception:
                log.error("Failure to update job DB", exc_info=True)
            time.sleep(self.time_interval)

    def stop(self, *args, **kwargs):
        log.critical(f"Shutting down: {args}, {kwargs}")
        exit(0)

With a bit at the end of main that does something like:

def main(argv=None):
    ...
    logging.getLogger('kafka').setLevel(logging.INFO)
    jm = JobsMonitor(config)

    signal.signal(signal.SIGTERM, jm.stop)
    signal.signal(signal.SIGINT, jm.stop)

    jm.run()

Good point, see what you think of the implementation I've just committed.

added 1 commit

• 07d40437 - Clean shutdown on SIGTERM

Compare with previous version

added 1 commit

• 0f67b362 - Rename karabo_id -> detector_installation in Kafka message

Compare with previous version

Looks good, thanks!

Given you restructured the database, we probably need changes to serve_overview?

Does it need to be added to caldeploy explicitly?

Given you restructured the database, we probably need changes to serve_overview?

Good point, thanks - I had missed that serve_overview accesses the database directly as well. I'll get on that.

That reminds me: do we also want to store the python -m xfel-calibrate ... command line in the database (in the executions table)? It would make the database grow a bit quicker, but we could get rid of some log parsing in serve_overview by reading the database instead.

Does it need to be added to caldeploy explicitly?

The database? Or something else? caldeploy doesn't currently know anything about the database - it's all just set up by the webservice code when it's launched.

The database? Or something else? caldeploy doesn't currently know anything about the database - it's all just set up by the webservice code when it's launched.

Having it tracked and launched.

do we also want to store the python -m xfel-calibrate ... command line in the database (in the executions table)?

I'm not sure. I always thought of xfel-calibrate as stateless. I suppose you suggest having some command line argument to the sqlite database passed by the webservice, but not used interactively?

Having it tracked and launched.

This doesn't add anything new to launch - there was another MR that already split the job monitor out as a separate process (!668 (merged)).

I'm not sure. I always thought of xfel-calibrate as stateless. I suppose you suggest having some command line argument to the sqlite database passed by the webservice, but not used interactively?

I think we might be talking at cross purposes here. My suggestion was about storing the xfel-calibrate command that the webservice launches in the database, not about passing anything extra in that command.

I've briefly tested serve_overview with my new commit (8a3f12bc).

I think we might be talking at cross purposes here. My suggestion was about storing the xfel-calibrate command that the webservice launches in the database, not about passing anything extra in that command.

Yes, I think I misread that sentence grammatically. Sure, makes sense. I do not think we should worry about space.

Ultimately I was considering asking whether we want to move to MySQL now already, but it should be its own MR.

OK, I'll do that.

Do you see any particular reason to change DB? I'm probably guilty of being an SQLite fanboy, but it's very convenient not having a separate server to manage or authentication to deal with. And in this context, I quite like that we naturally get a new database file on each deployment, so you don't have to worry about migrations or anything (although I have on occasion copied the file over from one deployment to the next).

It's mainly about integration with Grafana. Right now we essentially have yet another service that translates into MySQL...

OK, I've added the command to the database, and updated the serve_overview code to use that. I've just checked on max-exfl017 that it's reading jobs back correctly from the database:

added 1 commit

• 8a3f12bc - Use new database structure in serve_overview

Compare with previous version

Nice MR @kluyvert . I think we need to include this script as well. As it breaks now with these changes https://git.xfel.eu/detectors/pycalibration/-/blob/master/webservice/sqlite_view.py

added 1 commit

• 85959cde - Update sqlite_view.py script

Compare with previous version

added 1 commit

• 55df983d - Ensure run numbers are consistently integers

Compare with previous version

added 2 commits

• 4f068c21 - Parse run numbers as integers in sqlite_view.py
• fc6b814f - Clean up sqlite_view script a bit

Compare with previous version

added 1 commit

• f88fc9d0 - Convert run number back to string for Kafka messages

Compare with previous version

added 1 commit

• 0b847163 - Rename detector_installation -> detector_identifier in Kafka message

Compare with previous version

added 2 commits

• 452133c8 - Store xfel-calibrate command in jobs database
• dacb2cfc - Use commands from database in serve_overview instead of log scraping

Compare with previous version