Skip to content

Add additional parameters to reconfiguring VMs #103

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add additional parameters to reconfiguring VMs #103

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/main/java/org/jenkinsci/plugins/vsphere/builders/ReconfigureCpu.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.jenkinsci.plugins.vsphere.tools.VSphereLogger;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import com.vmware.vim25.ResourceAllocationInfo;

import javax.annotation.Nonnull;
import javax.servlet.ServletException;
Expand All @@ -34,11 +35,16 @@ public class ReconfigureCpu extends ReconfigureStep {

private final String cpuCores;
private final String coresPerSocket;
private final String cpuLimitMHz;
private final ResourceAllocationInfo cpuReservation;
Copy link
Member

@pjdarton pjdarton Jul 8, 2019

Choose a reason for hiding this comment

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

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 {
Copy link
Member

@pjdarton pjdarton Jul 8, 2019

Choose a reason for hiding this comment

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

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.

this.cpuCores = cpuCores;
this.coresPerSocket = coresPerSocket;
this.cpuLimitMHz = cpuLimitMHz;
this.cpuReservation = new ResourceAllocationInfo();
this.cpuReservation.setReservation((long)Integer.valueOf(this.cpuLimitMHz));
}

public String getCpuCores() {
Expand Down Expand Up @@ -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;
Copy link
Member

@pjdarton pjdarton Jul 8, 2019

Choose a reason for hiding this comment

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

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.


EnvVars env;
try {
env = run.getEnvironment(listener);
Expand 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);
Copy link
Member

@pjdarton pjdarton Jul 8, 2019

Choose a reason for hiding this comment

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

If the user hasn't set cpuLimitMHz then spec.setCpuAllocation(...) should not be called.
i.e. preserve existing default behavior.


VSphereLogger.vsLogger(jLogger, "Finished!");
return true;
Expand Down