CLOUDSTACK-10146: Bypass Secondary Storage for KVM templates#2379
CLOUDSTACK-10146: Bypass Secondary Storage for KVM templates#2379yadvr merged 10 commits intoapache:masterfrom
Conversation
|
Ping for review @rhtyd @DaanHoogland @rafaelweingartner |
|
@blueorangutan package |
|
@blueorangutan test |
|
@blueorangutan package |
|
@blueorangutan test matrix |
|
@blueorangutan package |
|
@rhtyd @borisstoyanov @DaanHoogland marvin tests added, results on my local environment: |
|
@nvazquez before I start reviewing I have some doubts. Can you answer them?
|
|
Thanks for your time @rafaelweingartner, let me answer your questions:
|
|
Tests LGTM. I'll wait for lgtm from @borisstoyanov @DaanHoogland @rafaelweingartner |
| RandomAccessFile out = null; | ||
| try { | ||
| client.executeMethod(request); | ||
| remoteSize = getRemoteSize(request); |
There was a problem hiding this comment.
remoteSize is never read. If the line is remove, the function can be removed too since it's not used.
There was a problem hiding this comment.
Hi @marcaurele, in fact it is, on copyBytes method in line 141
There was a problem hiding this comment.
Since you're using this info in the copyBytes method, this is overkill. It can even throw an exception but having the filesize isn't mandatory to download it. This information, if present in the header, should be used as a nice to have but the download must work without this info as @wido pointed that some servers don't send this information in the header (#1934 (comment)). So it must be changed
nvazquez
force-pushed
the
bypasssecstoragekvm
branch
from
682e09e to
ae88533
Compare
| } finally { | ||
| try { | ||
| out.close(); | ||
| in.close(); |
There was a problem hiding this comment.
Would be nicer to move the code in a try with resource statement (https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html)
| return isDynamicallyScalable == null ? Boolean.FALSE : isDynamicallyScalable; | ||
| } | ||
|
|
||
| public Boolean isDirectDownload() { |
There was a problem hiding this comment.
can return a boolean instead since it cannot be null
| return isRoutingType; | ||
| } | ||
|
|
||
| public Boolean isDirectDownload() { |
|
|
||
| @Override | ||
| public void setExecuteInSequence(boolean inSeq) { | ||
| } |
| @Override | ||
| public void setExecuteInSequence(boolean inSeq) { | ||
|
|
||
| } |
| @Override | ||
| public void setExecuteInSequence(boolean inSeq) { | ||
|
|
||
| } |
|
|
||
| String getInstallPath(); | ||
|
|
||
| boolean isDirectDownload(); |
There was a problem hiding this comment.
Just for your info, be aware of not mixing Boolean and boolean types since the JVM won't check for those kind of mismatch as it does auto casting to the expected type. But you will end up with a NPE when getting a Boolean NULL on a method expecting a boolean.
| for (int i = 0; i < 2; i++) { | ||
| // retry one more time in case of template reload is required for Vmware case | ||
| AsyncCallFuture<VolumeApiResult> future = null; | ||
| boolean bypassed = false; |
| ADD COLUMN `direct_download` TINYINT(1) DEFAULT '0' COMMENT 'Indicates if Secondary Storage is bypassed and template is downloaded to Primary Storage'; | ||
|
|
||
| DROP VIEW IF EXISTS `cloud`.`template_view`; | ||
| CREATE VIEW `template_view` AS |
There was a problem hiding this comment.
Better to put those drop & create in a single statement:
CREATE OR REPLACE VIEW `template_view` AS
| this.updated = updated; | ||
| } | ||
|
|
||
| public Boolean isDirectDownload() { |
| List<VMTemplateStoragePoolVO> existingRefs = templatePoolDao.listByTemplateId(templateId); | ||
| pool = getOneMatchingPoolIdFromRefs(existingRefs, pools); | ||
| } | ||
| VMTemplateStoragePoolVO spoolRef = templatePoolDao.findByPoolTemplate(pool, templateId); |
There was a problem hiding this comment.
pool can be null here, according to https://github.com/apache/cloudstack/pull/2379/files#diff-92e0bb0fd44a51c2c2234ae65d88fbbdR181
Unsure if findBYPoolTemplate will like it or not
|
|
||
| private List<PrimaryDataStore> listPrimaryDataStoresByClusterId(Long clusterId) { | ||
| List<StoragePoolVO> storagePoolVOS = dataStoreDao.listPoolsByCluster(clusterId); | ||
| return null; |
There was a problem hiding this comment.
return null ? I guess it should be return storagePoolVOS
| public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) { | ||
| final PrimaryDataStoreTO pool = cmd.getDestPool(); | ||
| if (!pool.getPoolType().equals(StoragePoolType.NetworkFilesystem)) { | ||
| return new DirectDownloadAnswer(false, "Unsopported pool type " + pool.getPoolType().toString()); |
There was a problem hiding this comment.
typo: Unsupported pool type...
| } else if (cmd instanceof MetalinkDirectDownloadCommand) { | ||
| downloader = new MetalinkDirectTemplateDownloader(cmd.getUrl(), destPool.getLocalPath(), cmd.getTemplateId(), cmd.getChecksum()); | ||
| } else { | ||
| return new DirectDownloadAnswer(false, "Unsopported protocol, please provide HTTP(S), NFS or a metalink"); |
There was a problem hiding this comment.
Same typo here: Unsupported...
| protected boolean dynamicallyScalable; | ||
|
|
||
| @Column(name = "direct_download") | ||
| private Boolean directDownload; |
There was a problem hiding this comment.
Since it's 0 by default and cannot be null, it can be switched to a boolean
| private String tempZonePair; // represent a distinct (templateId, data_center_id) pair | ||
|
|
||
| @Column(name = "direct_download") | ||
| private Boolean directDownload; |
There was a problem hiding this comment.
boolean here too, same comment as on the other VO
| this.accountId = accountId; | ||
| } | ||
|
|
||
| public Boolean getDirectDownload() { |
| profile.getTemplateType(), profile.getUrl(), profile.getRequiresHVM(), profile.getBits(), profile.getAccountId(), profile.getCheckSum(), | ||
| profile.getDisplayText(), profile.getPasswordEnabled(), profile.getGuestOsId(), profile.getBootable(), profile.getHypervisorType(), | ||
| profile.getTemplateTag(), profile.getDetails(), profile.getSshKeyEnabled(), profile.IsDynamicallyScalable()); | ||
| profile.getTemplateTag(), profile.getDetails(), profile.getSshKeyEnabled(), profile.IsDynamicallyScalable(), profile.isDirectDownload()); |
There was a problem hiding this comment.
profile.isDirectDownload() might return null since all constructors don't set the direct download Boolean variable. The method declaration expects a boolean, so no null.
| SetupDirectDownloadCertificate cmd = new SetupDirectDownloadCertificate(certificate, certificateName); | ||
| Answer answer = agentManager.easySend(hostId, cmd); | ||
| if (answer != null && answer.getResult()) { | ||
| s_logger.debug("Certificate " + certificateName + "successfully uploaded to host: " + hostId); |
There was a problem hiding this comment.
would be good to have it at the infolevel
| List<String> urls = new ArrayList<>(); | ||
| try { | ||
| if (httpClient.executeMethod(getMethod) == HttpStatus.SC_OK) { | ||
| InputStream is = getMethod.getResponseBodyAsStream(); |
There was a problem hiding this comment.
same remark for the try with resource statement here. Currently the inputstream close is missing.
rafaelweingartner
left a comment
rafaelweingartner
left a comment
There was a problem hiding this comment.
@nvazquez I have checked part of the PR, but there are more things to review.
|
|
||
| /** | ||
| * Return filename from url | ||
| * @return |
There was a problem hiding this comment.
You can remove this empty "@return/@param" or others in the same style
|
|
||
| /** | ||
| * Return install full path | ||
| * @return |
|
|
||
| /** | ||
| * Return extract command to execute given downloaded file | ||
| * @return |
| */ | ||
| private String getExtractCommandForDownloadedFile() { | ||
| if (downloadedFilePath.endsWith(".zip")) { | ||
| return "unzip -p " + downloadedFilePath + " | cat > " + getInstallFullPath(); |
There was a problem hiding this comment.
Are all of these programs required by agent installation?
Otherwise, this will cause a runtime error.
|
|
||
| /** | ||
| * Return checksum command from algorithm | ||
| * @param algorithm |
|
|
||
| /** | ||
| * Download template/ISO into poolId bypassing secondary storage. Download performed by hostId | ||
| * @param templateId |
|
|
||
| @Override | ||
| public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) { | ||
| // Not implemented for OV3 |
There was a problem hiding this comment.
These comments can be remove
|
|
||
| @Override | ||
| public Answer handleDownloadTemplateToPrimaryStorage(DirectDownloadCommand cmd) { | ||
| // Not implemented for Vmware |
| sc.and(sc.entity().getResourceState(), Op.EQ, ResourceState.Enabled); | ||
| sc.and(sc.entity().getRemoved(), Op.NULL); | ||
| List<HostVO> hosts = sc.list(); | ||
| if (hosts == null || hosts.isEmpty()) { |
There was a problem hiding this comment.
CollectionUtils.isEmpty
| /** | ||
| * Persist template marking it for direct download to Primary Storage, skipping Secondary Storage | ||
| * @param templateId template id | ||
| * @param size |
|
Thanks @DaanHoogland I'll check again tomorrow when the results are back, may tackle outstanding issues myself. |
|
Trillian test result (tid-2046)
|
|
@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-1610 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2063)
|
|
SSVM tests are not related to this, let me have a quick look at the template ones. |
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1623 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2085)
|
|
Tests LGTM (template related errors not seen in last run), merging this based on code reviews and test results. |
7 participants
JIRA Ticket: https://issues.apache.org/jira/browse/CLOUDSTACK-10146
Background
This feature allows using templates and ISOs avoiding secondary storage as intermediate cache on KVM. The virtual machine deployment process is enhanced to supported bypassed registered templates and ISOs, delegating the work of downloading them to primary storage to the KVM agent instead of the SSVM agent.
Description
Template and ISO registration:
registerTemplateandregisterISOare both extended with this new parameterdirectdownload.template_store_refindicating that template or ISO has been marked as 'Direct Download' (bypassing Secondary Storage). These entries are persisted as:(Note: these entries allow users to deploy virtual machine from registered templates or ISOs)
{ALGORITHM}CHKSUMHASHVirtual machine deployment
In case of HTTPS, a new API method is added
uploadTemplateDirectDownloadCertificateto allow user importing a client certificate into all KVM hosts' keystore before deployment.template_spool_refindicating the mapping between template/ISO and storage pool.