Linstor: add HA support and small cleanups by rp- · Pull Request #8407 · apache/cloudstack

rp- · 2023-12-27T09:56:17Z

Description

This PR add HA support for Linstor primary storage.
It checks output of drbd event and drbd status output, to check if it has quorum or connections to other hosts,
which in turn reveals if this node is isolated or not.

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

Screenshots (if appropriate):

How Has This Been Tested?

Nested HA Linstor cluster with virtualBMC IPMI:

Simulated node crash (kernel panic)
Blocking of drbd incoming and outgoing connections

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

codecov · 2023-12-27T10:50:19Z

Codecov Report

Attention: 89 lines in your changes are missing coverage. Please review.

Comparison is base (af2e277) 30.93% compared to head (2d37f0e) 30.94%.
Report is 1 commits behind head on 4.19.

Files	Patch %	Lines
...oud/hypervisor/kvm/storage/LinstorStoragePool.java	0.00%	70 Missing ⚠️
...cloudstack/storage/datastore/util/LinstorUtil.java	0.00%	8 Missing ⚠️
...torage/datastore/provider/LinstorHostListener.java	0.00%	6 Missing ⚠️
.../hypervisor/kvm/storage/LinstorStorageAdaptor.java	0.00%	3 Missing ⚠️
...tore/driver/LinstorPrimaryDataStoreDriverImpl.java	0.00%	1 Missing ⚠️
.../provider/LinstorPrimaryDatastoreProviderImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #8407      +/-   ##
============================================
+ Coverage     30.93%   30.94%   +0.01%     
- Complexity    34200    34226      +26     
============================================
  Files          5346     5347       +1     
  Lines        375449   375493      +44     
  Branches      54605    54616      +11     
============================================
+ Hits         116140   116200      +60     
+ Misses       244020   243998      -22     
- Partials      15289    15295       +6

Flag	Coverage Δ
simulator-marvin-tests	`24.84% <0.00%> (+0.01%)`	⬆️
uitests	`4.39% <ø> (ø)`
unit-tests	`16.54% <0.00%> (-0.01%)`	⬇️

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.

sureshanaparti · 2023-12-27T11:06:42Z

@blueorangutan package

blueorangutan · 2023-12-27T11:08:03Z

@sureshanaparti 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 · 2023-12-27T12:26:24Z

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

yadvr

LGTM - all changes are in the specific plugin - as long as the maintainer can certify testing/stability and smoketests pass - we should accept it.

DaanHoogland · 2024-02-06T09:02:57Z

clgtm and i agree with rohit, but can you expand a bit on the testing @rp-? (as there is 0% coverage in this PR)

rp- · 2024-02-06T10:25:44Z

clgtm and i agree with rohit, but can you expand a bit on the testing @rp-? (as there is 0% coverage in this PR)

Well as I said, I setup a nested cluster with a VBMC IPMI and configured everything in CloudStack and
tested a few failover scenarios. Like trigger a kernel panic on a node with a running VM (HA enabled) and let it fail over to another node and also see if it got fenced correctly.
The same with the node still seen by CloudStack, but blocking any DRBD traffic and having it look like the node can't access its storage anymore.
This will also trigger the fence action.
Other actions like VM starting/stoping/deleting worked as before.

Do you want me to test any specific scenarios?

DaanHoogland

clgtm, trouwens

rp- · 2024-02-07T12:40:50Z

I made a small change, to don't specify the full path to hostname to remedy concerns from this issue: #8310

github-actions · 2024-02-08T13:19:07Z

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

yadvr · 2024-02-08T18:08:56Z

The hostname issue is probably a critical/blocker for some users, @rp- can you consider a smaller fix for 4.18 branch? Thanks.

yadvr · 2024-02-08T18:34:57Z

@rp- could you help review/fix conflicts and change base branch if it's applicable for 4.18 or 4.19

rp- · 2024-02-09T04:48:58Z

@rp- could you help review/fix conflicts and change base branch if it's applicable for 4.18 or 4.19

rebased to 4.19

rp- · 2024-02-09T04:57:40Z

@rohityadavcloud here #8633 is a PR for 4.18

github-actions · 2024-02-09T11:52:21Z

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

DaanHoogland · 2024-02-12T09:27:29Z

@blueorangutan package

blueorangutan · 2024-02-12T09:28:04Z

@DaanHoogland 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 · 2024-02-12T10:32:08Z

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

yadvr

LGTM - changes are strictly in the linstor plugin that our smoketests can't test. Github actions confirm other tests are passing with simulator.

* linstor: Outline get storagepools from resourcegroup into function * linstor: move getHostname() to kvm/Pool and reimplement * linstor: implement CloudStack HA support

yadvr · 2024-02-22T11:12:27Z

Hi @rp- @Philipp-Reisner quick question - does Linstor currently support VM HA with ACS 4.18/4.19? cc @harikrishna-patnala @borisstoyanov @NuxRo

rp- · 2024-02-22T11:26:21Z

@rohityadavcloud
Not in any released version, this patchset here is needed and also this fix here to get it working with larger setups:
#8670

* linstor: Outline get storagepools from resourcegroup into function * linstor: move getHostname() to kvm/Pool and reimplement * linstor: implement CloudStack HA support

boring-cyborg Bot added component:primary-storage component:storage labels Dec 27, 2023

rp- force-pushed the mydev-main branch from 73392d3 to 8f00d30 Compare December 27, 2023 09:57

sureshanaparti reviewed Dec 27, 2023

View reviewed changes

Comment thread ...e/volume/linstor/src/main/java/org/apache/cloudstack/storage/datastore/util/LinstorUtil.java

yadvr approved these changes Feb 5, 2024

View reviewed changes

DaanHoogland approved these changes Feb 6, 2024

View reviewed changes

rp- force-pushed the mydev-main branch 2 times, most recently from 2c5bcd0 to ce06a0f Compare February 7, 2024 12:45

github-actions Bot added the status:has-conflicts label Feb 8, 2024

yadvr added this to the 4.18.2.0 milestone Feb 8, 2024

rp- force-pushed the mydev-main branch from ce06a0f to c433e76 Compare February 9, 2024 04:47

rp- changed the base branch from main to 4.19 February 9, 2024 04:48

github-actions Bot added status:has-conflicts and removed status:has-conflicts labels Feb 9, 2024

rp- added 3 commits February 9, 2024 13:15

linstor: Outline get storagepools from resourcegroup into function

61f5f2b

linstor: move getHostname() to kvm/Pool and reimplement

f4e3a61

linstor: implement CloudStack HA support

2d37f0e

rp- force-pushed the mydev-main branch from c433e76 to 2d37f0e Compare February 9, 2024 12:17

github-actions Bot removed the status:has-conflicts label Feb 9, 2024

yadvr approved these changes Feb 13, 2024

View reviewed changes

yadvr merged commit 70b634f into apache:4.19 Feb 13, 2024

yadvr modified the milestones: 4.18.2.0, 4.19.1.0 Feb 13, 2024

Conversation

rp- commented Dec 27, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

codecov Bot commented Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

sureshanaparti commented Dec 27, 2023

Uh oh!

blueorangutan commented Dec 27, 2023

Uh oh!

blueorangutan commented Dec 27, 2023

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 6, 2024

Uh oh!

rp- commented Feb 6, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

rp- commented Feb 7, 2024

Uh oh!

github-actions Bot commented Feb 8, 2024

Uh oh!

yadvr commented Feb 8, 2024

Uh oh!

yadvr commented Feb 8, 2024

Uh oh!

rp- commented Feb 9, 2024

Uh oh!

rp- commented Feb 9, 2024

Uh oh!

github-actions Bot commented Feb 9, 2024

Uh oh!

DaanHoogland commented Feb 12, 2024

Uh oh!

blueorangutan commented Feb 12, 2024

Uh oh!

blueorangutan commented Feb 12, 2024

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 22, 2024

Uh oh!

rp- commented Feb 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Dec 27, 2023 •

edited

Loading