Add additional parameters to reconfiguring VMs #103
Conversation
There was a problem hiding this comment.
I'm guessing that you're coming at this from a pipeline, otherwise you'd've noticed the lack of support in the Jenkins job configuration WebUI. You'll also need to :
- edit the accompanying
config.jellyfile so that this field appears in the configuration WebUI. - add some help (a file called
help-cpuLimitMHz.html, in the same folder ashelp-coresPerSocket.htmletc) explaining what the field is for, what values are permitted and what units its in. - add a
public FormValidation doCheckCpuLimitMHz(@QueryParameter String value)method to theReconfigureCpuDescriptorso that the field is properly validated (e.g. a user who types "foo" into the field should be told that that isn't an integer).
| @@ -34,11 +35,16 @@ | |||
|
|
|||
| private final String cpuCores; | |||
| private final String coresPerSocket; | |||
| private final String cpuLimitMHz; | |||
| private final ResourceAllocationInfo cpuReservation; | |||
There was a problem hiding this comment.
You've introduced data duplication here.
EITHER store the cpuLimitMHz only OR store the cpuReservation only ... but do not store a cpuReservation that (always) contains a limit of cpuLimitMHz.
Personally, I'd recommend having only a field of cpuLimitMHz and pushing the code that creates a ResourceAllocationInfo down to reconfigureCPU where it's used.
|
|
||
| @DataBoundConstructor | ||
| public ReconfigureCpu(String cpuCores, String coresPerSocket) throws VSphereException { | ||
| public ReconfigureCpu(String cpuCores, String coresPerSocket, String cpuLimitMHz) throws VSphereException { |
There was a problem hiding this comment.
Don't change the constructor - that can break things for existing users, especially folks who're using pipeline builds.
Create a @DataBoundSetter method setCpuLimitMHz() instead.
(Yes, this will mean that cpuLimitMHz can't be final - just put a comment in saying /* almost final */)
FYI while this plugin pre-dates such a practice being widely known, these days it's considered Jenkins "best practice" to have a constructor that only take mandatory arguments (and to have as few of those as possible) with everything else being set by data bound set methods. This makes it much easier for code that doesn't know or care about newly introduced optional fields to continue to work.
| @@ -75,6 +81,8 @@ public boolean reconfigureCPU (final Run<?, ?> run, final Launcher launcher, fin | |||
| PrintStream jLogger = listener.getLogger(); | |||
| String expandedCPUCores = cpuCores; | |||
| String expandedCoresPerSocket = coresPerSocket; | |||
| ResourceAllocationInfo resAllInfo = cpuReservation; | |||
There was a problem hiding this comment.
Replace with something like
ResourceAllocationInfo resAllInfo = new ResourceAllocationInfo();
cpuReservation.setReservation((long)Integer.valueOf(getCpuLimitMHz()));
...except that you'll also need to make sure that, if cpuLimitMHz is null or empty (which it can be for folks who aren't using this new capability), you don't call spec.setCpuAllocation(resAllInfo) at all.
| @@ -91,6 +99,7 @@ public boolean reconfigureCPU (final Run<?, ?> run, final Launcher launcher, fin | |||
| VSphereLogger.vsLogger(jLogger, "Preparing reconfigure: CPU"); | |||
| spec.setNumCPUs(Integer.valueOf(expandedCPUCores)); | |||
| spec.setNumCoresPerSocket(Integer.valueOf(expandedCoresPerSocket)); | |||
| spec.setCpuAllocation(resAllInfo); | |||
There was a problem hiding this comment.
If the user hasn't set cpuLimitMHz then spec.setCpuAllocation(...) should not be called.
i.e. preserve existing default behavior.
|
@nakkapeddi I'd like to do a new release before the middle of next month (there are other changes awaiting release) so time is running out to resolve the changes requested... If anything is unclear, just ask; I'm happy to advise. |
|
This PR has been inactive for some time now... I'm not keen on having PRs open for ages - I'd prefer that we either work on them until they're complete and get them merged in, or we close them if that's not going to happen. |
Howdy folks,
According to the vSphere WSSDK docs for vim.vm.ConfigSpec, you can setup reservations and limits for both CPU and memory.
In the course of my work, I've had to specify a CPU reservation limit for my linked clone build agent VMs, and ended up forking the plugin to achieve this. Are there any plans to support the additional config changes to the spec? I rewrote the existing default constructor, but ideally I'd create a new one to support the additional config parameters. Thoughts?