Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract queuedness of runs in the REST API #968

Merged
merged 18 commits into from Apr 19, 2017

Conversation

5 participants
@i386
Copy link
Contributor

commented Apr 18, 2017

All look upon this refactor and despair no more
Holy be the client, for now it is blessed with innocence
Of architectural sins visited by the father
For now what is run is queued
And for what is run is queued no more

Description

Removed all the knowledge of queued items from the client so that non-pipeline queued runs isn't so broken anymore. Bonus feature: causeOfBlockage JENKINS-41540.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
  • Ran Acceptance Test Harness against PR changes.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@i386 i386 requested review from vivek, imeredith and cliffmeyers Apr 18, 2017

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

Updated unit tests now that semantics of starting runs has changed and /activities is no more.

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@vivek
Copy link
Collaborator

left a comment

@i386 Going over proposed changes, it looks confusing. I can see these changes driven by the fact client has to deal with different set of apis /activities, /queue and /runs, where /activities is superset of run and queue items.

My main concern with this PR is, it overloads /runs/:id API with queued items and assumes expected run id to be a guaranteed run id, which it's not. Its more of guess and can disappear entirely based on queue activities. So a Run lookup based on expectedBuildNumber can be erroneous as client may not receive what they are expecting. There is no guarantee that expected build number will be the same number as run id after a queued item moves to run state. That's the main reason we had separate run and queue API

This is messy problem, right thing is for Jenkins to generate guaranteed run id when a queue item is created created but thats not happening any time soon:(

@@ -83,7 +82,7 @@ public BlueRunState getStateObj() {
} else if(!run.isLogUpdated()){
return BlueRunState.FINISHED;
} else {
return BlueRunState.QUEUED;

This comment has been minimized.

Copy link
@vivek

vivek Apr 18, 2017

Collaborator

I thought QUEUED is right state to return, why change to RUNNING? Also if RUNNING is really the right state to return the conditions could be further simplified.

This comment has been minimized.

Copy link
@i386

i386 Apr 18, 2017

Author Contributor

Agreed - I need to look into this. The non-pipeline version of this should never return queued as it will receive a BlueQueuedRun instead of this model

}

@Override
public String getId() {

This comment has been minimized.

Copy link
@vivek

vivek Apr 18, 2017

Collaborator

Its quasi run id, its not run yet. This can be confusing to client as expected run id, might be reused and is simply a guess. Better to call it out what it is, that is getExpectedRunId() and client should treat it as such. Otherwise I am pretty sure someone later on is going to re-write this whole thing again:)

throw new NotFoundException(String.format("Run %s not found in organization %s and pipeline %s",
name, pipeline.getOrganization(), job.getName()));
}
for (BlueQueueItem item : QueueContainerImpl.getQueuedItems(job)) {

This comment has been minimized.

Copy link
@vivek

vivek Apr 18, 2017

Collaborator

Note: QueueContainerImpl.getQueuedItems(job) returns active queued items. If a queue item was aborted, it won't be returned here. Maybe its not any different than current implementation.

This comment has been minimized.

Copy link
@i386

i386 Apr 18, 2017

Author Contributor

the client does not handle this today and that comes at a high price of complexity there. It depends where we want to wear the ugliness.

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

assumes expected run id to be a guaranteed run id, which it's not

My argument for this change is that this is the exact same assumption the client makes several times in the frontend and fails to handle properly.

If we take the opinion that in the future the architecture of the queue changes and items have guaranteed run numbers (numbers are no longer "expected") then my proposed way of handling queued items would continue to work and the client would be none the wiser.

@i386 i386 force-pushed the topic/sins-of-the-queue branch from 0a96a96 to 769dfa9 Apr 18, 2017

}

@Exported(name = QUEUE_ID)
public String getQueueId() {

This comment has been minimized.

Copy link
@i386

i386 Apr 18, 2017

Author Contributor

@vivek to allay your concern the client can always find out if the Run is actually a queued run by the presence of this property in the fetch response.

@vivek
Copy link
Collaborator

left a comment

Discussed with @i386, client has enough information to differentiate a run object from queued one. It also clears the path forward where when jenkins core fixes mismatch of queued id vs run id. 🐝 from me on backend changes as long as test/ATH passes.

@i386

This comment has been minimized.

@i386 i386 force-pushed the topic/sins-of-the-queue branch from 76a089e to 11ba074 Apr 19, 2017

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

Found what broke the ATH test. Simple enough to fix :)

