initial watchdog #505

cehbrecht · 2019-12-16T16:32:19Z

Overview

This PR introduces a new watchdog module. The code includes the PR #497.

Docs are not updated!

Changes:

watchdog module to handle job queue.
added click command line interface to start pywps and watchdog service.
updated pywps config to load process list from python module.
updated pywps configuration ... still some issues ... using os.environ PYWPS_CFG.

I have moved the CLI command line from the birdhouse template to pywps:
bird-house/cookiecutter-birdhouse#52

You can start a pywps with the watchdog like this:

$ pywps start -c /path/to/pywps.cfg --use-watchdog

You can also start both services standalone:

$ pywps start -c /path/to/pywps.cfg
$ pywps watchdog -c /path/to/pywps.cfg

The pywps.cfg needs to point to the processes list with a python list, see Emu example:
https://github.com/bird-house/emu/blob/6347264a8fd0b86bf6d0b7b5fc857aaf449003cd/pywps.cfg#L10

This is a initial implemention to get the cli working. See also issue #118.

Related Issue / Discussion

#497

Additional Information

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

[x ] I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
[x ] I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

cehbrecht · 2019-12-16T16:33:53Z

@jachym @davidcaron @huard could you please have a look at this PR and give your opinion. You can try it with the Emu on the watchdog branch:
https://github.com/bird-house/emu/tree/watchdog

cehbrecht · 2019-12-16T17:02:02Z

Should the watchdog be started as its own command?

$ watchdog start -c pywps.cfg

coveralls · 2019-12-16T17:02:55Z

Coverage decreased (-2.008%) to 73.153% when pulling c8ac7dd on cehbrecht:watchdog into 9709866 on geopython:master.

cehbrecht · 2019-12-16T17:03:28Z

We need also a clean task to remove stalled jobs from the running queue.

cehbrecht · 2019-12-16T17:04:19Z

The scheduler joblauncher should be refactored to also load the job status from the database.

huard

The daemon is sometimes called daemon, sometimes watchdog. I suggest a more descriptive name such as "RequestQueueDaemon".

There seems to be a mechanism in your PR to have a list of processes in the the configuration file. Is this documented ?

huard · 2019-12-16T21:04:35Z

Started Emu with pywps start --bind-host 0.0.0.0 -c pywps.cfg --use-watchdog
then

from birdy import WPSClient
wps_a = WPSClient("http://localhost:5000/wps", progress=True)                                                                                                                                       
resp_a = wps_a.inout()

fails with

HTTPError: 500 Server Error: INTERNAL SERVER ERROR for url: http://localhost:5000/wps

No such problem in sync mode.

huard · 2019-12-16T21:22:05Z

Also, would it be useful to have two logs, one for the daemon, and one for the server? Since the daemon generates a lot of noise in the logs (with DEBUG), it's hard to find the info relevant to the server.

cehbrecht · 2019-12-17T10:33:53Z

The daemon is sometimes called daemon, sometimes watchdog. I suggest a more descriptive name such as "RequestQueueDaemon".

class name: JobQueueHandler?
module name: jobqueue or queue?

There seems to be a mechanism in your PR to have a list of processes in the the configuration file. Is this documented ?

Not yet. It is a quick solution ... I needed this to get the CLI into pywps. See code:
https://github.com/cehbrecht/pywps/blob/7821562ef13c755b3b8ffab737a7c8241301308d/pywps/app/Service.py#L59

It is using the python expression of the process list and loads it via importlib.

huard · 2019-12-17T11:55:53Z

+1 Le mar. 17 déc. 2019 05 h 33, MacPingu <notifications@github.com> a écrit :

…

The daemon is sometimes called daemon, sometimes watchdog. I suggest a more descriptive name such as "RequestQueueDaemon". class name: JobQueueHandler? module name: jobqueue or queue? There seems to be a mechanism in your PR to have a list of processes in the the configuration file. Is this documented ? Not yet. It is a quick solution ... I needed this to get the CLI into pywps. See code: https://github.com/cehbrecht/pywps/blob/7821562ef13c755b3b8ffab737a7c8241301308d/pywps/app/Service.py#L59 It is using the python expression of the process list and loads it via importlib. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#505?email_source=notifications&email_token=AAAT2Q2V3N6MI5TDZVOH7ETQZCTJDA5CNFSM4J3MTL6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHB5SXQ#issuecomment-566483294>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT2Q2RZI5LQPEDG5ODMATQZCTJDANCNFSM4J3MTL6A> .

cehbrecht · 2019-12-17T11:59:26Z

It is still working on my side ... I couldn't reproduce the exception.

Started Emu with pywps start --bind-host 0.0.0.0 -c pywps.cfg --use-watchdog
then

from birdy import WPSClient
wps_a = WPSClient("http://localhost:5000/wps", progress=True)                                                                                                                                       
resp_a = wps_a.inout()

fails with

HTTPError: 500 Server Error: INTERNAL SERVER ERROR for url: http://localhost:5000/wps

No such problem in sync mode.

cehbrecht · 2019-12-17T12:02:41Z

Also, would it be useful to have two logs, one for the daemon, and one for the server? Since the daemon generates a lot of noise in the logs (with DEBUG), it's hard to find the info relevant to the server.

I'm not sure if I want to have two log files ... but I understand the trouble. As an alternative we could have two different types of logging handlers ... skip the db logs in debug mode ... unless they are wanted. Also the job queue can be triggered less frequently ... default was 30 secs.

cehbrecht · 2019-12-17T12:03:42Z

@huard Do you have an opinion on the CLI interface?

huard · 2019-12-17T12:15:48Z

Not really. Apart from watchdog name change, it looks good to me.

davidcaron

That's excellent! Thank you for doing this, I think it will make the queue more resilient.

docs/deployment.rst

pywps/app/Process.py

pywps/app/Service.py

pywps/cli.py

pywps/configuration.py

pywps/dblog.py

requirements.txt

docs/configuration.rst

cehbrecht · 2019-12-18T14:17:07Z

@huard @davidcaron
renamed watchdog to:

class: pywps.queue.JobQueueService
cli: pywps jobqueue -c pywps.cfg

tlvu · 2020-01-14T16:54:49Z

@tlvu I tried the configuration overwrite locally with emu. It worked for me and all the configs are written to the log file:

@cehbrecht I think I understood why the emu docker image did not find any default config files:

https://github.com/cehbrecht/pywps/blob/2099b6e4494923eb9b4c5284b079519f295a4e47/pywps/configuration.py#L178-L183
1. PYWPS_INSTALLATION/etc/pywps.cfg
2. /etc/pywps.cfg
3. $HOME/.pywps.cfg

https://github.com/bird-house/emu/blob/0b36f1baabaa33c5e16a60438e333d2a44c410b1/Dockerfile#L31
Default config file in emu docker image: /opt/wps/pywps.cfg

As we see, the default config file inside the emu docker image (/opt/wps/pywps.cfg) is not in any of the 3 locations above. Should we modify emu docker image to put the default config file in 1 of those 3 locations or add more default locations?

cehbrecht · 2020-01-20T16:29:46Z

As we see, the default config file inside the emu docker image (/opt/wps/pywps.cfg) is not in any of the 3 locations above. Should we modify emu docker image to put the default config file in 1 of those 3 locations or add more default locations?

@tlvu we may want to add an additional default location for searching in the app path. Maybe we can do this in another PR? I think we have to change a bit more for this. Would it be ok to use /etc/pywps.cfg as an intermediate step? You only have on pywps in your container.

tlvu · 2020-01-20T16:34:03Z

@cehbrecht

@tlvu we may want to add an additional default location for searching in the app path. Maybe we can do this in another PR? I think we have to change a bit more for this.

Sure I am fine with this.

Would it be ok to use /etc/pywps.cfg as an intermediate step? You only have on pywps in your container.

Yes for how we can drop the default config file in /etc/pywps.cfg instead of /opt/wps/pywps.cfg, since as you said, there's only 1 pywps per container so this is perfectly fine.

cehbrecht · 2020-01-21T16:20:58Z

@jachym @tlvu @davidcaron What should we do with the alembic database upgrade? Currently the alembic scripts are not aware that there is an old db with a different schema and the migration step will fail. Should we drop the old DB manually? Or should we add the following to the alembic upgrade script (https://github.com/cehbrecht/pywps/blob/2099b6e4494923eb9b4c5284b079519f295a4e47/pywps/alembic/versions/eead25e2ad3a_create_tables.py#L19):

    # remove previous db
    try:
        op.drop_table('pywps_requests')
        op.drop_table('pywps_stored_requests')
    except Exception:
        pass

davidcaron · 2020-01-21T16:40:39Z

A couple things to consider, and my opinions:

Do we want to keep the history of pywps_requests or not? (I would personally not mind if it was dropped)
The pywps_prefix in the table is configurable. (Alembic could have access to this configuration value, but I don't think the migration command parses the pywps configuration files.)
If the schemas for pywps_requests and pywps_jobs at the first revision are the same, we could rename pywps_requests to pywps_jobs ... I think this would cover most of the environments if not all.
Same thing for pywps_stored_requests, if it exists, keep it.

cehbrecht · 2020-01-21T17:38:59Z

A couple things to consider, and my opinions:

* Do we want to keep the history of `pywps_requests` or not? (I would personally not mind if it was dropped)

I would prefer to drop it.

* The `pywps_`prefix in the table is configurable. (Alembic could have access to this configuration value, but I don't think the migration command parses the pywps configuration files.)

I have removed the configuration for the prefix. It is now fixed to pywps_.

* If the schemas for `pywps_requests` and `pywps_jobs` at the first revision are the same, we could rename `pywps_requests` to `pywps_jobs` ... I think this would cover most of the environments if not all.

* Same thing for `pywps_stored_requests`, if it exists, keep it.

I would prefer to just drop the old tables. We just want to get in the alembic managed mode.

tlvu · 2020-01-21T18:47:22Z

I suppose there's no real value to keep the existing data so we can drop those tables but I think @huard might understand better the implication of flushing existing data.

On the flip side, if the existing data is not so useful to preserve, why are we preserving them in the first place?

cehbrecht · 2020-01-21T19:40:13Z

On the flip side, if the existing data is not so useful to preserve, why are we preserving them in the first place?

we need a database for the job queue. PyWPS also uses the database for request logging ... but those infos are not meant to be kept for ever.

cehbrecht · 2020-01-23T12:26:32Z

@jachym @tlvu @davidcaron What should we do with the alembic database upgrade? Currently the alembic scripts are not aware that there is an old db with a different schema and the migration step will fail. Should we drop the old DB manually? Or should we add the following to the alembic upgrade script (https://github.com/cehbrecht/pywps/blob/2099b6e4494923eb9b4c5284b079519f295a4e47/pywps/alembic/versions/eead25e2ad3a_create_tables.py#L19):
    # remove previous db
    try:
        op.drop_table('pywps_requests')
        op.drop_table('pywps_stored_requests')
    except Exception:
        pass

@jachym @tomkralidis @ldesousa Can you give me advise on this? I think this is the last thing to fix before we can merge.

tomkralidis · 2020-01-23T12:33:52Z

I don't know the full context, but for data/database deletion I would prompt the user if they would like to drop the data in question, and if applicable add a -y/--yes flag if driven from a CLI. So the underlying function that calls the table dropping has a force=True/False type flag which can be acted upon accordingly.

cehbrecht · 2020-01-23T14:25:05Z

@tomkralidis I could extend the pywps migrate cli with --force option to drop the previous database. I would leave the alembic untouched. At least this would help on the transition to alembic.

cehbrecht · 2020-01-23T16:58:15Z

@tomkralidis I have added a force option to the migrate cli to drop the existing db:

pywps migrate --force

c8ac7dd

jachym · 2020-02-02T10:21:33Z

Everybody is happy ? Squash and merge? @tomkralidis ? @cehbrecht ? @huard and @davidcaron ?

please add your review check

cehbrecht · 2020-02-03T09:38:00Z

Everybody is happy ? Squash and merge? @tomkralidis ? @cehbrecht ? @huard and @davidcaron ?

please add your review check

@jachym I'm happy with it. Should we make a 4.2.4 release before and/or open a 4.2.x branch?
4.2.3...master

jachym · 2020-02-03T21:39:52Z

Should we make a 4.2.4 release before and/or open a 4.2.x branch?
4.2.3...master

I agree with making 4.2 branch and make release.

IMHO since this is compability break, maybe the next release should be called 5.x ?

cehbrecht · 2020-02-04T11:13:30Z

Should we make a 4.2.4 release before and/or open a 4.2.x branch?
4.2.3...master

I agree with making 4.2 branch and make release.

IMHO since this is compability break, maybe the next release should be called 5.x ?

I'm ok with 5.x.

jachym · 2020-02-09T15:29:43Z

merged to master - thank you all

initial watchdog

f5e877f

cehbrecht requested a review from jachym December 16, 2019 16:32

cehbrecht added the enhancement label Dec 16, 2019

fix tests

7821562

huard reviewed Dec 16, 2019

View reviewed changes

davidcaron reviewed Dec 17, 2019

View reviewed changes

cehbrecht added 7 commits December 18, 2019 11:45

removed unused finally clause

dba42b4

simplified processes load

b2402c1

reverted to NullPool

3a93271

removed flufl.enum

0e5a0de

pep8

15b759e

renamed watchdog to jobqueue

4ad9b27

fixed jobqueue config

869f3dc

cehbrecht added 3 commits December 18, 2019 15:51

updated docs

2c7cbea

fixed parallelprocesses

c22a0a6

update service description

1826c3e

cehbrecht mentioned this pull request Jan 14, 2020

update template ... pin pywps 4.2 cp4cds/c4cds-wps#29

Merged

cehbrecht mentioned this pull request Jan 23, 2020

Watchdog bird-house/ansible-wps-playbook#92

Closed

added drop_db to migrate

c8ac7dd

tomkralidis self-requested a review February 2, 2020 13:05

tomkralidis approved these changes Feb 2, 2020

View reviewed changes

davidcaron approved these changes Feb 3, 2020

View reviewed changes

This was referenced Feb 6, 2020

Add support for multiple languages #510

Merged

release 4.2.4 #511

Merged

jachym merged commit 71a12c2 into geopython:master Feb 9, 2020

cehbrecht deleted the watchdog branch February 10, 2020 09:43

cehbrecht mentioned this pull request Feb 18, 2020

More robust queue handling #493

Open

tlvu mentioned this pull request Sep 30, 2020

Using progress=True results in hung process after a previous run of sync process bird-house/finch#121

Closed

cehbrecht mentioned this pull request Feb 8, 2021

Backport master to 4.4 #574

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial watchdog #505

initial watchdog #505

cehbrecht commented Dec 16, 2019 •

edited

Loading

cehbrecht commented Dec 16, 2019

cehbrecht commented Dec 16, 2019

coveralls commented Dec 16, 2019 •

edited

Loading

cehbrecht commented Dec 16, 2019

cehbrecht commented Dec 16, 2019

huard left a comment

huard commented Dec 16, 2019

huard commented Dec 16, 2019

cehbrecht commented Dec 17, 2019

huard commented Dec 17, 2019 via email

cehbrecht commented Dec 17, 2019

cehbrecht commented Dec 17, 2019

cehbrecht commented Dec 17, 2019

huard commented Dec 17, 2019

davidcaron left a comment

cehbrecht commented Dec 18, 2019

tlvu commented Jan 14, 2020

cehbrecht commented Jan 20, 2020

tlvu commented Jan 20, 2020

cehbrecht commented Jan 21, 2020

davidcaron commented Jan 21, 2020

cehbrecht commented Jan 21, 2020

tlvu commented Jan 21, 2020

cehbrecht commented Jan 21, 2020

cehbrecht commented Jan 23, 2020

tomkralidis commented Jan 23, 2020

cehbrecht commented Jan 23, 2020

cehbrecht commented Jan 23, 2020

jachym commented Feb 2, 2020

cehbrecht commented Feb 3, 2020

jachym commented Feb 3, 2020 •

edited

Loading

cehbrecht commented Feb 4, 2020

jachym commented Feb 9, 2020

initial watchdog #505

initial watchdog #505

Conversation

cehbrecht commented Dec 16, 2019 • edited Loading

Overview

Related Issue / Discussion

Additional Information

Contribution Agreement

cehbrecht commented Dec 16, 2019

cehbrecht commented Dec 16, 2019

coveralls commented Dec 16, 2019 • edited Loading

cehbrecht commented Dec 16, 2019

cehbrecht commented Dec 16, 2019

huard left a comment

Choose a reason for hiding this comment

huard commented Dec 16, 2019

huard commented Dec 16, 2019

cehbrecht commented Dec 17, 2019

huard commented Dec 17, 2019 via email

cehbrecht commented Dec 17, 2019

cehbrecht commented Dec 17, 2019

cehbrecht commented Dec 17, 2019

huard commented Dec 17, 2019

davidcaron left a comment

Choose a reason for hiding this comment

cehbrecht commented Dec 18, 2019

tlvu commented Jan 14, 2020

cehbrecht commented Jan 20, 2020

tlvu commented Jan 20, 2020

cehbrecht commented Jan 21, 2020

davidcaron commented Jan 21, 2020

cehbrecht commented Jan 21, 2020

tlvu commented Jan 21, 2020

cehbrecht commented Jan 21, 2020

cehbrecht commented Jan 23, 2020

tomkralidis commented Jan 23, 2020

cehbrecht commented Jan 23, 2020

cehbrecht commented Jan 23, 2020

jachym commented Feb 2, 2020

cehbrecht commented Feb 3, 2020

jachym commented Feb 3, 2020 • edited Loading

cehbrecht commented Feb 4, 2020

jachym commented Feb 9, 2020

cehbrecht commented Dec 16, 2019 •

edited

Loading

coveralls commented Dec 16, 2019 •

edited

Loading

jachym commented Feb 3, 2020 •

edited

Loading