Skip to content

CLOUDSTACK-10197: Rename xentools iso for XenServer 7.0+#2365

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

Merged
yadvr merged 1 commit into from
Jan 7, 2018
Merged

CLOUDSTACK-10197: Rename xentools iso for XenServer 7.0+ #2365

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion ...ervisors/xenserver/src/com/cloud/hypervisor/xenserver/discoverer/XcpServerDiscoverer.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -536,7 +536,7 @@ private void createXsToolsISO() {
id = _tmpltDao.getNextInSequence(Long.class, "id");
VMTemplateVO template =
VMTemplateVO.createPreHostIso(id, isoName, isoName, ImageFormat.ISO, true, true, TemplateType.PERHOST, null, null, true, 64, Account.ACCOUNT_ID_SYSTEM,
null, "xen-pv-drv-iso", false, 1, false, HypervisorType.XenServer);
null, "XenServer Tools Installer ISO (xen-pv-drv-iso)", false, 1, false, HypervisorType.XenServer);

@rafaelweingartnerrafaelweingartnerJan 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will create an entry for deployments that do not have it yet using this new "standardized name". However, for environment that already had it from previous versions, the name (display_text) will not be updated, right?

@khos2owkhos2owJan 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That is correct, I can explicitly add tmplt.setDisplayText("XenServer Tools Installer ISO (xen-pv-drv-iso)"); at line 544 (couple lines down here)

@rafaelweingartnerrafaelweingartnerJan 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the right thing to do, so we get consistency.
You have to save the tmplt as well right? So, you update the data in the database

_tmpltDao.persist(template);
} else {
id = tmplt.getId();
Expand Down
26 changes: 22 additions & 4 deletions ...hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2592,9 +2592,10 @@ public VDI getIsoVDIByURL(final Connection conn, final String vmName, final Stri
String mountpoint = null;
if (isoURL.startsWith("xs-tools")) {

@rafaelweingartnerrafaelweingartnerJan 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This ISO URL remained the same?

@khos2owkhos2owJan 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it did. I had an idea to change the iso name (url here refers to name) to something general - like xenserver-tools and manage the same thing in actualIsoTemplate, but xs-tools.iso is used in some many places (specially in some test cases) which I ended up not changing this. Potentially it only needs a wiki/user guide doc only.

try {
final Set<VDI> vdis = VDI.getByNameLabel(conn, isoURL);
final String actualIsoURL = actualIsoTemplate(conn);
final Set<VDI> vdis = VDI.getByNameLabel(conn, actualIsoURL);
if (vdis.isEmpty()) {
throw new CloudRuntimeException("Could not find ISO with URL: " + isoURL);
throw new CloudRuntimeException("Could not find ISO with URL: " + actualIsoURL);
}
return vdis.iterator().next();

Expand DownExpand Up@@ -2630,6 +2631,22 @@ public VDI getIsoVDIByURL(final Connection conn, final String vmName, final Stri
}
}

private String actualIsoTemplate(final Connection conn) throws BadServerResponse, XenAPIException, XmlRpcException {

@rafaelweingartnerrafaelweingartnerJan 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind creating a documentation for the method? Then, you do not need comments in line.
Also, this is a good method to create a unit test.

@khos2owkhos2owJan 4, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I will add the documentation, but I really don't think unit testing on this specific method will be possible. Because it needs a live XenServer Host for the method to connect to (both Host.getByUuid(conn, _host.getUuid()); and host.getRecord(conn);) and I'm not sure if they can be Mocked in some way.

@rafaelweingartnerrafaelweingartnerJan 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We do not need a real host. We only want to test the logic. Therefore, you can mock the method calls. PowerMockito and Mockito are there for this kind of job.

final Host host = Host.getByUuid(conn, _host.getUuid());
final Host.Record record = host.getRecord(conn);
final String xenBrand = record.softwareVersion.get("product_brand");
final String xenVersion = record.softwareVersion.get("product_version");
final String[] items = xenVersion.split("\\.");

// guest-tools.iso for XenServer version 7.0+
if (xenBrand.equals("XenServer") && Integer.parseInt(items[0]) >= 7) {
return "guest-tools.iso";
}

// xs-tools.iso for older XenServer versions
return "xs-tools.iso";
}

public String getLabel() {
final Connection conn = getConnection();
final String result = callHostPlugin(conn, "ovstunnel", "getLabel");
Expand DownExpand Up@@ -3882,9 +3899,10 @@ protected VDI mount(final Connection conn, final String vmName, final DiskTO vol
final String templateName = iso.getName();
if (templateName.startsWith("xs-tools")) {
try {
final Set<VDI> vdis = VDI.getByNameLabel(conn, templateName);
final String actualTemplateName = actualIsoTemplate(conn);
final Set<VDI> vdis = VDI.getByNameLabel(conn, actualTemplateName);
if (vdis.isEmpty()) {
throw new CloudRuntimeException("Could not find ISO with URL: " + templateName);
throw new CloudRuntimeException("Could not find ISO with URL: " + actualTemplateName);
}
return vdis.iterator().next();
} catch (final XenAPIException e) {
Expand Down