@michaelneale

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@i386 what is the conflict with what is going on in this branch: #839 - is there a way to tell if there is overlap?

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@michaelneale there was a small conflict merging this with @scherler's karaoke reloaded branch around displaying the causeOfBlockage message instead of "waiting for run to start" but I don't mind if that gets stomped

@michaelneale

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Ah, so the ATH failure(s) was legit.

I ran commitMessages.js locally, and noticed activity tab was not updating when a new run happened:

which it is rightfully picking up as a regression (you only see the first run, not the second) - so clealy a 🐛 there which may be tripping up other things. no console errors, and pipeline did run, perhaps events being dropped on the floor?

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@michaelneale yeah there are a few events that we receive that we drop that actually need to be pumped to the client to make things work. Matter of rearranging a switch statement...

@michaelneale

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

AH, that would explain it then.

@i386

This comment has been minimized.

@i386 i386 removed the request for review from imeredith Apr 19, 2017

@michaelneale

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@vivek would you mind giving the java changes a once over again? There were a few things that I had to change along the way.

@michaelneale

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

This seems to work nicely - and ATH is happy. I tried it with queuing and so on.

Any specific areas we should look for @i386 to test this more?

@cliffmeyers was going to take a quite look (code only). Nice!

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@michaelneale @cliffmeyers heres a list of things to try for both multibranch and freestyle:

  • Run from dashboard until completed
  • Run from dashboard then stop
  • Run from activity page (freestyle)
  • Run from activity page then stop
  • Run from branch tab until completed (switching back to activity often)
  • Run from branch tab then stop
  • Rerun a freestyle run
  • Rerun a branch run
  • Keep hitting the run button on freestyle projects
  • Keep starting and stopping stuff
@cliffmeyers
Copy link
Contributor

left a comment

Good set of changes here to simplify down the handling of the queue that was always messy. I thought I spotted a bug in updating of runs but I see it was fixed in a64a503 - nice! Two other tiny nits, leave it up to you if you want to address. 🐝

@@ -20,7 +20,7 @@ export default {

activities(organization, pipeline, branch) {

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Apr 19, 2017

Contributor

Tiny nit, perhaps rename this method too.

This comment has been minimized.

Copy link
@i386

i386 Apr 19, 2017

Author Contributor

Updated but I will leave ActivityService as is.

@@ -74,7 +62,7 @@ export class ActivityService extends BunkerService {
* @param {boolean} overrideQueuedState Hack to make SSE work. Not use unless you know what you are doing!!!

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Apr 19, 2017

Contributor

Tiny nit: can remove this.

This comment has been minimized.

Copy link
@i386

i386 Apr 19, 2017

Author Contributor

Removed

}
this.pipelineService.updateLatestRun(d);
});
}

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Apr 19, 2017

Contributor

This code here seems responsible for adding a new run to the pager which holds runs for that job. Can it be safely removed?

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Apr 19, 2017

Contributor

Ah, looks like this same logic was being done in _updateRun anyways. Good to remove it, then.

This comment has been minimized.

Copy link
@i386

i386 Apr 19, 2017

Author Contributor

@cliffmeyers yep - all that mutation is in a single place now :)

@i386

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

Woohoo thanks @cliffmeyers!

How does one go about releasing corejs?

@@ -3,29 +3,28 @@ import ReactDOM from 'react-dom';
import Extensions from '@jenkins-cd/js-extensions';

import {
buildClassicInputUrl,

This comment has been minimized.

Copy link
@scherler

scherler Apr 19, 2017

Collaborator

We will have fun when merging karaoke and this file

This comment has been minimized.

Copy link
@i386

i386 Apr 19, 2017

Author Contributor

Feel free to stomp this file - Ill come back and update the changes to QueuedState when your branch is merged

This comment has been minimized.

Copy link
@michaelneale

michaelneale Apr 19, 2017

Member

@scherler yeah - @i386 had a look - the karaoke branch side can stomp it and @i386 can re-apply the stuff.

@i386 i386 force-pushed the topic/sins-of-the-queue branch from 759e93e to e76d492 Apr 19, 2017

i386 added some commits Apr 19, 2017

@i386 i386 merged commit 41936a8 into master Apr 19, 2017

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@i386 i386 deleted the topic/sins-of-the-queue branch Apr 19, 2017

i386 added a commit that referenced this pull request Apr 20, 2017

@i386 i386 referenced this pull request Apr 20, 2017

Merged

[FIX-JENKINS-38523] mobx driven rendering of karaoke #839

5 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.