Lock multiple resources using labels and quantities within the Pipeline lock step #26

Aeon512 · 2016-04-23T06:00:08Z

As a result of JENKINS-34268 and JENKINS-34273 I tried to extend the current Pipeline lock step implementation with an additional optional argument label: "LabelName" to specify that resources assigned to this label shall be requested as well as an additional optional argument quantity: 3 to request 3 such resources. (If left out, quantity defaults to 1.)

The locking and unlocking mechanisms had to be largely modified in order to deal with several resources in parallel (the current pipeline implementation can only handle a single resource within one step). One major modification was to move the list of queued context out of the actual LockableResource (since we are now dealing with several resources at once) and move this queue to the LockableResourcesManager instead.

Example Usage:

lock(label: "MyLabel", quantity: 2) {
    // body
}

Since this is my first Jenkins Plugin contribution (and the first time writing actual Java code) I'm open for any sorts of feedback, review comments or similar. The same holds actually for GitHub, so if there is anything I can improve doing forks, commits and pull requests, please let me know :)

As of today, 2016-04-22, this fork is up-to-date with the base repository, all unit-tests can be executed, and additional unit-tests for the newly added functionality (labels & quantities) have been added. The plugin worked also fine in my manual Jenkins tests.

…s-plugin

amuniz · 2016-04-25T10:19:31Z

src/main/java/org/jenkins/plugins/lockableresources/LockStep.java

 	}

 	@DataBoundSetter
 	public void setInversePrecedence(boolean inversePrecedence) {
 		this.inversePrecedence = inversePrecedence;
 	}

