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-32690 Make manually provision really work #195

Merged
merged 2 commits into from May 9, 2016

Conversation

Projects
None yet
2 participants
@francisu
Copy link
Member

commented May 7, 2016

No description provided.

@francisu

This comment has been minimized.

Copy link
Member Author

commented May 7, 2016

@johnnyshields - can you give this a test?

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 7, 2016

Sure, I can't seem to reproduce the error anymore tho...

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 7, 2016

@francisu I'm reading the code and it seems all this bitmask stuff is unnecessarily complex.

Would it be possible instead to remove both the allowCreateNew and forceCreateNew flags and just always attempt to provision in the instance?

@johnnyshields johnnyshields referenced this pull request May 8, 2016

Closed

add skipCheckInstance #189

@francisu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

@johnnyshields. The point of the original change is to allow a slave that it stopped to be started. If I removed the flags, that would not happen. Requested provisioning is different than when it's trying to find a node to run a job.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

I think the bitmask logic along with the generic variable name "options" makes this code hard to read. As long as there are only two possible flags, we should make everything to be booleans.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

Also should have some form of test here if possible, given that this logic is the source of a lot of noise on JIRA.

@francisu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

I generally don't like more than one boolean in a method, but I think
changing it to an EnumSet would make if more clear (and modern). Can you
live with that?

A test with this is difficult though since there is no means of testing
with the live EC2. I agree that we need tests, but its a much bigger
project to set something up to allow testing with EC2 (even a mock EC2). Also, a test is not
so much the issue with the this particular problem. This issue was caused by a deliberate design change
that I was unsure if it would be accepted by the community.

On Sun, May 8, 2016 at 2:55 AM, Johnny Shields notifications@github.com
wrote:

I think the bitmask logic along with a variable the generic name "options"
makes this code hard to read. As long as there are only two possible flags,
we should make everything to be booleans.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#195 (comment)

Cell +1 510 432 1589 (when in US)
Cell +33 7 83 72 48 66 (when in Europe)

@francisu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

Please see the changes to EnumSet, hope it's more clear.

@@ -571,7 +569,8 @@ private EC2AbstractSlave provisionOndemand(TaskListener listener, int options) t
}

if (existingInstance == null) {
if ((options & (PROVISION_ALLOW_CREATE_NEW | PROVISION_FORCE_CREATE_NEW)) == 0) {
if (!provisionOptions.contains(ProvisionOptions.FORCE_CREATE) &&
!provisionOptions.contains(ProvisionOptions.ALLOW_CREATE)) {

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields May 8, 2016

Contributor

This can be simplified to:

if (provisionOptions.isEmpty())
if (this.spotConfig != null) {
if (options != 0)
if (provisionOptions.contains(ProvisionOptions.ALLOW_CREATE) || provisionOptions.contains(ProvisionOptions.FORCE_CREATE))

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields May 8, 2016

Contributor

This can be be simplified to:

if (!provisionOptions.isEmpty())
@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 8, 2016

@francisu OK this looks good.

By the way, you could use if (!provisionOptions.isEmpty()), though the way you have it may be more explicit and future proof in case other Enum values are added so it's fine as-is I guess.

Green-light to merge!!

@francisu

This comment has been minimized.

Copy link
Member Author

commented May 8, 2016

Yes. That's exactly why I made that choice. In case other options were added. The point of the options is to be independent. I would like to have others test it before I merge it to make sure it actually fixes the problem.

Thanks for the review.

Sent from my iPhone

On May 8, 2016, at 09:20, Johnny Shields notifications@github.com wrote:

@francisu OK this looks good.

By the way, you could use if (!provisionOptions.isEmpty()), though the way you have it may be more explicit and future proof in case other Enum values are added so it's fine as-is I guess.

Green-light to merge!!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@francisu francisu merged commit e52a2ea into master May 9, 2016

1 check passed

Jenkins This pull request looks good
Details

@francisu francisu deleted the JENKINS-32690 branch May 9, 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.