git-ubuntu

Merge ~bryce/git-ubuntu:systemd-service into git-ubuntu:master

Proposed by Bryce Harrington on 2019-10-10

Status:

Merged

Merged at revision:

5d104bce612db56f3274c667ee7b008da5e68c18

Proposed branch:

~bryce/git-ubuntu:systemd-service

Merge into:

git-ubuntu:master

Diff against target:

298 lines (+212/-0)

7 files modified

bin/failure-email.sh (+12/-0)
bin/import-source-packages.py (+10/-0)
doc/usd-importer-service.md (+164/-0)
setup.py (+1/-0)
snap/snapcraft.yaml (+2/-0)
usd-importer-failure-email@.service (+8/-0)
usd-importer.service (+15/-0)

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2019-10-18
Robie Basak		2019-10-10	Needs Information on 2019-10-15
Review via email: mp+373984@code.staging.launchpad.net

Description of the change

I've tested this locally using sendmail configured for local delivery, and synthetically verified both timeout restart emails and service crashes are detected and the appropriate emails sent and delivered. Everything's also nicely logged to the journal, and hopefully the documentation is sufficiently thorough.

Of note, I mentioned some followup work I've started to refactor out the mailer logic from the bin script, into code under gitubuntu/, but that work is orthogonal to this so kept out of scope for this MP so we can get this one landed soon. In addition to the refactoring, I am considering some improvements to how import failures are captured and reported in the emails, and so if you have ideas/wishlists/guidance feel free to shoot them to me.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2019-10-10:

FAILED: Continuous integration, rev:a4164589b9e8fe6cbad1290fdc7944b8a7d53401
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/371/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    FAILED: Unit Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/371//rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Robie Basak (racb) wrote on 2019-10-15:

Looks good! Some questions inline; I'm not sure if they need changes or not yet.

Hopefully my separate branch fixing CI will pass, then it can land, and then you should be able to rebase your branch on master. After that if you request a retest, we should see it pass, which we'll want before landing.

review: Needs Information

Revision history for this message

Bryce Harrington (bryce) wrote on 2019-10-17:

On Tue, Oct 15, 2019 at 04:48:56PM -0000, Robie Basak wrote:
> Review: Needs Information
>
> Looks good! Some questions inline; I'm not sure if they need changes or not yet.
>
> Hopefully my separate branch fixing CI will pass, then it can land, and then you should be able to rebase your branch on master. After that if you request a retest, we should see it pass, which we'll want before landing.
>
> Diff comments:
>
> > diff --git a/bin/import-source-packages.py b/bin/import-source-packages.py
> > index 3b6c37c..3b06253 100755
> > --- a/bin/import-source-packages.py
> > +++ b/bin/import-source-packages.py
> > @@ -19,6 +19,7 @@ import pprint
> > import sys
> > import tempfile
> > import time
> > +import systemd.daemon
>
> Should we be declaring this as a dependency anywhere? For example, in setup.py, and/or as a package from the distribution during the snap build in snapcraft.yaml?
>

Right, I'll add it to setup.py directly.

It should also probably go into the snap config, but I'm not entirely
sure how to add it. I'll take a stab at it though, and you can direct
me if it's incorrect.

> > # We expect to be running from a git repository in master for this
> > # script, because the snap's python code is not exposed except within
> > diff --git a/usd-importer.service b/usd-importer.service
> > new file mode 100644
> > index 0000000..68c5705
> > --- /dev/null
> > +++ b/usd-importer.service
> > @@ -0,0 +1,17 @@
> > +[Unit]
> > +Description=Git Ubuntu USD Importer service
> > +#After=network.target
> > +#StartLimitIntervalSec=0
>
> Should these commented lines still be present?

They are applicable for system level installation, but I'm a bit more
certain now we should just be running in user space so can drop them.

> > +OnFailure=status-email@%n.service
> > +
> > +[Service]
> > +Type=notify
> > +ExecStart=/usr/bin/python3 /snap/git-ubuntu/current/bin/import-source-packages.py
> > +Environment=PYTHONUNBUFFERED=1
>
> Why is PYTHONUNBUFFERED required, please? If it is still needed, can we document the reason somewhere please?

Since the daemon process is detached, python buffers the stdout and
stderr, so messages only show up in systemd's logs on flushes. We don't
want error messages to get cut off, so switch off output buffering via
this env var.

I'll add it to the documentation, good call.

> > +Restart=always
> > +RestartSec=60
> > +TimeoutSec=30
> > +WatchdogSec=21600
> > +
> > +[Install]
> > +WantedBy=multi-user.target
>
>
> --
> https://code.launchpad.net/~bryce/usd-importer/+git/usd-importer/+merge/373984
> You are the owner of ~bryce/usd-importer:systemd-service.

On Tue, Oct 15, 2019 at 04:48:56PM -0000, Robie Basak wrote:
> Review: Needs Information
> 
> Looks good! Some questions inline; I'm not sure if they need changes or not yet.
> 
> Hopefully my separate branch fixing CI will pass, then it can land, and then you should be able to rebase your branch on master. After that if you request a retest, we should see it pass, which we'll want before landing.
> 
> Diff comments:
> 
> > diff --git a/bin/import-source-packages.py b/bin/import-source-packages.py
> > index 3b6c37c..3b06253 100755
> > --- a/bin/import-source-packages.py
> > +++ b/bin/import-source-packages.py
> > @@ -19,6 +19,7 @@ import pprint
> >  import sys
> >  import tempfile
> >  import time
> > +import systemd.daemon
> 
> Should we be declaring this as a dependency anywhere? For example, in setup.py, and/or as a package from the distribution during the snap build in snapcraft.yaml?
>

Right, I'll add it to setup.py directly.

It should also probably go into the snap config, but I'm not entirely
sure how to add it.  I'll take a stab at it though, and you can direct
me if it's incorrect.

> >  # We expect to be running from a git repository in master for this
> >  # script, because the snap's python code is not exposed except within
> > diff --git a/usd-importer.service b/usd-importer.service
> > new file mode 100644
> > index 0000000..68c5705
> > --- /dev/null
> > +++ b/usd-importer.service
> > @@ -0,0 +1,17 @@
> > +[Unit]
> > +Description=Git Ubuntu USD Importer service
> > +#After=network.target
> > +#StartLimitIntervalSec=0
> 
> Should these commented lines still be present?

They are applicable for system level installation, but I'm a bit more
certain now we should just be running in user space so can drop them.

> > +OnFailure=status-email@%n.service
> > +
> > +[Service]
> > +Type=notify
> > +ExecStart=/usr/bin/python3 /snap/git-ubuntu/current/bin/import-source-packages.py
> > +Environment=PYTHONUNBUFFERED=1
> 
> Why is PYTHONUNBUFFERED required, please? If it is still needed, can we document the reason somewhere please?

Since the daemon process is detached, python buffers the stdout and
stderr, so messages only show up in systemd's logs on flushes.  We don't
want error messages to get cut off, so switch off output buffering via
this env var.

I'll add it to the documentation, good call.

> > +Restart=always
> > +RestartSec=60
> > +TimeoutSec=30
> > +WatchdogSec=21600
> > +
> > +[Install]
> > +WantedBy=multi-user.target
> 
> 
> -- 
> https://code.launchpad.net/~bryce/usd-importer/+git/usd-importer/+merge/373984
> You are the owner of ~bryce/usd-importer:systemd-service.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2019-10-17:

FAILED: Continuous integration, rev:ff6482e313c124def40e2df2a7f95ac4551598ee
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/373/
Executed test runs:
SUCCESS: VM Setup
FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/373//rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2019-10-17:

FAILED: Continuous integration, rev:5a84858bb1ac8db6237b9a267d4b05b44af158cd
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/374/
Executed test runs:
SUCCESS: VM Setup
FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/374//rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2019-10-17:

FAILED: Continuous integration, rev:b697a4ee9e152d518b42d87f0fd8926b5ba0654b
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/375/
Executed test runs:
SUCCESS: VM Setup
FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/375//rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2019-10-18:

PASSED: Continuous integration, rev:5d104bce612db56f3274c667ee7b008da5e68c18
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/376/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/376//rebuild