[
https://issues.apache.org/jira/browse/CLOUDSTACK-9539?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15856802#comment-15856802
]
ASF GitHub Bot commented on CLOUDSTACK-9539:
--------------------------------------------
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1727
Long time I do not review a PR here. I have to say, @nvazquez , great job!!
@serg38 you read my mind!! I had just started reading @nvazquez PR
description when you called me.
Well documented methods. The methods are short and concise. The code is
clear and looks nice (at least for me).
I jumped over the 70+ messages here. So, excuse me if I ask something that
has already been clarified before.
However, as always I have something to complain; I might be getting old,
that is probably why :) . Just teasing; it is more like a set of suggestion;
* On VMSnapshotVO, what about declaring the instance attribute you create
as private? I know the others are not, and at the end of the day, it does not
change much. However, as long as we are using Java and if we consider
“VMSnapshotVO” as a POJO, then it feels a good idea to use private attributes
that are only accessible to their gets/sets.
* On VMSnapshotManagerImpl, here we have a “service” layer
object/component. I would have the same suggestion regarding the use of
private/protected attributes here (this is cosmetics).
* Still on VMSnapshotManagerImpl, what about extracting the code from lines
358-373 to a method? This may enable you to write unit test cases for these
lines.
* Also on VMSnapshotManagerImpl, methods
“addSupportForCustomServiceOffering”, “updateUserVmServiceOffering”,
“getVmMapDetails”, “changeUserVmServiceOffering”,
revertUserVmDetailsFromVmSnapshot, and “ugradeUserVmServiceOffering” are clean
and well documented. However, I am missing their test cases. What about some
test cases here, then it would be awesome.
* on VMSnapshotManagerImpl, the method “ugradeUserVmServiceOffering” seems
to have a typo mistake ;)
* the method “revertUserVmDetailsFromVmSnapshot” receives a parameter
“userVm”, but it does not seem to be used.
I am assuming you have executed the functional test “test_vm_snapshots.py”
and it passed successfully, right?
Very good feature to be added. Thanks for the valuable contribution to ACS
👍
> Support changing Service offering for instance with VM Snapshots
> ----------------------------------------------------------------
>
> Key: CLOUDSTACK-9539
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9539
> Project: CloudStack
> Issue Type: Bug
> Security Level: Public(Anyone can view this level - this is the
> default.)
> Reporter: Nicolas Vazquez
> Assignee: Nicolas Vazquez
>
> h3. Actual behaviour
> CloudStack doesn't support changing service offering for vm instances which
> have vm snapshots, they should be removed before changing service offering.
> h3. Goal
> Extend actual behaviour by supporting changing service offering for vms which
> have vm snapshots. In that case, previously taken snapshots (if reverted)
> should use previous service offering, future snapshots should use the newest.
> h3. Proposed solution:
> 1. Adding {{service_offering_id}} column on {{vm_snapshots}} table: This way
> snapshot can be reverted to original state even though service offering can
> be changed for vm instance.
> NOTE: Existing vm snapshots are populated on update script by {{UPDATE
> vm_snapshots s JOIN vm_instance v ON v.id = s.vm_id SET s.service_offering_id
> = v.service_offering_id;}}
> 2. New vm snapshots will use instance vm service offering id as
> {{service_offering_id}}
> 3. Revert to vm snapshots should use vm snapshot's {{service_offering_id}}
> value.
> h3. Example use case:
> - Deploy vm using service offering A
> - Take vm snapshot -> snap1 (service offering A)
> - Stop vm
> - Change vm service offering to B
> - Revert to VM snapshot snap 1
> - Start vm
> It is expected that vm has service offering A after last step
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
