Skip to content
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

CA-392836,CA-392847: Lost the power state on suspended VM import #5632

Merged
merged 1 commit into from
May 21, 2024

Conversation

minglumlu
Copy link
Member

@minglumlu minglumlu commented May 15, 2024

The VM power state should be preserved on a suspended VM import.

In commit ebb58a8, the power state and suspend VDI in vm_record were
reset on importing a suspended VM. This meant to facilitate the
following Client.VM.create_from_record. But this gets the restore of
power state and suspend VDI broken as it requires the data in
vm_record.

This commit preserves the data in vm_record and reset it just before
it being passed to Client.VM.create_from_record.

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

The code looks correct but the commit message could be improved. Specifically: on import maintain the power state and don't set it to Halted.

@minglumlu minglumlu changed the title CA-392836,CA-392847: Not enforce power_state in VM import CA-392836,CA-392847: Maintain the power state on VM import May 16, 2024
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Now I'm wondering how the previous code with the license feature actually worked

@robhoes
Copy link
Member

robhoes commented May 16, 2024

Those two lines were added a few weeks ago in ebb58a8 when replacing the function create_from_record_without_checking_licence_feature_for_vendor_device with just create_from_record a few line down in import.ml. And that function had:

let create_from_record_without_checking_licence_feature_for_vendor_device
    ~__context rpc session_id vm_record =
  (* vendor device is no longer licensed *)
  let mk_vm r =
    Client.Client.VM.create_from_record ~rpc ~session_id
      ~value:{r with API.vM_suspend_VDI= Ref.null; API.vM_power_state= `Halted}
  in

where r is the vm_record being passed in.

I think the problem is that the import code still needs the vm_record further down to check the original power state. But I don't think the change in this PR is correct; at least it won't revert to the behaviour before ebb58a87387de3849e884e809cd4ace2dc83dd38.

I think we need to use something like

{ value with API.vM_suspend_VDI= Ref.null; API.vM_power_state= `Halted }

within the let vm = block where create_from_record is called.

Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

As above.

@minglumlu
Copy link
Member Author

minglumlu commented May 17, 2024

Those two lines were added a few weeks ago in ebb58a8 when replacing the function create_from_record_without_checking_licence_feature_for_vendor_device with just create_from_record a few line down in import.ml. And that function had:

let create_from_record_without_checking_licence_feature_for_vendor_device
    ~__context rpc session_id vm_record =
  (* vendor device is no longer licensed *)
  let mk_vm r =
    Client.Client.VM.create_from_record ~rpc ~session_id
      ~value:{r with API.vM_suspend_VDI= Ref.null; API.vM_power_state= `Halted}
  in

where r is the vm_record being passed in.

I think the problem is that the import code still needs the vm_record further down to check the original power state. But I don't think the change in this PR is correct; at least it won't revert to the behaviour before ebb58a87387de3849e884e809cd4ace2dc83dd38.

I think we need to use something like

{ value with API.vM_suspend_VDI= Ref.null; API.vM_power_state= `Halted }

within the let vm = block where create_from_record is called.

Hi @robhoes thanks. I see it now.
The new VM DB object is created with API.vM_suspend_VDI= Ref.null; API.vM_power_state= `Halted always.
The problem is the vm_record.vM_power_state and vm_record.vM_suspend_VDI should not be updated so that later they could be checked for updating the new created VM DB record.

 676       (* Set the power_state and suspend_VDI if the VM is suspended.
 677                * If anything goes wrong, still continue if forced. *)
 678       if vm_record.API.vM_power_state = `Suspended then (
 679         try
 680           let vdi = (lookup vm_record.API.vM_suspend_VDI) state.table in
 681           Db.VM.set_power_state ~__context ~self:vm ~value:`Suspended ;
 682           Db.VM.set_suspend_VDI ~__context ~self:vm ~value:vdi ;
 683           let vm_metrics = Db.VM.get_metrics ~__context ~self:vm in
 684           Db.VM_metrics.set_current_domain_type ~__context ~self:vm_metrics
 685             ~value:vm_record.API.vM_domain_type
 686         with e ->
 687           if not config.force then (
 688             Backtrace.is_important e ;
 689             let msg =
 690               "Failed to find VM's suspend_VDI: "
 691               ^ Ref.string_of vm_record.API.vM_suspend_VDI
 692             in
 693             error "Import failed: %s" msg ;
 694             raise e
 695           )

Otherwise, a suspended VDI may be left on a Halted VM.

The VM power state should be preserved on a suspended VM import.

In commit ebb58a8, the power state and suspend VDI in `vm_record` were
reset on importing a suspended VM. This meant to facilitate the
following `Client.VM.create_from_record`. But this gets the restore of
power state and suspend VDI broken as it requires the data in
`vm_record`.

This commit preserves the data in `vm_record` and reset it just before
it being passed to `Client.VM.create_from_record`.

Signed-off-by: Ming Lu <ming.lu@cloud.com>
@minglumlu minglumlu changed the title CA-392836,CA-392847: Maintain the power state on VM import CA-392836,CA-392847: Lost the power state on suspended VM import May 17, 2024
@robhoes robhoes merged commit a0a1e72 into xapi-project:master May 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants