Ticket #248 (assigned task)

Opened 3 years ago

Last modified 3 years ago

depcheck: simultaneous run may report incorrect results

Reported by: kparal Owned by: wwoods
Priority: major Milestone: Depcheck
Component: tests Keywords:
Cc: Blocked By:
Blocking:

Description

This is taken from:
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

Change History

comment:1 in reply to: ↑ description Changed 3 years ago by jlaska

Replying to 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.

comment:2 Changed 3 years ago by wwoods

  • Owner set to wwoods
  • Status changed from new to assigned

comment:3 Changed 3 years ago 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.

  1. 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.

  1. Updates were untagged (obsoleted by maintainer or removed by rel-eng)

Same as above - Inputs changed, restart the test.

  1. 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.

comment:4 Changed 3 years ago 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

comment:5 Changed 3 years ago 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.

comment:6 Changed 3 years ago 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?

comment:7 Changed 3 years ago by kparal

  • Milestone changed from 0.4.4 to 0.5.0

I'm moving this to the next release milestone. We will just assume that everything goes well until then.

comment:8 Changed 3 years ago by kparal

  • Milestone changed from 0.5.0 to Future tasks

comment:9 Changed 3 years ago by kparal

  • Milestone changed from Future tasks to Depcheck
Note: See TracTickets for help on using tickets.