Code review comment for lp:~kissiel/checkbox/fix-1501354-back-after-testplan-selected

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

+1

For some context:

09:58 <@zyga> kissiel: https://code.launchpad.net/~zyga/checkbox/sa-api-changes/+merge/273502
09:58 <@zyga> kissiel: have a look and tell me what you think
09:58 < kissiel> ack
09:58 <@zyga> kissiel: I'll propagate this down to all the apps if you ack
10:00 < kissiel> zyga: those pep8 got fixed on the branch I proposed yesterday
10:01 <@zyga> kissiel: heh
10:01 <@zyga> kissiel: where? let's merge em
10:01 < kissiel> zyga: https://code.launchpad.net/~kissiel/checkbox/fix-1501354-back-after-testplan-selected/+merge/273460
10:02 * zyga reads
10:02 <@zyga> kissiel: can you ask the sa if a test plan was set?
10:02 <@zyga> kissiel: I'd like to minimize the state that apps need to have
10:03 < kissiel> zyga: I tried that approach
10:03 < kissiel> zyga: and do all the logic in SA
10:03 <@zyga> kissiel: what is the timestamp for?
10:03 < kissiel> zyga: there are problems with resuming session
10:03 <@zyga> kissiel: I remember that
10:04 <@zyga> kissiel: what I mean specifically is why don't we expose sa._manager.test_plans (read only)
10:04 < kissiel> zyga: timestamp goes to the app-blob
10:04 <@zyga> kissiel: so that app doesn't need to keep this in this case
10:05 < kissiel> zyga: the thing is you can have boostrap already done
10:05 <@zyga> kissiel: we already track test plan selection
10:05 < kissiel> zyga: and we want to get rid of all the progress
10:05 <@zyga> kissiel: how does bootstrap prevents this?
10:05 < kissiel> zyga: and I wouldn't like to expose job-state-map
10:05 <@zyga> kissiel: I understand, you want a new session
10:05 <@zyga> kissiel: all I'm saying that you store something wa already store
10:06 < kissiel> zyga: ok, so once again, app stores it, because when resuming session app will have it set to none
10:06 <@zyga> kissiel: c-c stores test plan that sa already keeps track of there, that if could have been: if sa.selected_test_plan: ...
10:06 < kissiel> zyga: app differentiates between resuming and starting
10:06 < kissiel> sa does not
10:06 <@zyga> kissiel: are you sure?
10:06 < kissiel> zyga: spent quite a bit of time on that yesterday
10:06 <@zyga> kissiel: it pretty much seeems that sa._manager.test_plans is exactly that
10:07 <@zyga> kissiel: what am I missing then?
10:07 < kissiel> zyga: when the session is resumed, sa will start a session out of a checkpoint, right?
10:08 < kissiel> zyga: and when we change the testplan (by navigating back and forth) bootstrapping is done for each tp selected
10:08 < kissiel> and once you go back, you need to restat
10:08 < kissiel> when resuming, we couldn't see if we need to drop the progress already done or not
10:08 < kissiel> becase we 'just already have a test plan selected in sa'
10:11 <@zyga> kissiel: I need to look into this
10:11 <@zyga> kissiel: I think we should not need to re-boostrap the whole way in this case, it's just unfortunate effect of how the gui design and API doesn't let you do what you want
10:12 <@zyga> kissiel: I think that what we have now is coarse-grained for the problem (we nuke and restart which is expensive)
10:12 < kissiel> zyga: yeah, it would be nice if session could stay, and tp could change
10:13 <@zyga> kissiel: I think we do want a new session anyway but I'd like to be able to reuse boostrap across sessions where possible
10:13 < kissiel> zyga: most plainbox internal docs says that tp shouldn't change once established
10:13 <@zyga> kissiel: that's true
10:13 < kissiel> zyga: um, that's impossible
10:13 <@zyga> kissiel: but we could keep bootstrap results in an escrow service
10:13 < kissiel> zyga: different TPs have different BS tasks
10:13 <@zyga> kissiel: yes
10:14 <@zyga> kissiel: but if there's _any_ overlap (and most of the time there is) then we should not re-run those jobs
10:14 < kissiel> you want to keep the intersection ?
10:14 < kissiel> right
10:14 <@zyga> kissiel: exactly
10:14 <@zyga> kissiel: right now the user experience won't be good
10:14 < kissiel> yeah, that would be perfect^2
10:14 <@zyga> kissiel: I'll check the API and see what we could do
10:14 <@zyga> kissiel: I'm also worried by the size of the API :)
10:14 <@zyga> kissiel: less is more if we can pull it off :)
10:14 < kissiel> haha
10:15 <@zyga> kissiel: essentially because it should be easy to fit into one's head without swapping
10:15 <@zyga> kissiel: I think the mainline + forks approach should work
10:15 <@zyga> kissiel: as in, most of the time you call a->b->c
10:15 < kissiel> zyga: remember that some parts of the setup are cached and reused
10:15 < kissiel> zyga: the performance is not affected noticably
10:15 <@zyga> kissiel: but you can also call side methods at various times, though they don't change what other methods you can call later
10:15 <@zyga> kissiel: which parts?
10:16 < kissiel> going back to testplan selection and re-bootstrapping
10:16 <@zyga> kissiel: well we know about providers already
10:16 <@zyga> kissiel: but we'll re-run all the (expensive perhaps) jobs that are in the boostrap section
10:16 < kissiel> indeed
10:17 <@zyga> kissiel: perhaps we can use some of the session history patches
10:17 <@zyga> kissiel: do you remember those
10:17 < kissiel> um, tbh, nope
10:17 < kissiel> earlier checkpoints?
10:17 <@zyga> https://code.launchpad.net/~zyga/checkbox/session-changes
10:18 <@zyga> kissiel: idea: as long as the app is running, we can keep a temporary directory
10:18 <@zyga> kissiel: where we stash (move) results of abandoned boostrap stages
10:18 <@zyga> kissiel: and bootstrap will fetch existing results from there

review: Approve

« Back to merge proposal