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
nakkapeddi wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add additional parameters to reconfiguring VMs #103

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f15f1fb
Add param to enable CPU reservation
nakkapeddi Mar 9, 2019
620b0a1
Merge branch 'master' into master
jimklimov Mar 12, 2026
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 edu.umd.cs.findbugs.annotations.NonNull;
import jakarta.servlet.ServletException;
Expand All @@ -34,63 +35,71 @@

private final String cpuCores;
private final String coresPerSocket;
private final String cpuLimitMHz;
private final ResourceAllocationInfo cpuReservation;
Copy link
Copy Markdown
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
Copy Markdown
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() {
return cpuCores;
}

public String getCoresPerSocket() {
return coresPerSocket;
}

@Override
public void perform(@NonNull Run<?, ?> run, @NonNull FilePath filePath, @NonNull Launcher launcher, @NonNull TaskListener listener) throws InterruptedException, IOException {
try {
reconfigureCPU(run, launcher, listener);
} catch (Exception e) {
throw new AbortException(e.getMessage());
}
}

@Override
public boolean perform(final AbstractBuild<?, ?> build, final Launcher launcher, final BuildListener listener) {
boolean retVal = false;
try {
retVal = reconfigureCPU(build, launcher, listener);
} catch (Exception e) {
e.printStackTrace();
}
return retVal;
//TODO throw AbortException instead of returning value
}

public boolean reconfigureCPU (final Run<?, ?> run, final Launcher launcher, final TaskListener listener) throws VSphereException {

PrintStream jLogger = listener.getLogger();
String expandedCPUCores = cpuCores;
String expandedCoresPerSocket = coresPerSocket;
ResourceAllocationInfo resAllInfo = cpuReservation;
Copy link
Copy Markdown
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);
} catch (Exception e) {
throw new VSphereException(e);
}

if (run instanceof AbstractBuild) {
env.overrideAll(((AbstractBuild) run).getBuildVariables()); // Add in matrix axes..
expandedCPUCores = env.expand(cpuCores);
expandedCoresPerSocket = env.expand(coresPerSocket);
}

VSphereLogger.vsLogger(jLogger, "Preparing reconfigure: CPU");
spec.setNumCPUs(Integer.valueOf(expandedCPUCores));
spec.setNumCoresPerSocket(Integer.valueOf(expandedCoresPerSocket));
spec.setCpuAllocation(resAllInfo);

Check warning on line 102 in src/main/java/org/jenkinsci/plugins/vsphere/builders/ReconfigureCpu.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 42-102 are not covered by tests
Copy link
Copy Markdown
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
Loading