+	@DataBoundSetter
+	public void setResource(@Nullable String resource) {


More clear if you add @CheckForNull in the field rather that using @Nullable here.

amuniz · 2016-04-25T10:42:07Z

After a quick review (as it would need a deeper one), it seems that the static state is not persisted now (queued step contexts). Why the restart tests are passing? I don't know, probably something to fix in tests.

Aeon512 · 2016-04-25T11:57:10Z

@amuniz Thanks for the quick review. I have addressed most of your points except the still open issue regarding queued step contexts. Your comment regarding static state is beyond my understanding.

As mentioned in the comment above, I think the queued step context is correctly stored by issuing the save() call. The problem with storing the queued step contexts in individual resources is mostly, that by using labels you don't know apriori which resource will be finally used within the lock as their availability might change. Hence, you would have to store the queue within all possible resource and maintain these different lists which yield to a quite complicated solution imho.

Any ideas how to progress forward here?

…able-resources-plugin into JENKINS-34268-34273

amuniz · 2016-04-25T12:49:41Z

@Aeon512 I need to spend some time doing a deeper review. But at least, you will need to add a migration process to keep backward compatibility with previous installations. That migration would need to move all waiting contexts in the old structure to the new one. For that purpose the old field LockableResources#queuedContexts needs to be kept (not removed but @Deprecated). The migration process can be implemented by using @InitMilestone. Perhaps you can find some similar example here.

Aeon512 · 2016-04-25T13:45:24Z

@amuniz Thanks a lot for your work so far. I would really appreciate if you could take the time for doing a deeper review.

I have restored the old field LockableResource#queuedContexts as well as a new getter to obtain its data LockableResource#getQueuedContexts. Reharding the migration process I have tried to implement some code within BackwardCompatibility.java but honestly speaking that's only a shot from the hip since I don't really know how to verify / test this scenario.

Aeon512 · 2016-05-14T14:58:10Z

Any news how this will progress?

amuniz · 2016-05-16T08:54:07Z

@Aeon512 I didn't have time yet 😢 I'm a bit concerned about how this PR impacts freestyle compatibility and how the migration works, as we don't have too much automated tests for the former and none for the latter. So merging without manual testing is kind of dangerous (and they require time). Adding that tests is in my list, but feel free to add some if you feel good 😃

Migration tests are implemented in other plugins by using @LocalData, this is an example.

Aeon512 · 2016-06-04T09:07:11Z

@amuniz ping

talcloudshare · 2016-06-30T12:14:48Z

Any news? Specifically about JENKINS-34268?
Thanks

Aeon512 · 2016-06-30T17:27:07Z

@talcloudshare Unfortunately not. Sadly, I haven't heard from @amuniz
Working with a locally build version of the plugin for the moment.

amuniz · 2016-07-01T09:33:41Z

Sorry, I didn't have time to review this, will do it as soon as I can.

amuniz · 2016-07-07T08:53:02Z

src/main/java/org/jenkins/plugins/lockableresources/BackwardCompatibility.java

+
+    @Initializer(after = InitMilestone.JOB_LOADED)
+    public static void beforeInitMilestonePluginsStarted() {
+        LOG.info("\n\n LOCKABLE RESOURCES COMPATABILITY INIT \n\n");


Better at FINE level.

amuniz · 2016-07-07T08:58:50Z

The snippet generator is allowing to create:

lock(inversePrecedence: true, label: 'label1', quantity: 3, resource: 'resource1') {
    // some block
}

Without any warning. As label and resource parameters are mutual exclusive a validation should be shown, and a WARNING log in runtime saying that label will be ignored as resource is specified (not sure if that's the current behaviour, but it should be).

Aeon512 · 2016-07-09T17:59:33Z

@amuniz How can I mark parameters as mutual exclusive?

amuniz · 2016-07-11T09:38:30Z

Form validation and
Exception throwing at runtime if both are set

amuniz · 2016-07-11T09:44:09Z

src/main/java/org/jenkins/plugins/lockableresources/LockStep.java

-			throw new IllegalArgumentException("must specify resource");
-		}
-		this.resource = resource;
+	public LockStep() {


This is backward incompatible, but I fear there is no way to preserve it in this case.
Any call to to lock in the form lock ('my-resource') { ... } is going to fail.

@jglick any idea on how keep backward compatibility here?

This is preventing me to go ahead and merge. Unfortunately I don't see how keep backward compatibility here...

You are right that this breaks backward compatibility.
But only because the first pipeline implementation done by cloudbees only focused on the usage of resource names (and skipped the usage of labels and quantities all together).

One could argue that either

the first pipeline implementation by cloudbees shouldn't have been merged at all, because it focused only on 33% and did not provide an upgrade path (the problem you are looking at now)

that increasing the functionality for the pipeline implementation to the full feature set of the classic non-pipeline implementation is allowed to introduce breaking-changes since pipelines are still under heavy development.

Hence, I don't agree with your assumption that due to a incomplete (wrong) implementation done previously, we are not able to correct this implementation and do it right.
Sometimes a breaking change must happen. The sooner the better.

I may be missing something here but I don't think this has to be a breaking change. resource can be left in the constructor, and you can use @DataBoundSetter for the new fields. Both should work just fine.

Aeon512 · 2016-07-11T11:24:24Z

Do you have an example for a form validation in the pipeline snippet generator?
Couldn't get this to work.

Aeon512 · 2016-07-11T15:48:54Z

Anyone has an idea why the latest build got stuck? Didn't change much and locally everything works

amuniz · 2016-07-11T16:19:59Z

Do you have an example for a form validation in the pipeline snippet generator?

It's regular Jenkins (stapler) validation, this is an example.

Aeon512 · 2016-07-26T10:25:40Z

@amuniz I have added the form validation.
However, I have no idea why the automatic build failed. Locally everything works fine.

RemcoTukker · 2016-10-26T10:31:41Z

Looks great! One question though: looking through the PR, it seems there's no way of finding out which resources you actually locked when using a label (eg through a global variable), or did I just miss it?

I think it would be very useful, at least when your resources are physical things. How else will you decide which resource actually to use after you acquired a lock on it? (Any one that's free is not good enough, because it could be free while someone else has a lock on it..)

chancez · 2016-12-06T00:38:58Z

I'd love to see this. I currently need a better way than the built-in throttle functionality to limit the number of builds in a multibranch pipeline so that our Jenkins executors aren't overwhelmed with builds from a single repo with lots of branches/PRs.

bmannix · 2016-12-08T23:16:33Z

Is this PR still under development, or has it been abandoned?

Aeon512 · 2016-12-09T05:40:45Z

From my side this PR was finished. It builds locally without any problems and works fine since then.
Why the build on the Jenkins CI got stuck is unknown to me.
unfortunately Maintenance / Integration of Community PR seems to have a low priority at CloudBees...

amuniz · 2016-12-09T10:17:25Z

Basically it was not merged because of the backward incompatible change, but @Vlatombe is pointing out that there isn't such backward incompatibility if the original @DataboundConstructor is kept and a @DataboundSetter is added for the new field.

The snippet generator is going to request for resource field always though, but I think it's a minor issue compared to the number of current calls the constructor change would break.

This is in my list, I can make and test the change by myself.

amuniz · 2016-12-14T11:45:21Z

@Vlatombe I just checked the snippet generation is completely broken if we keep the original constructor.

@Aeon512 I'm trying to fix it (will send some commits on top of yours).

amuniz · 2016-12-15T08:29:34Z

A follow up on this PR has been filed as #42 - it is ready for review and merge.

Aeon512 added 3 commits April 22, 2016 22:15

Extended Pipeline Lock-Step with Label and Quantity

784635f

Extended Pipeline Lock-Step with Label and Quantity

7f0a799

Merge branch 'master' of https://github.com/Aeon512/lockable-resource…

e691502

…s-plugin

amuniz reviewed Apr 25, 2016
View reviewed changes

Added comments from @amuniz

38d37b3

Aeon512 added 3 commits April 25, 2016 14:25

Extended Pipeline Lock-Step with Label and Quantity

e8425b5

Added comments from @amuniz

6168098

Merge branch 'JENKINS-34268-34273' of https://github.com/Aeon512/lock…

be1b3e3

…able-resources-plugin into JENKINS-34268-34273

Added backward compatibility

1b2895a

Aeon512 added 3 commits May 13, 2016 21:13

Bugfixes for manual unlock

ba14555

Merge remote-tracking branch 'upstream/master' into JENKINS-34268-34273

480a4c5

Fixed typo in new Tests

cd1350c

Merge remote-tracking branch 'upstream/master' into JENKINS-34268-34273

8015f63

amuniz reviewed Jul 7, 2016
View reviewed changes

amuniz reviewed Jul 11, 2016
View reviewed changes

Aeon512 added 2 commits July 11, 2016 13:01

Fixed comments from @amuniz

cb7de51

Throw exception if both step and resource are specified

fe38863

Added FormValidation

f7da3c9

amuniz mentioned this pull request Sep 7, 2016

Allow multiple required resources on job configuration #38

Closed

Epsilon-B mentioned this pull request Oct 22, 2016

Merge and add resources capabilities selection as replacement for labels #36

Closed

chancez mentioned this pull request Dec 6, 2016

Is it possible to support locks up to a certain count? #39

Closed

amuniz closed this Dec 14, 2016

amuniz reopened this Dec 14, 2016

amuniz added a commit to amuniz/lockable-resources-plugin that referenced this pull request Dec 14, 2016

Follow up fix for jenkinsci#26

0026229

amuniz mentioned this pull request Dec 14, 2016

Follow up on #26 #42

Merged

3 tasks

amuniz closed this in #42 Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock multiple resources using labels and quantities within the Pipeline lock step #26

Lock multiple resources using labels and quantities within the Pipeline lock step #26

Aeon512 commented Apr 23, 2016 •

edited by amuniz

Loading

amuniz Apr 25, 2016

amuniz commented Apr 25, 2016

Aeon512 commented Apr 25, 2016

amuniz commented Apr 25, 2016

Aeon512 commented Apr 25, 2016

Aeon512 commented May 14, 2016

amuniz commented May 16, 2016

Aeon512 commented Jun 4, 2016

talcloudshare commented Jun 30, 2016

Aeon512 commented Jun 30, 2016

amuniz commented Jul 1, 2016

amuniz Jul 7, 2016

amuniz commented Jul 7, 2016

Aeon512 commented Jul 9, 2016

amuniz commented Jul 11, 2016

amuniz Jul 11, 2016

amuniz Sep 17, 2016

Aeon512 Sep 18, 2016

Vlatombe Dec 2, 2016

Aeon512 commented Jul 11, 2016

Aeon512 commented Jul 11, 2016

amuniz commented Jul 11, 2016

Aeon512 commented Jul 26, 2016

RemcoTukker commented Oct 26, 2016

chancez commented Dec 6, 2016

bmannix commented Dec 8, 2016

Aeon512 commented Dec 9, 2016

amuniz commented Dec 9, 2016

amuniz commented Dec 14, 2016

amuniz commented Dec 15, 2016

Lock multiple resources using labels and quantities within the Pipeline lock step #26

Lock multiple resources using labels and quantities within the Pipeline lock step #26

Conversation

Aeon512 commented Apr 23, 2016 • edited by amuniz Loading

amuniz Apr 25, 2016

Choose a reason for hiding this comment

amuniz commented Apr 25, 2016

Aeon512 commented Apr 25, 2016

amuniz commented Apr 25, 2016

Aeon512 commented Apr 25, 2016

Aeon512 commented May 14, 2016

amuniz commented May 16, 2016

Aeon512 commented Jun 4, 2016

talcloudshare commented Jun 30, 2016

Aeon512 commented Jun 30, 2016

amuniz commented Jul 1, 2016

amuniz Jul 7, 2016

Choose a reason for hiding this comment

amuniz commented Jul 7, 2016

Aeon512 commented Jul 9, 2016

amuniz commented Jul 11, 2016

amuniz Jul 11, 2016

Choose a reason for hiding this comment

amuniz Sep 17, 2016

Choose a reason for hiding this comment

Aeon512 Sep 18, 2016

Choose a reason for hiding this comment

Vlatombe Dec 2, 2016

Choose a reason for hiding this comment

Aeon512 commented Jul 11, 2016

Aeon512 commented Jul 11, 2016

amuniz commented Jul 11, 2016

Aeon512 commented Jul 26, 2016

RemcoTukker commented Oct 26, 2016

chancez commented Dec 6, 2016

bmannix commented Dec 8, 2016

Aeon512 commented Dec 9, 2016

amuniz commented Dec 9, 2016

amuniz commented Dec 14, 2016

amuniz commented Dec 15, 2016

Aeon512 commented Apr 23, 2016 •

edited by amuniz

Loading