You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2026/10/01 15:28:57 UTC
[PR] deal with xcpng refactors [cloudstack]
DaanHoogland opened a new pull request, #14287:
URL: https://github.com/apache/cloudstack/pull/14287
### Description
This PR...
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
Fixes: #13292
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### 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
- [ ] Test (unit or integration test code)
### 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?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with xcpng refactors [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #14287:
URL: https://github.com/apache/cloudstack/pull/14287#discussion_r4258264532
##########
scripts/vm/hypervisor/xenserver/xcpserver83/NFSSR.py:
##########
@@ -28,6 +28,13 @@
from lock import Lock
import cleanup
+try:
+ # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where LOCK_TYPE_SR moved to lock.py
+ from lock import LOCK_TYPE_SR
+except ImportError:
+ # older sm (XenServer and XCP-ng releases prior to the 8.3 SM refactor)
+ LOCK_TYPE_SR = vhdutil.LOCK_TYPE_SR
Review Comment:
In the fallback branch, `vhdutil` is referenced without being imported in this block. If `vhdutil` is not imported earlier in the file, this will raise `NameError` at import time. Make the fallback branch self-contained by importing `vhdutil` there (or importing `LOCK_TYPE_SR` directly from the legacy module where it exists).
##########
scripts/vm/hypervisor/xenserver/xenserver84/cloud-plugin-storage:
##########
@@ -41,6 +39,26 @@ import random
import cloudstack_pluginlib as lib
import logging
+try:
+ # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where lvhdutil was folded into
+ # constants/lvmcowutil and vhdutil became a class
+ from constants import VG_PREFIX, VG_LOCATION
+ from lvmcowutil import LvmCowUtil
+ from vhdutil import VhdUtil
+ _vhdutil = VhdUtil()
+ lvhdutil_extractUuid = LvmCowUtil.extractUuid
+ vhdutil_getParent = _vhdutil.getParent
+ vhdutil_getVHDInfoLVM = _vhdutil.getInfoFromLVM
+except ImportError:
+ # older sm (XenServer and XCP-ng releases prior to the 8.3 SM refactor)
+ import vhdutil
+ import lvhdutil
+ VG_PREFIX = lvhdutil.VG_PREFIX
+ VG_LOCATION = lvhdutil.VG_LOCATION
+ lvhdutil_extractUuid = lvhdutil.extractUuid
+ vhdutil_getParent = vhdutil.getParent
+ vhdutil_getVHDInfoLVM = vhdutil.getVHDInfoLVM
Review Comment:
This SM-compatibility shim is duplicated (also in `xenserver84/vmopsSnapshot`). To reduce the risk of drift (e.g., function mappings diverging across scripts), consider extracting this into a shared helper module under the same directory and importing it from both scripts (exporting `VG_PREFIX/VG_LOCATION` and the `vhdutil_*`/`lvhdutil_*` callables). This should make future SM changes easier to handle in one place.
##########
scripts/vm/hypervisor/xenserver/xenserver84/vmopsSnapshot:
##########
@@ -41,6 +39,30 @@ import random
import cloudstack_pluginlib as lib
import logging
+try:
+ # sm >= 3.2.12-18 (e.g. XCP-ng 8.3), where lvhdutil was folded into
+ # constants/lvmcowutil and vhdutil became a class
+ from constants import VG_PREFIX, VG_LOCATION
+ from lvmcowutil import LvmCowUtil
+ from vhdutil import VhdUtil
Review Comment:
The PR description is still the template and doesn’t explain the behavioral change or validation. Please update the PR description to state that this introduces runtime compatibility for the XCP-ng SM refactor (vhdutil/lvhdutil + LOCK_TYPE_SR moves), and include how it was tested (at least one “old SM” and one “new SM” environment/version).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with xcpng refactors [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14287:
URL: https://github.com/apache/cloudstack/pull/14287#issuecomment-5934803360
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14287?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 4.02%. Comparing base ([`2e63c60`](https://app.codecov.io/gh/apache/cloudstack/commit/2e63c601fc224de20c345172d9b805ffb6bdbeac?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f2d5e10`](https://app.codecov.io/gh/apache/cloudstack/commit/f2d5e100f3ab2b85793d3a2f32267b83765bc190?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (2e63c60) and HEAD (f2d5e10). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (2e63c60) | HEAD (f2d5e10) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #14287 +/- ##
=============================================
- Coverage 18.00% 4.02% -13.99%
=============================================
Files 5936 449 -5487
Lines 535716 38239 -497477
Branches 65596 7082 -58514
=============================================
- Hits 96459 1538 -94921
+ Misses 428268 36489 -391779
+ Partials 10989 212 -10777
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14287/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14287/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.02% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14287/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14287?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with xcpng refactors [cloudstack]
Posted by "mvds-bbtk (via GitHub)" <gi...@apache.org>.
mvds-bbtk commented on PR #14287:
URL: https://github.com/apache/cloudstack/pull/14287#issuecomment-6299007639
Hi Daan, instead of patching the old NFSSR.py, can't we just use the stock version? The code to create SR's already contains the -nosub parameter. It would be one less file to patch and maintain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] deal with xcpng refactors [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #14287:
URL: https://github.com/apache/cloudstack/pull/14287#issuecomment-6339806175
> Hi Daan, instead of patching the old NFSSR.py, can't we just use the stock version that comes with XCP-NG? The code to create SR's already contains the -nosub parameter. It would be one less file to patch and maintain. Or do you know of any other reason that driver was forked?
I'm fine with that. I am not a daily xen user and am willing to drop this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org