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

linstor: Do not pretend handling disconnect paths that are non Linstor #8897

Conversation

rp-
Copy link
Contributor

@rp- rp- commented Apr 10, 2024

Description

Excerpt PR from #8790, that only fixes Linstor adaptor pretending handling paths that don't belong to Linstor.

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?

Manual with a Linstor cluster.

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

Copy link
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, trusting provider specoific testing and will run regression suits

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@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

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link

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

Copy link
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.

clgtm

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

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

LGTM. Manually tested.
I created a VM on a KVM enviroment; then, I sent a stop VM command:

  • Without this patch, forced other adaptors to fail so that Linstor would run (using a debug tool), even not having Linstor in my environment, disconnectPhysicalDisk would return true.
  • After the patch, redid the same test, and the method returns false.

This testing is limited in a sense that I cannot test the case were Linstor should actually return true, but at least we shouldn't have any regressions for users that do not use Linstor.

@blueorangutan
Copy link

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

Test Result Time (s) Test File
test_01_migrate_VM_and_root_volume Error 91.66 test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks Error 55.07 test_vm_life_cycle.py
test_08_migrate_vm Error 0.07 test_vm_life_cycle.py

@JoaoJandre
Copy link
Contributor

@DaanHoogland From what I've looked at the errors from #8897 (comment), them look like an environment instability; could we run the CI once more just to be safe?

@JoaoJandre
Copy link
Contributor

@sureshanaparti @rohityadavcloud @shwstppr could we run the CI again?

@blueorangutan
Copy link

[SF] Trillian test result (tid-9795)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 43358 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8897-t9795-kvm-rocky8.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

@rohityadavcloud rohityadavcloud added this to the 4.18.2.0 milestone Apr 12, 2024
@rohityadavcloud
Copy link
Member

Rekicked CI by close/reopen.

@blueorangutan package

@blueorangutan
Copy link

@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

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

@DaanHoogland
Copy link
Contributor

@sureshanaparti @rohityadavcloud @shwstppr could we run the CI again?

@JoaoJandre I had already started it in the background and it seems succeful:

[SF] Trillian test result (tid-9795) Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8 Total time taken: 43358 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8897-t9795-kvm-rocky8.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

sorry, for the lack of communication.

@JoaoJandre
Copy link
Contributor

Merging based on reviews, CI result and manual tests

@JoaoJandre JoaoJandre merged commit 6cd5c6a into apache:4.18 Apr 12, 2024
46 of 48 checks passed
@rp- rp- deleted the linstor-4.18-fix-pretending-handling-non-linstor-resources branch April 12, 2024 15:04
@rp-
Copy link
Contributor Author

rp- commented Apr 15, 2024

@DaanHoogland Who would merge this into 4.19 and main?

@DaanHoogland
Copy link
Contributor

@rp- , RMs do so on a regular basis. A lot of merge commits can be found. On any branche not in freeze you/any of us can as well. There is a tool git-fwd-merge in ./tools/git that can do it and rolls back on conflicts. when on the path it can be called as git fwd-merge. A regular git merge works as well when conflicts occur.

rp- added a commit to LINBIT/cloudstack that referenced this pull request Apr 17, 2024
@DaanHoogland
Copy link
Contributor

@rp- I merged a bunch forward including this. There was a conflict and I think I resolved it but you might want to check the method disconnectPhysicalDiskByPath.

@rp-
Copy link
Contributor Author

rp- commented Apr 18, 2024

@rp- I merged a bunch forward including this. There was a conflict and I think I resolved it but you might want to check the method disconnectPhysicalDiskByPath.

Looks fine for me

rp- added a commit to LINBIT/cloudstack that referenced this pull request Apr 18, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request May 3, 2024
rp- added a commit to LINBIT/cloudstack that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants