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

Fix Jenkins 61702 #118

Merged
merged 4 commits into from
Sep 8, 2020
Merged

Fix Jenkins 61702 #118

merged 4 commits into from
Sep 8, 2020

Conversation

pjdarton
Copy link
Member

@pjdarton pjdarton commented Aug 18, 2020

After 2.21, we (accidentally) lost the ability to manually create vSphere nodes from the UI.
See https://issues.jenkins-ci.org/browse/JENKINS-61702
Oops.

This is an attempt to solve that.

It shouldn't need a descriptor visibility filter as it says
isInstantiable=false.
It can remove most descriptor code just by inheriting from
vSphereCloudSlave's descriptor instead.
It shouldn't need a descriptor visibility filter as should be visible.
It doesn't need to specify isInstantiable=true as that's the default.
Don't call getComputerLauncherDescriptors() as that doesn't exist
anymore, call
computerLauncherDescriptors(it) instead.
Uses the same code pattern as used in DumbSlave in the core code.
@pjdarton pjdarton added WIP Not ready for merge. bugfix Fixes a bug (that should be in the Jenkins JIRA) labels Aug 18, 2020
@pjdarton
Copy link
Member Author

The original jenkinsci build failed because of infrastructure reasons (one AWS node died shortly after the build had successfully built, but before Jenkins had let it go, so the node death flagged the build as a failure).
Fortunately, it did yield a hpi file, https://ci.jenkins.io/job/Plugins/job/vsphere-cloud-plugin/job/PR-118/lastCompletedBuild/

To anyone watching this, it's a good idea to check the details of a "failed" build, as they're often unrelated to the code being built.

@pjdarton
Copy link
Member Author

This is now awaiting "someone other than me" to test that it fixes the issue it's meant to.
I've tried it within my own environment and I've checked that it doesn't seem to have broken anything, and my own (shallow) testing suggests it might've fixed it, but as I wasn't suffering from the issue reported I can't confirm/deny it's 100% sorted now.

However, once we've got positive confirmation (either here or on the JIRA issue) from someone who was hitting this problem that they've tried it and it fixes the issue, I'll be happy to merge it it and it can then go into the next release.

@pjdarton
Copy link
Member Author

@jetersen Can you (also) test that this PR doesn't break JCasC?
This PR is reworking some of the code modified in #110 and so there's the potential that by fixing this issue I'll end up breaking JCasC.

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I no longer have access to a vSphere + Jenkins setup 😅

Comment on lines 110 to 115
public boolean isInstantiable() {
/*
* This type of agent can't be directly created by the user through the UI.
* The user defines a vSphere agent template and _that_ then creates these "on demand".
*/
return false;
Copy link
Member

@jetersen jetersen Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are fine as long as this is kept in. This is meant to prevent JCasC from removing dynamically provisioned nodes from vSphere.

@pjdarton
Copy link
Member Author

pjdarton commented Sep 8, 2020

Feedback from JENKINS-61702 is that this PR fixes the bug.
That means we're all clear to merge.

@pjdarton pjdarton merged commit 5ca51cd into jenkinsci:master Sep 8, 2020
@pjdarton pjdarton deleted the jenkins-61702 branch September 8, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug (that should be in the Jenkins JIRA) WIP Not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants