-
Notifications
You must be signed in to change notification settings - Fork 6
Systemvm template api #45
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
Conversation
Ping for review @rhtyd, I'll be reviewing this PR. If you can review as well it will be great |
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.
Did a first review but could not finish it yet, leaving some comments ffrom my first review
|
||
// mount locally | ||
String mountPoint = "/tmp/nfsmount"; | ||
Script.runSimpleBashScript("mkdir " + mountPoint); |
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.
Add '-p'
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.
Done
for (String imageStore: imageStores){ | ||
try { | ||
URI uri = new URI(imageStore); | ||
Script.runSimpleBashScript("sudo mount -t nfs " + uri.getHost() + ":" + uri.getPath() + " " + mountPoint); |
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.
sudo not needed
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.
In my environment, this script doesn't work without sudo
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 checked other places in the code base, mount is called using sudo
} | ||
|
||
// now download the template to the image store for every hypervisor if not specified | ||
if (this.hypervisor != null && this.url != null){ |
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.
Use StringUtils.isNotBlank()
for these kind of checks
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 am specifically checking if these values were not passed in.
// now download the template to the image store for every hypervisor if not specified | ||
if (this.hypervisor != null && this.url != null){ | ||
|
||
if (this.hypervisor.equalsIgnoreCase("kvm") || this.hypervisor.equalsIgnoreCase("lxc")) { |
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.
Check Hypervisor.HypervisorType.getType(hypervisor)
function. You can convert the String to a Hypervisor type and compare using that type
|
||
this.templateId = _queryService.getSystemVMTemplateId(this); | ||
|
||
HttpDirectTemplateDownloader httpDirectTemplateDownloader = new HttpDirectTemplateDownloader(url, templateId, mountPoint, null,null); |
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.
Do not use this class, been checking it is duplicated and unused. You can invoke wget URL -P mountPoint
|
||
@Override | ||
public void execute(){ | ||
ListResponse<ImageStoreResponse> imageStoreResponses = _queryService.searchForImageStores(new ListImageStoresCmd()); |
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.
Maybe you can place all this logic on lines 56-71 to a new method on QueryService receiving a SeedSystemVMTemplateCmd parameter
try { | ||
URI uri = new URI(imageStoreResponse.getUrl()); | ||
|
||
Script.runSimpleBashScript("mkdir " + mountPoint); |
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.
-p and sudo below
|
||
String script = Script.findScript("scripts/storage/secondary/","cloud-install-sys-tmplt"); | ||
|
||
String command = script + " -h " + hypervisor + " -F -m " + mountPoint + " -f " + uploadPath + "/" + fileUUID; |
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.
Maybe use StringFormat.format() for readability
command += " -t " + templateId; | ||
} | ||
|
||
Script.runSimpleBashScriptForExitValue(command); |
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.
No checks on the exit value for the response?
|
||
import com.cloud.hypervisor.Hypervisor; | ||
|
||
public class OfficialSystemVMTemplate { |
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 whole class is using 4.11.3 instead of using a map between CloudStack version and the corresponding system VM template 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.
This code was added in an upgrade script, I moved it to a class.
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.
Maybe out of scope but I think these maps can no longer be final if they are to be configurable by the admin and/or developer. (though of course the contents of the maps aren't)
Maybe provide an interface to do individual updates to it? (see other comment; the upgrade should now call the systemVM management API and not still do it its own way.
And finally, is this still needed in code (we now have administration in DB)?
system, // system vm templates | ||
inactive, // inactive system vm templates | ||
all // all templates (only usable by admins) |
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 you explain why we're adding new template filters?
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 "system" filter is used to get the system template id for seeding.
The "inactive" filter is used to list all inactive system vm templates. The list is used on the front end to list other system vm templates that are not currently used. We needed this view because the "all" filter doesn't show inactive system vm templates.
return id; | ||
} | ||
|
||
@Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = ZoneResponse.class, required = true, description = "the ID of the zone") |
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.
Reformat the API implementation, see other/existing APIs how they're formatted.
We would for example prefer the params to be declared before the accessors (getters/setters); and then the execute() etc. methods.
import com.cloud.utils.http.downloader.HttpDirectTemplateDownloader; | ||
import com.cloud.utils.script.Script; | ||
|
||
@APICommand(name = "seedOfficialSystemVMTemplate", description = "Copies an official system vm template into secondary storage", |
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.
Could be simply rename this to 'registerSystemVMTemplate'?
import com.cloud.utils.script.Script; | ||
|
||
|
||
@APICommand(name = "seedSystemVMTemplate", description = "Copies a system vm template into secondary storage", |
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.
Why do we have two separate APIs for the same thing?
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.
do we?
hypervisor = hypervisor.toLowerCase(); | ||
|
||
String mountPoint = "/tmp/nfsmount"; | ||
String uploadPath = "/tmp/upload"; |
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.
Move the logic to a manager class; treat APIs as containers of params; and execute() to simply process and send a response.
|
||
@Override | ||
public String getCommandName() { | ||
return "seedsystemvmtemplateresponse"; |
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.
Refer to hackerbook and other APIs, on how to set the name. Use static API name.
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.
Left some comments.
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.
@Spaceman1984 please address the comments in the integration tests section
#Import System modules | ||
import time | ||
from marvin.cloudstackAPI import (createTemplate, listOsTypes) | ||
from marvin.cloudstackAPI import (createTemplate, listOsTypes, activateSystemVMTemplate) |
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.
why do we need changes in this file? I think the tests are in separate file?
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.
Done, removed the unused import.
|
||
def test_02_copySystemVMtemplate(self): | ||
# this test needs 2 zones, the template will be copied from 1 zone to the next | ||
pass |
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 report that test_02 is passing, if we cannot facilitate the execution of the test please remove it with propper comment, simply reporting that it's passing is wrong
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.
Removed the test
@@ -0,0 +1,182 @@ | |||
#!/usr/bin/env python |
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.
Perhaps it's worth adding tests for:
Register a template as systemvm template
Add tests for getSystemVMTemplateDefaultUrl - positive and negative
Investigate if upload template as system template is feasible/worth doing
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.
Already registering a template in the "activate" test.
Added a test for getSystemVMTemplateDefaultUrl.
Uploading a template will need to be done using a form post. This is theoretically possible but would take some effort.
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.
Please add at least a test for each new api and api change, we're now having 3 new apis and 3 old that have been changed
cls.apiclient, | ||
cls.services["service_offerings"]["tiny"] | ||
) | ||
cls._cleanup.append(cls.service_offering) |
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.
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.
@Spaceman1984 note that the order of cleanup can matter at times.
cleanup_resources(cls.apiclient, reversed(cls._cleanup))
might help. Also make sure non-deletable items are cleaned in a separate method; think of settings or trust configurations and such. (not doing a full review, just stumbled upon this)
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.
Removed added system VM template.
domainid=cls.domain.id | ||
) | ||
cls._cleanup.append(cls.user) | ||
cls.service_offering = ServiceOffering.create( |
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.
Why do we need service offering? Do you plan to use it in VM deployment?
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.
Leftover code from where I copied the test from, I have removed it.
for temp in cls.templates: | ||
cmd = activateSystemVMTemplate.activateSystemVMTemplateCmd() | ||
cmd.id = temp.id | ||
cls.apiclient.activateSystemVMTemplate(cmd) |
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.
As far as I see there's no template being deleted 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.
Done
cls.unsupportedHypervisor = False | ||
cls.hypervisor = cls.testClient.getHypervisorInfo() | ||
if cls.hypervisor.lower() not in ['kvm']: | ||
# Only testing kvm |
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.
why? This feature is agnostic to hypervisor, this test should be able to work with all hypervisors
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.
Done
|
||
self.activateTemplateCmd = activateSystemVMTemplate.activateSystemVMTemplateCmd() | ||
self.listConfigurationsCmd = listConfigurations.listConfigurationsCmd() | ||
self.listConfigurationsCmd.name = 'router.template.kvm' |
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.
please do not hardcode, this feature is not just kvm specific
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.
Done
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.
why the getUrl APIs?
Impressive bit of code, I reviewed until the test files (first 43 files)
looks mostly good.
authorized = {RoleType.Admin}) | ||
public class SeedSystemVMTemplateCmd extends BaseCmd { | ||
|
||
private static final String s_name = "seedsystemvmtemplateresponse"; |
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.
better use a name without response, use it in the annotation parameter APICommand.name and construct the commandsname with s_name (NAME is a better name) .toLower() + "response".
there are plenty examples about. (also of this older way of doing things admittedly)
import com.cloud.utils.script.Script; | ||
|
||
|
||
@APICommand(name = "seedSystemVMTemplate", description = "Copies a system vm template into secondary storage", |
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.
do we?
"Both zoneid and zoneids cannot be specified at the same time"); | ||
|
||
if (zoneId == null && (zoneIds == null || zoneIds.isEmpty())) | ||
if ((zoneId == null && (zoneIds == null || zoneIds.isEmpty())) && !isSystem()) |
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.
why this exemption? I don't see why systemVM templates can be region wide and user VM templates couldn't
@APICommand(name = ActivateSystemVMTemplateCmd.APINAME, | ||
description = "Activates an existing system virtual machine template to be used by CloudStack to create system virtual machines.", | ||
responseObject = ActivateSystemVMTemplateResponse.class, | ||
authorized = {RoleType.Admin}) | ||
public class ActivateSystemVMTemplateCmd extends BaseCmd { |
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 there (a plan for) a deactive. Deleting templates is not trivial as these may be used still but still we want to make sure we can in the long run.
if (type == TemplateType.SYSTEM){ | ||
uniqueName = "routing-" + id; | ||
} |
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.
not all systemVMs are routing. is this a sledge hammer to a fine nail? might be needed but it deserves a comment for future generations.
if (!_statsCollector.imageStoreHasEnoughCapacity(imageStore) && | ||
(template.getTemplateType().equals(Storage.TemplateType.SYSTEM) && !capacityChecker.hasEnoughCapacity(imageStore))) { |
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.
Are we checking for capacity in two ways?
" is disabled, so uploading template to management server" + imageStore.getId()); | ||
//continue; |
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.
don't like this change:
- space removed at end of string so imagestore id gets glued to the word server.
- string says management server biut then it give not the id of the server but on the store, without explanation.
- // continue ==> code in comment, please remove or uncomment
|
||
|
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.
superfluent empty lines
List<Long> zoneIdList = profile.getZoneIdList(); | ||
|
||
if (template.getTemplateType() == TemplateType.SYSTEM) { | ||
// TODO: Add a check to see if this template is used by console proxy, router or storagevm |
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.
these are TemplateType.SYSTEM as well aren't they?
List<VMTemplateVO> systemvmTmplts = _tmpltDao.listAllSystemVMTemplates(); | ||
for (VMTemplateVO template : systemvmTmplts) { | ||
if (template.getName().equalsIgnoreCase(name) || template.getDisplayText().equalsIgnoreCase(displayText)) { | ||
if (template.getTemplateType() != TemplateType.SYSTEM && (template.getName().equalsIgnoreCase(name) || template.getDisplayText().equalsIgnoreCase(displayText))) { |
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.
30 and 10 lines into a 130 lines method some things have changed! On cannot but love maintaining monolithic code with hundreds of people.
No idea what else to say about these two changes ;)
if cls.firstRun: | ||
# Save currently active template | ||
activeTemplate = cls.apiclient.listTemplates(cls.listTemplatesCmd)[0] | ||
cmd = deleteTemplate.deleteTemplateCmd() |
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.
deleteTemaplate?
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 need to delete the newly created template as part of the cleanup.
cls._cleanup.append(cls.user) | ||
|
||
cls.listTemplatesCmd = listTemplates.listTemplatesCmd() | ||
cls.listTemplatesCmd.templatefilter= '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.
isn't this going to return just a bunch of templates of type 'system'. Shouldn't we be looking to get the value from the global settings?
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'll see what I can do
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 call only returns the currently active system VM template
self.test_template.name = 'test-system-' + self.hypervisor + '-4.11.3' | ||
self.test_template.displaytext = 'test-system-' + self.hypervisor + '-4.11.3' | ||
self.test_template.url = urlResponse.url.url #"http://download.cloudstack.org/systemvm/4.11/systemvmtemplate-4.11.3-kvm.qcow2.bz2" | ||
self.test_template.format = "QCOW2" |
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.
format should be hypervisor specific also
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 it would be good to return the template file type as part of the getsystemVMTemplatedefaultURL call. I will add this.
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.
Done
|
||
@attr(tags=["advanced", "smoke"], required_hardware="true") | ||
def test_03_seedSystemVMTemplate(self): | ||
activeTemplate = self.apiclient.listTemplates(self.listTemplatesCmd)[0] |
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.
no need of this variable, cmd.templateid later can call directly the listTemplates api, and what if we have multiple active templates, how do we know which one we're dealing with? Probably include the given hypervisor.
a1b2bf8
to
dd2dd59
Compare
* TODO: Automated tests development and UI integration
* TODO: Automated tests development and UI integration
…emvm template will be whatever is stored in the database.
…es not being removed when adding system templates from the wizzard.
…flag to zone multi select box
02b9c24
to
d8532b9
Compare
Abandoned. |
* Prevent addition of duplicate PF rules on scale up and no rules left behind on scale down (#32) * fix missing dependency injection * NSX: Fix concurrency issues on port forwarding rules deletion (#37) * Fix concurrency issues on port forwarding rules deletion * Refactor objectExists * Fix unit test * Fix test * Small fixes * CKS: Externalize control and worker node setup wait time and installation attempts (#38) * NSX: Add shared network support (#41) * NSX: Fix number of physical networks for Guest traffic checks and leftover rules on CKS cluster deletion (#45) * Fix pf rules removal on CKS cluster deletion * Fix check for number of physical networks for guest traffic * Fix unit test * fix logger * NSX: Handle CheckHealthCommand to avoid host disconnection and errors on APIs * NSX: Handle CheckHealthCommand to avoid host disconnection and errors on APIs * Remove unused string * fix logger * Update UDP active monitor to ICMP * Fix NPE on restarting VPC with additional public IPs * NSX / VPC: Reuse Source NAT IP from systemVM range on restarts * CKS: Public IP not found for VPC networks * Externalize retries and inverval for NSX segment deletion (#67) * remove unused import * remove duplicate imports * remove unused import * revert externalizing cks settings * fix test * Refactor log messages * Address comments * Fix issue caused due to forward merge: 90fe1d --------- Co-authored-by: Nicolas Vazquez <nicovazquez90@gmail.com> Co-authored-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Description
[WIP] System Template API
PR for review
Release notes: https://shapeblue.sharepoint.com/:w:/r/Shared%20Documents/Development/Roadmap/FR%20Documents/FR%2038%20-%20System%20VM%20Management%20API/FR38%20-%20Release%20Notes.docx?d=w5a674714278342f49ebe182e7aa0afd0&csf=1&e=Siq9bB
Types of changes
Screenshots (if appropriate):
How Has This Been Tested?