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.
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
CLOUDSTACK-10197: Rename xentools iso for XenServer 7.0+ #2365
Conversation
Lgtm |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1408 |
@blueorangutan test centos7 xenserver-65sp1 |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
Trillian test result (tid-1822)
|
I see new iso related failures, rest are known issues. Can you check @khos2ow |
@rhtyd is there a way to see the error log? |
@khos2ow yes, download the marvin logs zip file from the test above, extract and see the logs for that test to get hints etc. |
@rhtyd damn! I haven't seen that link!!! thanks. |
3fa0c0b
to
f4b15af
Compare
@khos2ow can you fix the build failures? |
262b24a
to
249378c
Compare
@khos2ow let me if you've addressed the issues, so I can kick another round of testing? |
@rhtyd should be fine now, there was a code-style issue which I fixed it. |
Okay |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1449 |
@blueorangutan test centos7 xenserver-65sp1 |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
@khos2ow the new changes causes management server to fail on start, in a fresh installation. I get:
Please fix and test in your environment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes break management server install/start, please fix.
@@ -529,13 +540,12 @@ public boolean processCommands(long agentId, long seq, Command[] commands) { | |||
} | |||
|
|||
private void createXsToolsISO() { | |||
String isoName = "xs-tools.iso"; | |||
VMTemplateVO tmplt = _tmpltDao.findByTemplateName(isoName); | |||
VMTemplateVO tmplt = _tmpltDao.findByTemplateName(_toolsIsoName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is at this line _toolsIsoName
is not initialized yet (basically cretaeXsToolsISO
is executed before newly added determineToolsIsoName
method). My other idea is at this line we can register both xs-tools.iso
and guest-tools.iso
and either:
- later on (for example in
createServerResource
method) delete/deactivate the wrong iso - present both of them on UI with different names and let the user select the correct iso based on the version of their Xen
@rhtyd @wido @syed @rafaelweingartner @swill any comment, suggestion, concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would start with a different question. Do we need to check which XenServers versions are used? I mean, Can’t we simply persist both ISOs in the database? Without needing to use that “determine” method.
It seems that as long as we have both entries in the database (or new ones in the far far future ahead of us), your logic in CitrixResourceBase.java
would take care of the rest. I would recommend one thing though. You have the strings "guest-tools" and "xs-tools" in two different classes (at least the ones I am seeing in this PR). What about extracting them to an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do, that's the whole point of this PR. The tools ISO has been renamed. So the question will be we auto detect it or provide the options for user to select.
+1 on extracting to enum though, will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we create a more transparent mode? I mean, the user sees just the "Xen-server-tools", and then underneath we select the right name according to the XenServer version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I was trying to do but the way I did it was wrong, hence the first option mentioned above:
we can register both xs-tools.iso and guest-tools.iso and ... later on (for example in createServerResource method) delete/deactivate the wrong iso
Long id; | ||
if (tmplt == null) { | ||
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, | ||
VMTemplateVO.createPreHostIso(id, _toolsIsoName, _toolsIsoName, ImageFormat.ISO, true, true, TemplateType.PERHOST, null, null, true, 64, Account.ACCOUNT_ID_SYSTEM, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know what you think about the use of "_" here to denote object attributes, but I do not see any benefit on using it. As a matter of fact, I see most PRs removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I, honestly, don't like it and have never used it like this, but wanted to follow the same pattern in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always good to create a new trend :)
if (isoURL.startsWith("xs-tools")) { | ||
// XenServer 7.0+ => guest-tools.iso | ||
// XenServer [other] => xs-tools.iso | ||
if (isoURL.startsWith("xs-tools") || isoURL.startsWith("guest-tools")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for isoURL
to be null? If not, it is ok the way it is. Otherwise, it might be a good idea to use StringUtils.startsWith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I doubt it will be null, but checking for null almost always is good. I'll add it.
@khos2ow do make suitable changes and let me know when you next want to kick tests. |
@rhtyd @rafaelweingartner the new change:
Any comments, suggestions? |
What are the assumptions here? What about something different? The user does not need to know the version of XenServer the cloud providers use. We could present a “dummy” ISO option called “XenServer tools. Then, underneath, when plugging the ISO to the VM, we check the host the VM is running and select/find/discover the right VDI that represent the XenServer tools ISO to be connected to the VM (according to host’s XenServer version). This would require only a single entry for any type of XenServer tools. |
b53835c
to
ecda5d7
Compare
@rafaelweingartner I don't know if we can have a clusters/hosts with different XS versions, but in any case I changed it again as follow
in the future, if the name of iso changes again, only another |
ecda5d7
to
5ff7a1d
Compare
The xentools iso has been renamed from xs-tools to guest-tools starting from XenServer 7.0.
5ff7a1d
to
66ff2fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM subject to testing
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1561 |
@blueorangutan test centos7 xenserver-65sp1 |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + xenserver-65sp1) has been kicked to run smoke tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khos2ow I liked this solution better.
I pointed out some remarks, could you take a look at them?
@@ -2592,9 +2592,10 @@ public VDI getIsoVDIByURL(final Connection conn, final String vmName, final Stri | |||
String mountpoint = null; | |||
if (isoURL.startsWith("xs-tools")) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
@@ -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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Trillian test result (tid-2003)
|
@khos2ow can you address outstanding issues? |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1584 |
@rhtyd I don't think the failed tests are related to the change. The change was in attaching ISO. Can you re-run the test? |
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1604 |
@khos2ow I've kicked another test round against xenserver 7.1 |
Trillian test result (tid-2050)
|
The xentools iso has been renamed from xs-tools to guest-tools
starting from XenServer 7.0.
This PR is the follow up of #2346
@reviewers: The main change is in
getIsoVDIByURL
method [line ~2589] the rest is autoformatting.