Update / error handling fixes
cover Update / error handling fixes
The layer index has recently been failing to update, and that wasn't picked up earlier as the errors weren't actually being collected in the update records. Makesure they are collected and sufficiently highlighted in the UI.
The following changes since commit d17080d325b9b82e8b844c207053eb2b3ac111ad:
Show links to include files on recipe detail (2018-08-13 16:04:16 +0200)
are available in the Git repository at:
git://git.yoctoproject.org/layerindex-web paule/layerupdate
http://git.yoctoproject.org/cgit.cgi/layerindex-web/log/?h=paule/layerupdate
Paul Eggleton (5):
update: avoid errors if values could not be found in initial output
update_layer: improve exception handling
Improve collection/display of LayerUpdate records
update: ensure that exceptions during the update process are logged Include errors/warnings in main log in error/warning counts for
updates
TODO | 1 - layerindex/admin.py | 2 +- .../0021_layerupdate_add_layer_branch.py | 26 +++++++ .../0022_layerupdate_set_layer_branch.py | 41 +++++++++++ .../0023_layerupdate_layer_branch_finalise.py | 29 ++++++++ .../migrations/0024_layerupdate_vcs_revs.py | 35 ++++++++++ layerindex/models.py | 36 +++++++++- layerindex/static/css/additional.css | 10 +++ layerindex/templatetags/extrafilters.py | 4 ++ layerindex/update.py | 68 +++++++++++++------ layerindex/update_layer.py | 3 +- layerindex/views.py | 6 +- templates/layerindex/layerupdate.html | 19 +++++- templates/layerindex/updatedetail.html | 34 ++++++++-- templates/layerindex/updatelist.html | 6 +- 15 files changed, 284 insertions(+), 36 deletions(-) create mode 100644 layerindex/migrations/0021_layerupdate_add_layer_branch.py create mode 100644 layerindex/migrations/0022_layerupdate_set_layer_branch.py create mode 100644 layerindex/migrations/0023_layerupdate_layer_branch_finalise.py create mode 100644 layerindex/migrations/0024_layerupdate_vcs_revs.py
Signature · 1 line
2.17.1
PATCH 1/5 update: avoid errors if values could not be found in initial output
If for some reason update_layer.py does not print out the values we expect then we shouldn't be throwing a traceback, we should be handling it gracefully - i.e., print an error and then move on to the next layer.
--- layerindex/update.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
layerindex/update.py +16 -4
@@ -118,6 +118,13 @@ def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc): elif vcs_subdir: logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc)) +def extract_value(valuename, output): + res = re.search("^%s = \"(.*)\"" % valuename, output, re.M) + if res: + return res.group(1) or '' + else: + return '' + def main(): if LooseVersion(git.__version__) < '0.3.1': logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
@@ -395,10 +402,15 @@ def main(): sys.exit(254) elif ret != 0: continue - col = re.search("^BBFILE_COLLECTIONS = \"(.*)\"", output, re.M).group(1) or '' - ver = re.search("^LAYERVERSION = \"(.*)\"", output, re.M).group(1) or '' - deps = re.search("^LAYERDEPENDS = \"(.*)\"", output, re.M).group(1) or '' - recs = re.search("^LAYERRECOMMENDS = \"(.*)\"", output, re.M).group(1) or '' + + col = extract_value('BBFILE_COLLECTIONS', output) + if not col: + logger.error('Unable to find BBFILE_COLLECTIONS value in initial output') + # Assume (perhaps naively) that it's an error specific to the layer + continue + ver = extract_value('LAYERVERSION', output) + deps = extract_value('LAYERDEPENDS', output) + recs = extract_value('LAYERRECOMMENDS', output) deps_dict = utils.explode_dep_versions2(bitbakepath, deps) recs_dict = utils.explode_dep_versions2(bitbakepath, recs)
Signature · 1 line
2.17.1
PATCH 2/5 update_layer: improve exception handling
In reply to Paul EggletonWhen an exception occurs during the main part of update_layer, we were catching and printing it but that's not enough - we need to do the following as well:
* Use logger.error() to print the exception information, so that it gets logged and highlighted as an error in the layer update
* Exit with a non-zero return code so that update.py knows it has failed
--- layerindex/update_layer.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
layerindex/update_layer.py +2 -1
@@ -808,7 +808,8 @@ def main(): pass except: import traceback - traceback.print_exc() + logger.error(traceback.format_exc().rstrip()) + sys.exit(1) finally: if tinfoil and (LooseVersion(bb.__version__) > LooseVersion("1.27")): tinfoil.shutdown()
Signature · 1 line
2.17.1
PATCH 3/5 Improve collection/display of LayerUpdate records
In reply to Paul EggletonMake layerupdate collection slightly more reliable and make it easier to see when updates have actually been captured:
* Split layerbranch into separate layer and branch fields, since there may not be a layerbranch in existence but we might want to log an error relating to the branch and layer.
* Show all layerupdates on the update detail page, not just those with log messages
* Record before and after revisions and show these in the update detail and layerupdate detail (with links)
* Record return code of update_layer process
* Highlight layer updates with a non-zero return code, errors or warnings in the output on the update detail page
* Show duration on the layerupdate detail page
--- TODO | 1 - layerindex/admin.py | 2 +- .../0021_layerupdate_add_layer_branch.py | 26 +++++++++++ .../0022_layerupdate_set_layer_branch.py | 41 +++++++++++++++++ .../0023_layerupdate_layer_branch_finalise.py | 29 ++++++++++++ .../migrations/0024_layerupdate_vcs_revs.py | 35 +++++++++++++++ layerindex/models.py | 28 ++++++++++-- layerindex/static/css/additional.css | 10 +++++ layerindex/templatetags/extrafilters.py | 4 ++ layerindex/update.py | 44 ++++++++++++------- layerindex/views.py | 4 +- templates/layerindex/layerupdate.html | 19 +++++++- templates/layerindex/updatedetail.html | 34 ++++++++++++-- 13 files changed, 249 insertions(+), 28 deletions(-) create mode 100644 layerindex/migrations/0021_layerupdate_add_layer_branch.py create mode 100644 layerindex/migrations/0022_layerupdate_set_layer_branch.py create mode 100644 layerindex/migrations/0023_layerupdate_layer_branch_finalise.py create mode 100644 layerindex/migrations/0024_layerupdate_vcs_revs.py
TODO +0 -1
@@ -43,7 +43,6 @@ Features * Use bar instead of pie graphs for OE-Classic statistics * Ability for reviewers to comment before publishing a layer? * Show a note at the top of the layer edit form if there's a validation error -* Record layer update start/end revisions * Show count in duplicates page * Search on layer selection dialogs
layerindex/admin.py +1 -1
@@ -85,7 +85,7 @@ class UpdateAdmin(admin.ModelAdmin): pass class LayerUpdateAdmin(admin.ModelAdmin): - list_filter = ['update__started', 'layerbranch__layer__name', 'layerbranch__branch__name'] + list_filter = ['update__started', 'layer', 'branch'] class RecipeAdmin(admin.ModelAdmin): search_fields = ['filename', 'pn']
layerindex/migrations/0021_layerupdate_add_layer_branch.py new file +26 -0
@@ -0,0 +1,26 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.12 on 2018-08-13 22:44 +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('layerindex', '0020_update_manual'), + ] + + operations = [ + migrations.AddField( + model_name='layerupdate', + name='branch', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='layerindex.Branch'), + ), + migrations.AddField( + model_name='layerupdate', + name='layer', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='layerindex.LayerItem'), + ), + ]
layerindex/migrations/0022_layerupdate_set_layer_branch.py new file +41 -0
41 lines not on this page · raw message
layerindex/migrations/0023_layerupdate_layer_branch_finalise.py new file +29 -0
@@ -0,0 +1,29 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.12 on 2018-08-13 23:06 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('layerindex', '0022_layerupdate_set_layer_branch'), + ] + + operations = [ + migrations.AlterField( + model_name='layerupdate', + name='branch', + field=models.ForeignKey(to='layerindex.Branch'), + ), + migrations.AlterField( + model_name='layerupdate', + name='layer', + field=models.ForeignKey(to='layerindex.LayerItem'), + ), + migrations.RemoveField( + model_name='layerupdate', + name='layerbranch', + ), + ]
layerindex/migrations/0024_layerupdate_vcs_revs.py new file +35 -0
@@ -0,0 +1,35 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.12 on 2018-08-13 23:24 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('layerindex', '0023_layerupdate_layer_branch_finalise'), + ] + + operations = [ + migrations.AddField( + model_name='layerupdate', + name='vcs_after_rev', + field=models.CharField(blank=True, max_length=80, verbose_name='Revision after'), + ), + migrations.AddField( + model_name='layerupdate', + name='vcs_before_rev', + field=models.CharField(blank=True, max_length=80, verbose_name='Revision before'), + ), + migrations.AddField( + model_name='layerupdate', + name='retcode', + field=models.IntegerField(default=0), + ), + migrations.AlterField( + model_name='layerupdate', + name='finished', + field=models.DateTimeField(blank=True, null=True), + ), + ]
layerindex/models.py +25 -3
@@ -394,13 +394,35 @@ class LayerNote(models.Model): class LayerUpdate(models.Model): - layerbranch = models.ForeignKey(LayerBranch) + layer = models.ForeignKey(LayerItem) + branch = models.ForeignKey(Branch) update = models.ForeignKey(Update) started = models.DateTimeField() - finished = models.DateTimeField() + finished = models.DateTimeField(blank=True, null=True) errors = models.IntegerField(default=0) warnings = models.IntegerField(default=0) + vcs_before_rev = models.CharField('Revision before', max_length=80, blank=True) + vcs_after_rev = models.CharField('Revision after', max_length=80, blank=True) log = models.TextField(blank=True) + retcode = models.IntegerField(default=0) + + def layerbranch_exists(self): + """Helper function for linking""" + return LayerBranch.objects.filter(layer=self.layer, branch=self.branch).exists() + + def vcs_before_commit_url(self): + if self.vcs_before_rev: + layerbranch = LayerBranch.objects.filter(layer=self.layer, branch=self.branch).first() + if layerbranch: + return layerbranch.commit_url(self.vcs_before_rev) + return None + + def vcs_after_commit_url(self): + if self.vcs_after_rev: + layerbranch = LayerBranch.objects.filter(layer=self.layer, branch=self.branch).first() + if layerbranch: + return layerbranch.commit_url(self.vcs_after_rev) + return None def save(self): warnings = 0
@@ -415,7 +437,7 @@ class LayerUpdate(models.Model): super(LayerUpdate, self).save() def __str__(self): - return "%s: %s: %s" % (self.layerbranch.layer.name, self.layerbranch.branch.name, self.started) + return "%s: %s: %s" % (self.layer.name, self.branch.name, self.started) class Recipe(models.Model):
layerindex/static/css/additional.css +10 -0
@@ -258,3 +258,13 @@ td.info { .hidden-select { display: none !important; } + +.pre-plain { + background-color: transparent; + border-style: none; + padding: 0; +} + +.td-pre { + background-color: #f5f5f5; +}
layerindex/templatetags/extrafilters.py +4 -0
@@ -12,6 +12,10 @@ def replace_commas(string): def squashspaces(strval): return utils.squashspaces(strval) [email protected] +def truncatesimple(strval, length): + return strval[:length] + @register.filter def timesince2(date, date2=None): # Based on http://www.didfinishlaunchingwithoptions.com/a-better-timesince-template-filter-for-django/
layerindex/update.py +29 -15
76 lines not on this page · raw message
layerindex/views.py +2 -2
@@ -373,7 +373,7 @@ class LayerDetailView(DetailView): context['distros'] = layerbranch.distro_set.order_by('name') context['appends'] = layerbranch.bbappend_set.order_by('filename') context['classes'] = layerbranch.bbclass_set.order_by('name') - context['updates'] = layerbranch.layerupdate_set.order_by('-started') + context['updates'] = LayerUpdate.objects.filter(layer=layerbranch.layer, branch=layerbranch.branch).order_by('-started') context['url_branch'] = self.kwargs['branch'] context['this_url_name'] = resolve(self.request.path_info).url_name if 'rrs' in settings.INSTALLED_APPS:
@@ -718,7 +718,7 @@ class UpdateDetailView(DetailView): context = super(UpdateDetailView, self).get_context_data(**kwargs) update = self.get_object() if update: - context['layerupdates'] = update.layerupdate_set.exclude(log__isnull=True).exclude(log__exact='') + context['layerupdates'] = update.layerupdate_set.order_by('-started') return context
templates/layerindex/layerupdate.html +17 -2
@@ -1,5 +1,6 @@ {% extends "base.html" %} {% load i18n %} +{% load extrafilters %} {% comment %}
@@ -11,13 +12,27 @@ {% endcomment %} <!-- -{% block title_append %} - {{ layerupdate.layerbranch.layer.name }} {{ layerupdate.layerbranch.branch.name }} - {{ layerupdate.started }} {% endblock %} +{% block title_append %} - {{ layerupdate.layer.name }} {{ layerupdate.branch.name }} - {{ layerupdate.started }} {% endblock %} --> {% block content %} {% autoescape on %} -<h2>{{ layerupdate.layerbranch.layer.name }} {{ layerupdate.layerbranch.branch.name }} - {{ layerupdate.started }}</h2> +<h2>{{ layerupdate.layer.name }} {{ layerupdate.branch.name }} - {{ layerupdate.started }} <small>({{ layerupdate.started|timesince2:layerupdate.finished }})</small></h2> + +{% if layerupdate.layerbranch_exists %} +{% if layerupdate.vcs_before_rev != layerupdate.vcs_after_rev %} +<p> + Updated + {% with before_url=layerupdate.vcs_before_commit_url after_url=layerupdate.vcs_after_commit_url %} + {% if before_url %}<a href="{{ before_url }}">{% endif %}{{ layerupdate.vcs_before_rev|truncatesimple:10 }}{% if before_url %}</a>{% endif %} → {% if after_url %}<a href="{{ after_url }}">{% endif %}{{ layerupdate.vcs_after_rev|truncatesimple:10 }}{% if after_url %}</a>{% endif %} + {% endwith %} + {% else %} + {{ layerupdate.vcs_before_rev|truncatesimple:10 }} → {{ layerupdate.vcs_after_rev|truncatesimple:10 }} +</p> +{% endif %} +{% endif %} + <pre>{{ layerupdate.log }}</pre>
templates/layerindex/updatedetail.html +30 -4
@@ -1,11 +1,12 @@ {% extends "base.html" %} {% load i18n %} +{% load extrafilters %} {% comment %} layerindex-web - update page - Copyright (C) 2016 Intel Corporation + Copyright (C) 2016, 2018 Intel Corporation Licensed under the MIT license, see COPYING.MIT for details {% endcomment %}
@@ -30,12 +31,37 @@ {% endif %} {% for layerupdate in layerupdates %} - <a href="{% url 'layer_item' layerupdate.layerbranch.branch.name layerupdate.layerbranch.layer.name %}"><h3>{{ layerupdate.layerbranch.layer.name }} {{ layerupdate.layerbranch.branch.name }}</h3></a> - <pre>{{ layerupdate.log }}</pre> + <table class="table table-bordered"> + <thead> + {% with layerbranch_exists=layerupdate.layerbranch_exists %} + <tr><td{% if layerupdate.errors > 0 or layerupdate.retcode != 0 %} class="error"{% elif layerupdate.warnings %} class="warning"{% endif %}> + {% if layerbranch_exists %}<a href="{% url 'layer_item' layerupdate.branch.name layerupdate.layer.name %}">{% endif %}<strong>{{ layerupdate.layer.name }} {{ layerupdate.branch.name }}</strong>{% if layerbranch_exists %}</a>{% endif %} + {% if layerupdate.vcs_before_rev != layerupdate.vcs_after_rev %} + <span class="pull-right"> + {% if layerbranch_exists %} + {% with before_url=layerupdate.vcs_before_commit_url after_url=layerupdate.vcs_after_commit_url %} + {% if before_url %}<a href="{{ before_url }}">{% endif %}{{ layerupdate.vcs_before_rev|truncatesimple:10 }}{% if before_url %}</a>{% endif %} → {% if after_url %}<a href="{{ after_url }}">{% endif %}{{ layerupdate.vcs_after_rev|truncatesimple:10 }}{% if after_url %}</a>{% endif %} + {% endwith %} + {% else %} + {{ layerupdate.vcs_before_rev|truncatesimple:10 }} → {{ layerupdate.vcs_after_rev|truncatesimple:10 }} + {% endif %} + </span> + {% endif %} + </td></tr> + {% endwith %} + </thead> + <tbody> + {% if layerupdate.log %} + <tr><td class="td-pre"> + <pre class="pre-scrollable pre-plain">{{ layerupdate.log }}</pre> + </td></tr> + {% endif %} + </tbody> + </table> {% endfor %} {% if not update.log and not layerupdates %} - <p>No messages</p> + <p>No messages or layer updates</p> {% endif %} {% if update.comparisonrecipeupdate_set.exists %}
Signature · 1 line
2.17.1
PATCH 4/5 update: ensure that exceptions during the update process are logged
In reply to Paul EggletonIf an exception occurred during the update then we were managing to save the update record, but we did not include the exception traceback in the log for the update. Catch the exception and log it which ensures it gets captured in the update record and still gets printed as well.
--- layerindex/update.py | 4 ++++ 1 file changed, 4 insertions(+)
layerindex/update.py +4 -0
@@ -540,6 +540,10 @@ def main(): except KeyboardInterrupt: logger.info('Update interrupted, exiting') sys.exit(254) + except Exception: + import traceback + logger.error(traceback.format_exc().rstrip()) + sys.exit(1) finally: update.log = ''.join(listhandler.read()) update.finished = datetime.now()
Signature · 1 line
2.17.1
PATCH 5/5 Include errors/warnings in main log in error/warning counts for updates
In reply to Paul EggletonWhen showing the error/warning counts for update records we need to include any errors/warnings that are shown only in the main update log, so we need to adjust how these are collected. Use a function rather than pure aggregation to give a bit more control, and a {% with ... %} block in the template to avoid the functions being called more than necessary.
--- layerindex/models.py | 8 ++++++++ layerindex/views.py | 2 +- templates/layerindex/updatelist.html | 6 ++++-- 3 files changed, 13 insertions(+), 3 deletions(-)
layerindex/models.py +8 -0
@@ -98,6 +98,14 @@ class Update(models.Model): task_id = models.CharField(max_length=50, blank=True, db_index=True) triggered_by = models.ForeignKey(User, blank=True, null=True, on_delete=models.SET_NULL) + def error_count(self): + sums = self.layerupdate_set.aggregate(errors=models.Sum('errors')) + return (sums['errors'] or 0) + self.log.count('ERROR:') + + def warning_count(self): + sums = self.layerupdate_set.aggregate(warnings=models.Sum('warnings')) + return (sums['warnings'] or 0) + self.log.count('WARNING:') + def __str__(self): return '%s' % self.started
layerindex/views.py +1 -1
@@ -708,7 +708,7 @@ class UpdateListView(ListView): paginate_by = 50 def get_queryset(self): - return Update.objects.all().order_by('-started').annotate(errors=Sum('layerupdate__errors'), warnings=Sum('layerupdate__warnings')) + return Update.objects.all().order_by('-started') class UpdateDetailView(DetailView):
templates/layerindex/updatelist.html +4 -2
@@ -35,12 +35,14 @@ <tbody> {% for update in updates %} + {% with error_count=update.error_count warning_count=update.warning_count %} <tr> <td><a href="{% url 'update' update.id %}">{{ update.started }}{% if update.reload %} (reload){% endif %}</a></td> <td>{% if update.finished %}{{ update.started|timesince2:update.finished }}{% else %}(in progress){% endif %}</td> - <td>{% if update.errors %}<span class="badge badge-important">{{ update.errors }}</span>{% endif %}</td> - <td>{% if update.warnings %}<span class="badge badge-warning">{{ update.warnings }}</span>{% endif %}</td> + <td>{% if error_count %}<span class="badge badge-important">{{ error_count }}</span>{% endif %}</td> + <td>{% if warning_count %}<span class="badge badge-warning">{{ warning_count }}</span>{% endif %}</td> </tr> + {% endwith %} {% endfor %} </tbody>
Signature · 1 line
2.17.1