Differential D368

better explain ABORTED checks
ClosedPublic
Actions

Authored by kparal on May 6 2015, 12:35 PM.

Details

Reviewers
tflink
mkrizek
jskladan
Maniphest Tasks
T471: bodhi_comment_directive crashes when update is modified during task execution
Commits
rDEPCKedf28defd679: better explain ABORTED checks
Summary

This adds a longer explanation when some check is aborted because of a
missing build in an update (which means that the update got edited
during the check execution). A list of such missed builds is printed
out. This should make it easier for readers to understand what's going on.

A safety check for build2update() correctness was removed, it's better
to put these safety checks directly into build2update() method.

The explanation now looks like:

not ok - depcheck for Bodhi update NetworkManager-1.0.2-1.fc22,network-manager-applet-1.0.2-1.fc22,NetworkManager-openconnect-1.0.2-1.fc22,NetworkManager-openvpn-1.0.2-1.fc22,NetworkManager-vpnc-1.0.2-1.fc22,NetworkManager-openswan-1.0.2-1.fc22    # FAIL 
  ---
  arch: i386
  details:
    output: |-
      Update was incomplete - not all builds from this update have been tested. This can happen when the update is edited in Bodhi during the check execution.
      Builds which haven't been tested:
        NetworkManager-openconnect-1.0.2-1.fc22
  item: NetworkManager-1.0.2-1.fc22,network-manager-applet-1.0.2-1.fc22,NetworkManager-openconnect-1.0.2-1.fc22,NetworkManager-openvpn-1.0.2-1.fc22,NetworkManager-vpnc-1.0.2-1.fc22,NetworkManager-openswan-1.0.2-1.fc22
  outcome: ABORTED
  summary: NetworkManager-1.0.2-1.fc22,network-manager-applet-1.0.2-1.fc22,NetworkManager-openconnect-1.0.2-1.fc22,NetworkManager-openvpn-1.0.2-1.fc22,NetworkManager-vpnc-1.0.2-1.fc22,NetworkManager-openswan-1.0.2-1.fc22
    into F22 testing
  type: bodhi_update
  ...

The safety check (everything connected to untested_builds variable) was instead put directly into build2update(), it will be a separate diff against libtaskotron.

Test Plan

test suite passes. I also simulated this manually by commenting out koji tag download phase in task formula and instead downloading NetworkManager update manually (containing several builds). Then I removed RPMs for one of those builds. And run runtask with --setopt and set workdir to my custom location.

Diff Detail

Repository
rDEPCK task-depcheck
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kparal retitled this revision from to better explain ABORTED checks.May 6 2015, 12:35 PM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added reviewers: tflink, jskladan, mkrizek.
kparal added a comment.May 6 2015, 12:38 PM

The related libtaskotron patch is in D369.

zbyszek added a subscriber: zbyszek.May 6 2015, 1:31 PM

The text looks good. I'd change "update was incomplete" to "check was incomplete" though :)

jskladan accepted this revision.May 6 2015, 2:21 PM
This revision is now accepted and ready to land.May 6 2015, 2:21 PM
tflink added a comment.May 6 2015, 4:15 PM
In D368#6635, @zbyszek wrote:

The text looks good. I'd change "update was incomplete" to "check was incomplete" though :)

The check is complete. If I'm understanding this correctly, the problem happens when the check downloads rpms, executes and when it's gathering data to report results, the builds involved with an update at that time are not the same as those downloaded prior to execution.

I don't see how we could know that the update is edited during execution and the currently planned method for handling this (mark as ABORTED, don't report to bodhi) seems appropriate to me. It'd fix itself with the next depcheck run assuming the update wasn't edited again. That should be deployed to production before final freeze.

As for the warning message - how about something like:
"Update was incomplete as downloaded at execution time and not all builds have been tested. This can happen if the update is edited in Bodhi during check execution"

kparal added a comment.May 7 2015, 9:07 AM

If I'm understanding this correctly, the problem happens when the check downloads rpms, executes and when it's gathering data to report results, the builds involved with an update at that time are not the same as those downloaded prior to execution.

Yes. However, there might be possibly other reasons why would would end up with an incompletely tested update (including some internal errors).

As for the warning message - how about something like:
"Update was incomplete as downloaded at execution time and not all builds have been tested. This can happen if the update is edited in Bodhi during check execution"

I'll go with that.

Closed by commit rDEPCKedf28defd679: better explain ABORTED checks (authored by kparal). · Explain WhyMay 7 2015, 9:11 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mdepcheck/squash_results.py (40 lines)
Mtests/test_squash_results.py (47 lines)

Diff 980

View Options

depcheck/squash_results.py

Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Line(s)62 for build, update in pending_fail.items():
66 if update['title'] not in updates: 66​ if update['title'] not in updates:
67 updates[update['title']] = update 67​ updates[update['title']] = update
68 68​
69 updates_results = [] 69​ updates_results = []
70 for update_title, update in updates.items(): 70​ for update_title, update in updates.items():
71 cd = check.CheckDetail(update_title, check.ReportType.BODHI_UPDATE, outcome = 'PASSED') 71​ cd = check.CheckDetail(update_title, check.ReportType.BODHI_UPDATE, outcome = 'PASSED')
72 updates_results.append(cd) 72​ updates_results.append(cd)
73 73​
74 untested_builds = [] 74​ builds = sorted([build['nvr'] for build in update['builds']])
75 for build in update['builds']: 75​ for build in builds:
76 build = build['nvr'] ​
77 ​
78 # If build is in pending_fail either when it has no assigned ​
79 # update (that can not happen, as we traverse builds of ​
80 # a specific update), or when an update was incomplete ​
81 # (e.g. missing some builds). ​
82 # If an update is incomplete, all it's builds are put ​
83 # into the pending_fail, so this should fail early, ​
84 if build in pending_fail.keys(): 76​ if build in pending_fail.keys():
77​ # A build is in pending_fail either when it has no assigned
78​ # update (that can not happen, as we traverse builds of
79​ # a specific update), or when an update was incomplete
80​ # (e.g. missing some builds).
81​ # If an update is incomplete, all its builds are put
82​ # into the pending_fail, so this should get detected immediately.
85 cd.update_outcome('ABORTED') 83​ cd.update_outcome('ABORTED')
86 cd.store('Update was incomplete.') 84​ cd.store('Update was incomplete as downloaded at execution '
85​ 'time and not all builds have been tested. This can '
86​ 'happen if the update is edited in Bodhi during check '
87​ 'execution.', printout=False)
88​ expected = set(builds)
89​ tested = set([bld for bld, upd in pending_fail.items() if upd is update])
90​ missing = expected.difference(tested)
91​ cd.store("Builds which haven't been tested:", printout=False)
92​ for bld in sorted(missing):
93​ cd.store(' %s' % bld, printout=False)
87 break 94​ break
88 95​
89 if build not in pending_ok.keys(): ​
90 cd.update_outcome('ABORTED') ​
91 untested_builds.append(build) ​
92 log.logger.warning("Untested build %s for update %s" % (build, update_title)) ​
93 continue ​
94 ​
95 if results_nvr[build]['result'] != Pass: 96​ if results_nvr[build]['result'] != Pass:
96 cd.update_outcome(results_nvr[build]['result']) 97​ cd.update_outcome(results_nvr[build]['result'])
97 cd.store('Build %s failed depcheck' % build) 98​ cd.store('Build %s failed depcheck' % build, printout=False)
98 cd.output.extend(results_nvr[build]['details']) 99​ cd.output.extend(results_nvr[build]['details'])
99 100​
100 if untested_builds: ​
101 cd.store('Update containted untested builds: %s' % untested_builds) ​
102 ​
103 # set summary 101​ # set summary
104 if update['request']: 102​ if update['request']:
105 target = 'into %s %s' % (update['release']['name'], 103​ target = 'into %s %s' % (update['release']['name'],
106 update['request']) 104​ update['request'])
107 else: 105​ else:
108 target = 'in %s %s' % (update['release']['name'], 106​ target = 'in %s %s' % (update['release']['name'],
109 update['status']) 107​ update['status'])
110 cd.summary = '%s %s' % (update_title, target) 108​ cd.summary = '%s %s' % (update_title, target)
Show All 39 Lines
View Options

tests/test_squash_results.py

Show First 20 Lines • Show All 61 Lines • ▼ Show 20 Line(s)47 def test__squash_builds_to_updates_all_ok(self):
62 build_2_update = ( 62​ build_2_update = (
63 { 63​ {
64 'nvr1': update, 64​ 'nvr1': update,
65 'nvr2': update, 65​ 'nvr2': update,
66 }, {}) 66​ }, {})
67 67​
68 fake_bodhi = Dingus(build2update__returns = build_2_update) 68​ fake_bodhi = Dingus(build2update__returns = build_2_update)
69 69​
70 outcome = _squash_builds_to_updates(run_results, fake_bodhi) 70​ cds = _squash_builds_to_updates(run_results, fake_bodhi)
71 assert len(outcome) == 1 71​ assert len(cds) == 1
72 72​
73 detail = outcome[0] 73​ detail = cds[0]
74 assert detail.item == update['title'] 74​ assert detail.item == update['title']
75 assert detail.outcome == 'PASSED' 75​ assert detail.outcome == 'PASSED'
76 76​
77 def test__squash_builds_to_updates_build_failed(self): 77​ def test__squash_builds_to_updates_build_failed(self):
78 """ 78​ """
79 Checks whether __squash_builds_to_updates provides correct output 79​ Checks whether __squash_builds_to_updates provides correct output
80 when some of the builds in an update failed depchecking. 80​ when some of the builds in an update failed depchecking.
81 """ 81​ """
Show All 9 Lines
91 build_2_update = ( 91​ build_2_update = (
92 { 92​ {
93 'nvr1': update, 93​ 'nvr1': update,
94 'nvr2': update, 94​ 'nvr2': update,
95 }, {}) 95​ }, {})
96 96​
97 fake_bodhi = Dingus(build2update__returns = build_2_update) 97​ fake_bodhi = Dingus(build2update__returns = build_2_update)
98 98​
99 outcome = _squash_builds_to_updates(run_results, fake_bodhi) 99​ cds = _squash_builds_to_updates(run_results, fake_bodhi)
100 assert len(outcome) == 1 100​ assert len(cds) == 1
101 101​
102 detail = outcome[0] 102​ detail = cds[0]
103 assert detail.item == update['title'] 103​ assert detail.item == update['title']
104 assert detail.outcome == 'FAILED' 104​ assert detail.outcome == 'FAILED'
105 assert '### TEST @@@' in detail.output 105​ assert '### TEST @@@' in detail.output
106 106​
107 def test__squash_builds_to_updates_incomplete_update(self): 107​ def test__squash_builds_to_updates_incomplete_update(self):
108 """ 108​ """
109 Checks whether __squash_builds_to_updates provides correct output 109​ Checks whether __squash_builds_to_updates provides correct output
110 when some update is incomplete (i.e. present in the second return-value 110​ when some update is incomplete (i.e. present in the second return-value
111 from the build2update method). 111​ from the build2update method).
112 """ 112​ """
113 run_results = { 113​ run_results = {
114 'nvr1': {'result': Pass, 'details': []}, 114​ 'nvr1': {'result': Pass, 'details': []},
115 'nvr2': {'result': Pass, 'details': []}, 115​ 'nvr2': {'result': Pass, 'details': []},
116 } 116​ }
117 update = {'title': 'update_name', 117​ update = {'title': 'update_name',
118 'builds': [{'nvr': k} for k in run_results.keys()], 118​ 'builds': [{'nvr': k} for k in run_results.keys()],
119 'release': {'name': 'F20'}, 119​ 'release': {'name': 'F20'},
120 'request': 'stable', 120​ 'request': 'stable',
121 } 121​ }
122​ update['builds'].append({'nvr':'nvr3'})
122 build_2_update = ({}, 123​ build_2_update = ({},
123 { 124​ {
124 'nvr1': update, 125​ 'nvr1': update,
125 'nvr2': update, 126​ 'nvr2': update,
127​ 'nvr3': update,
126 }) 128​ })
127 129​
128 fake_bodhi = Dingus(build2update__returns = build_2_update) 130​ fake_bodhi = Dingus(build2update__returns = build_2_update)
129 131​
130 outcome = _squash_builds_to_updates(run_results, fake_bodhi) 132​ cds = _squash_builds_to_updates(run_results, fake_bodhi)
131 assert len(outcome) == 1 133​ assert len(cds) == 1
132 134​
133 detail = outcome[0] 135​ detail = cds[0]
134 assert detail.item == update['title'] 136​ assert detail.item == update['title']
135 assert detail.outcome == 'ABORTED' 137​ assert detail.outcome == 'ABORTED'
136 assert 'Update was incomplete.' in detail.output 138​ assert any('Update was incomplete' in line for line in detail.output)
137 139​
138 def test__squash_builds_to_updates_missing_build(self): 140​ def test__squash_builds_to_updates_missing_update(self):
139 """ 141​ """
140 Checks whether __squash_builds_to_updates provides correct output 142​ Checks whether __squash_builds_to_updates provides correct output
141 when update is missing a result for one of its builds. 143​ when there is no update for a certain build.
142 """ 144​ """
143 run_results = { 145​ run_results = {
144 'nvr1': {'result': Pass, 'details': []}, 146​ 'nvr1': {'result': Pass, 'details': []},
145 'nvr2': {'result': Pass, 'details': []}, 147​ 'nvr2': {'result': Pass, 'details': []},
146 } 148​ }
147 update = {'title': 'update_name', 149​ update = {'title': 'update_name',
148 'builds': [{'nvr': k} for k in run_results.keys()], 150​ 'builds': [{'nvr': 'nvr1'}],
149 'release': {'name': 'F20'}, 151​ 'release': {'name': 'F20'},
150 'request': 'stable', 152​ 'request': 'stable',
151 } 153​ }
152 update['builds'].append({'nvr':'nvr3'}) ​
153 build_2_update = ( 154​ build_2_update = (
154 { 155​ { 'nvr1': update, },
155 'nvr1': update, 156​ { 'nvr2': None, },
156 'nvr2': update, 157​ )
157 }, {}) ​
158 158​
159 fake_bodhi = Dingus(build2update__returns = build_2_update) 159​ fake_bodhi = Dingus(build2update__returns = build_2_update)
160 160​
161 outcome = _squash_builds_to_updates(run_results, fake_bodhi) 161​ cds = _squash_builds_to_updates(run_results, fake_bodhi)
162 assert len(outcome) == 1 162​ # 'nvr2' got ignored (just logged), so only one CheckDetail got returned
163 163​ assert len(cds) == 1
164 detail = outcome[0] 164​ detail = cds[0]
165 assert detail.item == update['title'] 165​ assert detail.item == update['title']
166 assert detail.outcome == 'ABORTED' 166​ assert detail.outcome == 'PASSED'
167 assert "Update containted untested builds: ['nvr3']" in detail.output ​
168 167​