Skip to content

linstor: cleanup diskless nodes on disconnect#8790

Merged
DaanHoogland merged 1 commit intoapache:4.18from
LINBIT:linstor-4.18-cleanup-diskless-nodes
Apr 26, 2024
Merged

linstor: cleanup diskless nodes on disconnect#8790
DaanHoogland merged 1 commit intoapache:4.18from
LINBIT:linstor-4.18-cleanup-diskless-nodes

Conversation

@rp-
Copy link
Copy Markdown
Contributor

@rp- rp- commented Mar 14, 2024

Description

If disconnect is running on a diskless node, try to cleanup the resource.

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?

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

Moving/Stopping resources on a hyperconverged and non hyperconverged linstor cluster

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 13.16%. Comparing base (f8fd22c) to head (af48a83).
Report is 3 commits behind head on 4.18.

Files Patch % Lines
.../hypervisor/kvm/storage/LinstorStorageAdaptor.java 0.00% 41 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.18    #8790      +/-   ##
============================================
- Coverage     13.16%   13.16%   -0.01%     
+ Complexity     9205     9203       -2     
============================================
  Files          2724     2724              
  Lines        258149   258170      +21     
  Branches      40235    40240       +5     
============================================
- Hits          33987    33982       -5     
- Misses       219856   219883      +27     
+ Partials       4306     4305       -1     

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

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

@rp- rp- force-pushed the linstor-4.18-cleanup-diskless-nodes branch from 083d49c to af48a83 Compare March 25, 2024 11:28
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

code lgtm

@sureshanaparti
Copy link
Copy Markdown
Contributor

@blueorangutan package

@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Mar 27, 2024

@DaanHoogland
Is this planned for 4.18.2.0?
As described, if not, this will introduce a bug with disconnectPhysicalDiskByPath for other storage providers

@DaanHoogland
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@JoaoJandre
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Apr 2, 2024

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@DaanHoogland
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@rp- , I have no way to test this (i think). @JoaoJandre do you use linstor?

@JoaoJandre
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@rp- , I have no way to test this (i think). @JoaoJandre do you use linstor?

@rp- are you the only known linstor developer/user? @DaanHoogland do you know if we have anyone else in the community that works with linstor?

@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Apr 2, 2024

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@rp- , I have no way to test this (i think). @JoaoJandre do you use linstor?

@rp- are you the only known linstor developer/user? @DaanHoogland do you know if we have anyone else in the community that works with linstor?

I'm for sure the only Linstor-plugin dev, but we have some customers and I know Rohit has a Linstor cluster at home, but I guess already at 4.19...

@DaanHoogland
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@rp- , I have no way to test this (i think). @JoaoJandre do you use linstor?

@rp- are you the only known linstor developer/user? @DaanHoogland do you know if we have anyone else in the community that works with linstor?

I'm for sure the only Linstor-plugin dev, but we have some customers and I know Rohit has a Linstor cluster at home, but I guess already at 4.19...

@Rohit, can you have a look at this?

@JoaoJandre
Copy link
Copy Markdown
Contributor

@JoaoJandre , as @rp- describes this will prevent a bug being introduced. Can we add it to 4.18.2?

@DaanHoogland sure, but some third party testing would be good.

Anyway I can help here?

@rp- , I have no way to test this (i think). @JoaoJandre do you use linstor?

@rp- are you the only known linstor developer/user? @DaanHoogland do you know if we have anyone else in the community that works with linstor?

I'm for sure the only Linstor-plugin dev, but we have some customers and I know Rohit has a Linstor cluster at home, but I guess already at 4.19...

@Rohit, can you have a look at this?

@DaanHoogland I think you meant @rohityadavcloud, right?

@JoaoJandre
Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@JoaoJandre 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 9196

@JoaoJandre
Copy link
Copy Markdown
Contributor

@DaanHoogland can we run the CI here?

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

[SF] Trillian Build Failed (tid-9761)

@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

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@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
Copy link
Copy Markdown

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

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

[SF] Trillian test result (tid-9832)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 43639 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8790-t9832-kvm-alma9.zip
Smoke tests completed. 110 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@rp- rp- force-pushed the linstor-4.18-cleanup-diskless-nodes branch from 6617b08 to 5a99e90 Compare April 17, 2024 13:04
@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Apr 17, 2024

I have fixed a NPE if the device paths had been null (reported from Linstor, because a node was down)

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan package

If disconnect is running on a diskless node, try to cleanup the resource.
@rp- rp- force-pushed the linstor-4.18-cleanup-diskless-nodes branch from 5a99e90 to 4855ba7 Compare April 17, 2024 13:15
@blueorangutan
Copy link
Copy Markdown

@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
Copy link
Copy Markdown

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

@JoaoJandre JoaoJandre modified the milestones: 4.18.2.0, 4.18.3 Apr 17, 2024
@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link
Copy Markdown

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

@blueorangutan
Copy link
Copy Markdown

[SF] Trillian test result (tid-9885)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 49564 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8790-t9885-kvm-alma9.zip
Smoke tests completed. 108 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_attach_and_distribute_multiple_volumes Failure 27.44 test_attach_multiple_volumes.py
test_01_migrate_VM_and_root_volume Error 94.66 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 52.87 test_vm_life_cycle.py
test_08_migrate_vm Error 47.27 test_vm_life_cycle.py

@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Apr 26, 2024

@DaanHoogland can we merge this now?

@DaanHoogland DaanHoogland merged commit 9d5d4e5 into apache:4.18 Apr 26, 2024
@DaanHoogland
Copy link
Copy Markdown
Contributor

DaanHoogland commented Apr 29, 2024

@rp- , this is causing some conflicts in merge forward. Can you advise how tryDisconnectLinstor should be implemented in main?

I think I know but please review in main once I have pushed.

@rp-
Copy link
Copy Markdown
Contributor Author

rp- commented Apr 29, 2024

@rp- , this is causing some conflicts in merge forward. Can you advise how tryDisconnectLinstor should be implemented in main?

I think I know but please review in main once I have pushed.

I put the cherry-picked/resolved here: https://github.com/LINBIT/cloudstack/tree/linstor-main-cleanup-diskless

@DaanHoogland
Copy link
Copy Markdown
Contributor

I think I got it right and running a test compile now. if that passes , I'll push.

@rp- rp- mentioned this pull request May 2, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request May 3, 2024
@DaanHoogland DaanHoogland modified the milestones: 4.18.3, 4.19.1.0 May 7, 2024
rp- added a commit to LINBIT/cloudstack that referenced this pull request May 16, 2024
@rp- rp- deleted the linstor-4.18-cleanup-diskless-nodes branch July 4, 2024 13:04
rp- added a commit to LINBIT/cloudstack that referenced this pull request Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

8 participants