#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: Type: task | Status: new Priority: major | Milestone: 0.4.4 Component: tests | Keywords: --------------------+------------------------------------------------------- This is taken from:[[BR]] https://fedorahosted.org/pipermail/autoqa-devel/2010-November/001306.html
I have imagined a situation where two updates are accepted into the -pending tag in quick succession. Thus we can end up with two depcheck tests running simultaneously. And the one that was started earlier can finish up later. It could then report incorrect result.
Do you have an idea what we can we do about that? Maybe we can somehow write down the state of the -pending tag in the time of starting the test, and if it is different from when the test is finished, we can just throw away the results? Just a wild idea.
Wwoods mentioned that he's working on --accepted option for depcheck, here is short IRC excerpt, hopefully he will provide more info soon:
{{{ (05:16:13 PM) wwoods: I'm working on a mailing list post about the "accepted" flag (05:19:13 PM) wwoods: the short answer is this: we mark packages as "accepted" when they pass depcheck (05:19:48 PM) wwoods: exactly *how* we mark the package is kind of an implementation detail - could be a koji tag or a local database of accepted package names or whatever (05:20:04 PM) wwoods: but since we're already planning to give +1 karma for packages that are accepted (05:21:02 PM) wwoods: then we can (I hope!) just check Bodhi for each thing in -pending (05:21:11 PM) wwoods: if it has +1, it was previously accepted (05:21:21 PM) wwoods: err - has a +1 from autoqa / depcheck (05:21:51 PM) wwoods: in the future we might want to query resultsdb instead, or use a second koji tag (05:22:39 PM) wwoods: I'll still write a (possibly longer) explanation for the list - or maybe a blog post }}}
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: Type: task | Status: new Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Comment (by jlaska):
Replying to [ticket:248 kparal]:
Do you have an idea what we can we do about that? Maybe we can somehow write down the state of the -pending tag in the time of starting the test, and if it is different from when the test is finished, we can just throw away the results? Just a wild idea.
iirc, the changes wwoods mentioned will mitigate the problem using a procedure similar to what you mention above. However, that only mitigates the problem, the potential exposure still exists. Long term, I think we need some sort of locking/semaphore mechanism when publishing results to bodhi.
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Changes (by wwoods):
* owner: => wwoods * status: new => assigned
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Comment (by wwoods):
There should only be four reasons the pending/accepted sets will change during a test:
1. New updates arrived in pending.
This implies that the test has been scheduled again for the new update. So we can cancel our run and let the newer run override it.
2. Another depcheck test completed, and moved updates from pending to accepted.
Our results are invalid because our input data has changed. Restart the test if there's still something to test.
3. Updates were untagged (obsoleted by maintainer or removed by rel-eng)
Same as above - Inputs changed, restart the test.
4. Updates were pushed live.
Again, our inputs have changed, so we must restart. Technically, though, if the ''only'' change to either set is that accepted updates were pushed live, the test results will come out the same. So in that case restarting is unnecessary, but also harmless.
So the test wrapper can work sort of like this: {{{ result = None tries_remaining = 10 (pending, accepted) = get_pending_and_accepted_sets() while pending and tries_remaining and (result is None): tries_remaining -= 1 result = do_test(pending, accepted)
# did anything change? old_pending = pending old_accepted = accepted (pending, accepted) = get_pending_and_accepted_sets() if (old_pending == pending) and (old_accepted == accepted): post_results(result) else: result = None # invalidate result if pending.difference(old_pending): # new updates means another test scheduled test_exit() if result is None: test_error("too many retries") }}}
{{{tries_remaining}}} should be unnecessary but it might be a good idea to have it in there, just in case.
So, as far as I can tell the only place we might need locking is around {{{get_pending_and_accepted_sets()}}} and {{{post_results(result)}}} - we can't have the sets changing after the check, but before/during the posting of results.
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Comment (by kparal):
The new watcher jskladan is working on should detect any change to -pending tag (new updates, removed updates, updates pushed live). Therefore it seems to me that the wrapper can be this simple:
{{{ def run_once(self, kojitag, timestamp, **kwargs): # kojitag is e.g. dist-f13-updates-pending # timestamp is e.g. 123456 (unix time), it's the last modification time of the kojitag; # it's important to have it passed over by the watcher, because it may have changed between # the watcher has run and this test has started
if not timestamp_still_actual(kojitag, timestamp): # kojitag changed in the meantime, that means another test is scheduled test_exit()
(pending, accepted) = get_pending_and_accepted_sets() result = do_test(pending, accepted)
if not timestamp_still_actual(kojitag, timestamp): # kojitag changed in the meantime, that means another test is scheduled test_exit()
report_result(result)
def timestamp_still_actual(kojitag, timestamp): current_timestamp = get_current_timestamp(kojitag) assert current_timestamp >= timestamp # just a sanity check return current_timestamp == timestamp }}}
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Comment (by wwoods):
As of commit 4ab4527 (on the depcheck branch), the depcheck wrapper should provide a list of packages split into 'accepted' and 'pending', and the depcheck test itself will handle them accordingly.
I think we just need to lay out a hack to force depcheck tests for the same release to run in serial, and we'll be ready to close this ticket.
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.4.4 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Comment (by jlaska):
I think we decided that only one depcheck test could be running per platform (aka arch), per distro, right? To accomplish that, how about something like the following in the depcheck/control.autoqa file?
{{{
# To serialize depcheck tests, we add the following label requirements: # label = platform -- architecture (handled by the autoqa script) # label = 'depcheck' -- system specifically tagged for depcheck tests # label = 'FNN' -- specific distro # TODO - if/when depcheck is able to serialize itself, these label # requirements can be loosened
labels = ['depcheck']
# From the 'envr', find the %{distro} value labels.append(autoqa_args.get('envr','').split('.')[-1]) }}}
We can also add the new ''depcheck'' label to the wiki documentation (https://fedoraproject.org/wiki/Managing_autotest_labels). Is this, or something similar, acceptable?
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: 0.5.0 Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Changes (by kparal):
* milestone: 0.4.4 => 0.5.0
Comment:
I'm moving this to the next release milestone. We will just assume that everything goes well until then.
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: Future tasks Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Changes (by kparal):
* milestone: 0.5.0 => Future tasks
#248: depcheck: simultaneous run may report incorrect results --------------------+------------------------------------------------------- Reporter: kparal | Owner: wwoods Type: task | Status: assigned Priority: major | Milestone: Depcheck Component: tests | Resolution: Keywords: | --------------------+------------------------------------------------------- Changes (by kparal):
* milestone: Future tasks => Depcheck
autoqa-devel@lists.fedorahosted.org
-
fedora-badges