Skip to content

server: event for HA vm start#9202

Merged
vishesh92 merged 11 commits intoapache:4.19from
shapeblue:ha-vmstart-event
Jun 26, 2024
Merged

server: event for HA vm start#9202
vishesh92 merged 11 commits intoapache:4.19from
shapeblue:ha-vmstart-event

Conversation

@shwstppr
Copy link
Copy Markdown
Contributor

@shwstppr shwstppr commented Jun 10, 2024

Description

Fixes #8021

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

After change:

User VM

  • Destroy a HA-enabled VM on KVM hypervisor host using virsh
  • Observe events for the VM
root@pr8601-t6928-kvm-ubuntu22-kvm1:~# date
Thu Jun 13 05:14:13 AM UTC 2024
root@pr8601-t6928-kvm-ubuntu22-kvm1:~# virsh destroy i-2-14-VM
Domain 'i-2-14-VM' destroyed

image

VR

  • Destroy a HA-enabled VR on KVM hypervisor host using virsh
  • Observe events for the VR
root@pr8601-t6928-kvm-ubuntu22-kvm2:~# date
Thu Jun 13 05:18:12 AM UTC 2024
root@pr8601-t6928-kvm-ubuntu22-kvm2:~# virsh destroy r-13-VM
Domain 'r-13-VM' destroyed

image

How did you try to break this feature and the system with this change?

Fixes apache#8021

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.29%. Comparing base (8d02e5f) to head (6a83a22).

Additional details and impacted files
@@             Coverage Diff              @@
##               4.19   #9202       +/-   ##
============================================
- Coverage     14.95%   4.29%   -10.67%     
============================================
  Files          5378     363     -5015     
  Lines        469930   29387   -440543     
  Branches      59422    5138    -54284     
============================================
- Hits          70301    1261    -69040     
+ Misses       391842   27983   -363859     
+ Partials       7787     143     -7644     
Flag Coverage Δ
uitests 4.29% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
shwstppr added 7 commits June 12, 2024 16:55
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr shwstppr marked this pull request as ready for review June 13, 2024 05:29
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm, just wondering if we could call the methods just startForHA(..) instead of type specific names? I know the services to call are different, so it's only cosmetic.

Co-authored-by: dahn <daan.hoogland@gmail.com>
@shwstppr
Copy link
Copy Markdown
Contributor Author

@DaanHoogland separate methods are called allow publishing of events of different types. Initially, I did the same.
Other way to allow publishing of correct events could be manually calling ActionEventUtils.onStarted/onCompleted/onError

@shwstppr
Copy link
Copy Markdown
Contributor Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland
Copy link
Copy Markdown
Contributor

@DaanHoogland separate methods are called allow publishing of events of different types. Initially, I did the same. Other way to allow publishing of correct events could be manually calling ActionEventUtils.onStarted/onCompleted/onError

not sure if we understand each other @shwstppr , couldn't routerService.startRouterForHA(vm, params, planner); be just called routerService.startForHA(vm, params, planner); and consoleProxyManager.startProxyForHA(vm, params, planner); be called consoleProxyManager.startForHA(vm, params, planner); and so on?

@shwstppr
Copy link
Copy Markdown
Contributor Author

oh. I thought you meant using a single method. That can be changed. I based the on the existing methods in the service, eg: https://github.com/apache/cloudstack/pull/9202/files#diff-94dd8d188fcf167c1fe8196f66e9817665641c95eda7f3c8fa03c2876ad1a1a2R60

@DaanHoogland
Copy link
Copy Markdown
Contributor

oh. I thought you meant using a single method. That can be changed. I based the on the existing methods in the service, eg: https://github.com/apache/cloudstack/pull/9202/files#diff-94dd8d188fcf167c1fe8196f66e9817665641c95eda7f3c8fa03c2876ad1a1a2R60

I was just seeing the possiboility for some polimorphism. retrieve the interface and run a generic method on it. I am not sure if this will give us much cleanup or maintainability in this case though. Do as you see fit.

@blueorangutan
Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9917

@shwstppr
Copy link
Copy Markdown
Contributor Author

yes, currently it won't benefit. Will try to explore a generic interface like BaseVmManager in the coming weeks

@github-actions
Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jun 18, 2024

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9992

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link
Copy Markdown

[SF] Trillian test result (tid-10504)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43660 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9202-t10504-kvm-centos7.zip
Smoke tests completed. 130 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_02_trigger_shutdown Failure 341.66 test_safe_shutdown.py

@vishesh92
Copy link
Copy Markdown
Member

Tested. Working as expected. I could see events for Stop & Start after destroying the vm from host.
Followed the below steps from the issue to reproduce on two environments with and without the patch:

  1. Create a VM with ha enabled offering
  2. Destroy the VM on the hypervisor (in KVM via virsh destroy )
  3. Wait until CS is restarting the VM
  4. Take a look at Events tab of VM and/or Events Tab of main menu
Details

Without patch
image

With patch
image

@vishesh92 vishesh92 merged commit b22315d into apache:4.19 Jun 26, 2024
@vishesh92 vishesh92 deleted the ha-vmstart-event branch June 26, 2024 10:08
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Abhishek's Bugfix List
Status: Done

Development

Successfully merging this pull request may close these issues.

Missing event after ha restart of VM

6 participants