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

JENKINS-36479 - clean up hard killed/deleted builds' locks #34

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@abayer
Copy link
Member

commented Jul 6, 2016

JENKINS-36479

  • Successfully clear locks from hard killed/deleted builds on next lock request.
  • Write tests for both hard kill and deleted build scenarios.
  • Decide whether this is sufficient or if we need to automatically clear those locks without needing a new lock request.

cc @reviewbybees, esp @jglick and @amuniz

abayer added some commits Jul 6, 2016

JENKINS-36479. First work on auto-clearing invalid locks.
When we try to lock, check if the resource is locked by either a null
build or a build that's no longer running. If so, unlock the resource
before continuing.

This probably needs a *lot* more work. It's just a first thought.
Initial test.
Note - test needs to be reworked to actually fail if it can't get the
lock. Right now it'll hang forever. I'll figure that out!
Timeout annotation didn't do anything, but that's ok!
JenkinsRule tests time out after 180 seconds anyway, it turns
out. Would prefer to have a shorter timeout than that, but it works,
so hey.
@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2016

Oh, and I've verified that the tests fail against master, so that's nice. =)

&& (lockingRun == null
|| !lockingRun.isBuilding()
|| lockingRun.getParent().getBuild(lockingRun.getId()) == null)) {
performUnlocking(r, lockingRun, inversePrecedence);

This comment has been minimized.

Copy link
@jglick

jglick Jul 6, 2016

Member

Should probably at least print a warning in this build’s log.

// Kill b1 hard.
b1.doKill();
story.j.waitForMessage("Hard kill!", b1);
story.j.waitForCompletion(b1);

This comment has been minimized.

Copy link
@jglick

jglick Jul 6, 2016

Member

Could use assertLogContains after waitForCompletion. But no matter.

// Make sure that b2 is blocked on b1's lock.
story.j.waitForMessage("[resource1] is locked, waiting...", b2);

Thread.sleep(2000);

This comment has been minimized.

Copy link
@jglick

jglick Jul 6, 2016

Member

Why?

This comment has been minimized.

Copy link
@abayer

abayer Jul 6, 2016

Author Member

I forget. Probably not needed. Lemme poke.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

I think you could use RunListener to immediately unblock waiting builds.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2016

I tried some experiments with RunListener, and, well, as far as I could tell, it's not firing for deleted-in-progress builds. More experiments ahoy!

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2016

Oh, yeah, it's not because AbstractBuild.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2016

Hmm - looks like changing all the AbstractBuild and AbstractProject references to Run and Job may do the trick without my change...

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

Huh? This code should fire for any Run.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

Oh you mean LockRunListener. Yeah fix that. It might need to have different cases for AbstractBuild vs. other Run, @amuniz would know.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2016

Gonna do an alternate pr for that approach and leave this open for the moment.

WorkflowRun b3 = p3.scheduleBuild2(0).waitForStart();

// Verify that b2 gets the lock.
story.j.waitForMessage("Lock acquired on [resource1]", b2);

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 7, 2016

Member

So, b2 is getting the lock because b3 release it before, right? Weird that a new build is required to unlock builds already waiting for the resource. Acceptable I guess, but weird.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2016

Closing in favor of #35

@abayer abayer closed this Jul 7, 2016

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.