Skip to content

Add Reconnect and Revert Idle Action#90

Merged
pjdarton merged 1 commit intojenkinsci:masterfrom
rsennewald:reconnectAndRevert
Mar 26, 2018
Merged

Add Reconnect and Revert Idle Action#90
pjdarton merged 1 commit intojenkinsci:masterfrom
rsennewald:reconnectAndRevert

Conversation

@rsennewald
Copy link
Copy Markdown
Contributor

  • Added idle action to vSphereCloudSlave and
    vSphereCloudProvisionedSlave for Reconnect and Revert.
  • When selected, the VM will reconnect it's computer
    to Jenkins when it's disconnected. When a snapshot
    is specified, it will be reverted to at startup
    (default functionality in the plugin).

- Added idle action to vSphereCloudSlave and
vSphereCloudProvisionedSlave for Reconnect and Revert.
- When selected, the VM will reconnect it's computer
to Jenkins when it's disconnected. When a snapshot
is specified, it will be reverted to at startup
(default functionality in the plugin).
@pjdarton
Copy link
Copy Markdown
Member

Hi,
While I am the last remaining plugin maintainer, the idleOption functionality isn't an area of the plugin I know much about (I'm interested in the Cloud functionality to create 100% dynamic slaves, rather than having static ones started and stopped), so I'm going to need some context to understand what you're trying to achieve here...

e.g. How does "Reconnect and Revert" differ from "Revert and Reset"? What's the point of one if we've got the other?
I don't really want to litter the plugin with many different ways of achieving the same result (unless we deprecate the old methods in favor of the new ones), so could you please explain what it is that you needed to achieve, why the old code didn't allow you to do this, and what this new code does in order to resolve that?
...and, if this new option is an all-round improvement from one of the old ones, we should be encouraging people to switch (by deprecating the old in favor of the new)?

Lastly, are you quite sure this code achieves what is claimed in the help text? It's not obvious (at least, it isn't obvious to me) how calling "slaveComputer.connect(false)" results in the revert happening.

@rsennewald
Copy link
Copy Markdown
Contributor Author

rsennewald commented Mar 13, 2018

Hi,
I was also a user of spinning up slaves dynamically through this plugin, but we need a fresh environment for every job and hence use the Disconnect After Limited Builds and set the value to 1. For both dynamic and static slaves, this allows you to perform a selected action when the slave is disconnected. What this PR does is reconnects the slave when it gets disconnected. When a connection attempt happens it will revert itself to a snapshot (if one is specified) automatically already built into the plugin as shown here. The idle action "Revert and Reset" does exactly that, it does a Revert and a Reset. Maybe there's a reason someone would want this, but it's not what I need. I need just a Revert, which is handled if I just reconnect the slave (which I also want to do).

I agree "littering" the plugin with different ways of achieving the same result is bad, but in this case I don't want to mess up someone else's workflow if they depend on any of these other idle actions or perhaps simply don't want the slave reconnected. If you think it'd be preferable to merely update the existing option for "Revert" to do a Reconnect and then Revert, I could do that. But again, I don't want to change functionality when a user may depend on it.

I'm completely confident the code achieves what is claimed, I'm using it right now. Look at the link above, which shows the snapshot revert being done in launch. I also indicated this is the case in the commit message and in the help message when you go to select it in the plugin's UI.

@pjdarton
Copy link
Copy Markdown
Member

Hmm, that's interesting - we also want a "fresh environment for every job" but in our setup that's done by destroying the VM once it's been used and spinning up a fresh one (and hence waiting for it to boot up) each time.
Sounds like your solution achieves much the same aims, but faster - I'll have to write myself a TODO item to take a look at this when I have time ;-)

...and on the subject of "time", I don't have much of it right now, so I can't easily do a merge and release at present.
i.e. I will merge this, but I can't commit to a "when" at present.

@rsennewald
Copy link
Copy Markdown
Contributor Author

rsennewald commented Mar 13, 2018 via email

@rsennewald
Copy link
Copy Markdown
Contributor Author

rsennewald commented Mar 13, 2018 via email

@pjdarton pjdarton merged commit 6f78bb0 into jenkinsci:master Mar 26, 2018
@pjdarton
Copy link
Copy Markdown
Member

@rsennewald FYI release 2.18 was uploaded a few minutes ago; it should get indexed in the next 12 hours or so and thus appear as an available update in your Jenkins server(s) within 48 hours.
These code changes were included so you should be safe to update and use the official release instead of a private build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants