You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "stag7824 (via GitHub)" <gi...@apache.org> on 2026/09/09 11:16:09 UTC
[PR] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
stag7824 opened a new pull request, #14096:
URL: https://github.com/apache/cloudstack/pull/14096
### Description
Fixes #14013
A backup repository whose mount options carry stray whitespace, for example `vers=4.1 `,
backs up fine but fails to restore:
mount.nfs: an incorrect mount option was specified for
/usr/share/cloudstack-agent/tmp/csbackup...
`LibvirtRestoreBackupCommandWrapper.mountBackupDirectory` builds the mount command as an argv
list and runs it without a shell:
if (StringUtils.isNotBlank(mountOptions)) {
mountCmd.add("-o");
mountCmd.add(mountOptions);
}
Script.executeCommand(mountCmd.toArray(new String[0]));
`Script.executeCommand(String...)` hands each element to `ProcessBuilder` verbatim, so mount
receives the literal option list `vers=4.1 `. libmount splits that list on commas only, so the
blank stays glued to the last option and the kernel rejects it as part of the option value.
Backup, delete and stats do not hit this today because they pass the options to `nasbackup.sh`,
which expands them unquoted:
mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS})
Shell word splitting drops the blank there, which is why restore is the only operation that
fails: it is the only one of the four that builds the mount command in Java.
Fixed by normalising the option list before it is used. Each comma-separated option is trimmed
and empty ones are dropped. Only the whitespace around the delimiters is removed; the option
text itself is passed through unchanged, since deciding whether an option is well formed
belongs to the API layer rather than the agent.
The normalisation happens **before** the cifs branch appends `,nobrl`. Trimming afterwards
would leave `vers=3.0 ,nobrl`, moving the blank into the middle of the list where libmount
cannot ignore it either.
#### Relationship to #14009
#14009 stops this class of value from being stored: it rejects whitespace per option in
`AddBackupRepositoryCmd` / `UpdateBackupRepositoryCmd`, and it quotes `"${MOUNT_OPTS}"` in
`nasbackup.sh`. That is the right place for validation and I have deliberately not duplicated
any of it here — the two changes touch no common files.
They are complementary rather than alternatives, because #14009 validates on the way in and
cannot clean rows that are already in the database. On an existing deployment such as the one
in #14013, the stored `vers=4.1 ` survives, and once `MOUNT_OPTS` is quoted it will break the
script-based operations too, not just restore. Normalising at the point of use fixes those
repositories without a schema migration.
### Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [x] Minor
### How Has This Been Tested?
Added seven unit tests to `LibvirtRestoreBackupCommandWrapperTest` that invoke
`mountBackupDirectory` and assert on the exact `-o` argument handed to `mount`:
| mount options | repo type | `-o` argument | fails without the fix |
| --- | --- | --- | --- |
| `vers=4.1 ` | nfs | `vers=4.1` | yes |
| ` vers=4.1 , soft ` | nfs | `vers=4.1,soft` | yes |
| `vers=4.1,,soft` | nfs | `vers=4.1,soft` | yes |
| `vers=3.0 ` | cifs | `vers=3.0,nobrl` | yes |
| `username=some user` | cifs | `username=some user,nobrl` | no |
| ` ` | nfs | no `-o` at all | no |
| ` ` | cifs | `nobrl` | no |
The last three already hold today. They are there to pin behaviour the fix must not change:
that trimming is scoped to the delimiters rather than stripping whitespace throughout, and
that an all-whitespace value still collapses to no options at all (or to bare `nobrl` for
cifs) instead of becoming an empty option.
mvn test -pl plugins/hypervisors/kvm -Dtest=LibvirtRestoreBackupCommandWrapperTest
Tests run: 24, Failures: 0, Errors: 0, Skipped: 0
Removing just the `normalizeMountOptions` call and rerunning gives 4 failures, each showing
the blank that mount rejects:
expected:<vers=4.1[]> but was:<vers=4.1[ ]>
expected:<[vers=4.1,soft]> but was:<[ vers=4.1 , soft ]>
expected:<vers=4.1,[]soft> but was:<vers=4.1,[,]soft>
expected:<vers=3.0[],nobrl> but was:<vers=3.0[ ],nobrl>
The last of those is the ordering point: the blank lands in the middle of the list once
`nobrl` is appended after it.
I do not have a KVM zone with a NAS repository available, so this is unit-level only; the
mount argument that the kernel rejects is asserted directly rather than end to end.
The same construction is present on 4.20. I targeted 4.22 because that is the version the
issue is reported against, but happy to retarget if it should land on 4.20 first.
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5675655074
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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] nas: trim whitespace around backup repository mount options [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#discussion_r4297391582
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -217,6 +217,7 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
mountCmd.add(backupRepoType);
mountCmd.add(backupRepoAddress);
mountCmd.add(mountDirectory);
+ mountOptions = normalizeMountOptions(mountOptions);
Review Comment:
that covers it, all five places now go through the one helper in 4b4dd3f
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -242,6 +243,31 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
return mountDirectory;
}
+ /**
+ * Removes the whitespace around each option of a comma-separated mount option list.
+ *
+ * mount(8) is handed the list as a single argument and libmount splits it on commas only, so a
+ * blank next to a comma, or at either end of the list, stays glued to the neighbouring option
+ * and is rejected as part of its name or value.
+ *
+ * Only the whitespace around the delimiters is removed. The option text itself is passed
+ * through unchanged, as rejecting unsafe or malformed options belongs to the API layer, not
+ * to the agent.
+ */
+ protected static String normalizeMountOptions(String mountOptions) {
+ if (StringUtils.isBlank(mountOptions)) {
+ return mountOptions;
+ }
+ List<String> options = new ArrayList<>();
+ for (String option : mountOptions.split(",")) {
+ String trimmedOption = option.trim();
+ if (!trimmedOption.isEmpty()) {
Review Comment:
that covers it, the new test keeps pa,,ss as it is
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#discussion_r4273543961
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -217,6 +217,7 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
mountCmd.add(backupRepoType);
mountCmd.add(backupRepoAddress);
mountCmd.add(mountDirectory);
+ mountOptions = normalizeMountOptions(mountOptions);
Review Comment:
should this trim happen where the stored options are read, so backup, delete and stats get it too? once #14009 quotes them in the script, old rows with a trailing space would break those as well
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -242,6 +243,31 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
return mountDirectory;
}
+ /**
+ * Removes the whitespace around each option of a comma-separated mount option list.
+ *
+ * mount(8) is handed the list as a single argument and libmount splits it on commas only, so a
+ * blank next to a comma, or at either end of the list, stays glued to the neighbouring option
+ * and is rejected as part of its name or value.
+ *
+ * Only the whitespace around the delimiters is removed. The option text itself is passed
+ * through unchanged, as rejecting unsafe or malformed options belongs to the API layer, not
+ * to the agent.
+ */
+ protected static String normalizeMountOptions(String mountOptions) {
+ if (StringUtils.isBlank(mountOptions)) {
+ return mountOptions;
+ }
+ List<String> options = new ArrayList<>();
+ for (String option : mountOptions.split(",")) {
+ String trimmedOption = option.trim();
+ if (!trimmedOption.isEmpty()) {
Review Comment:
what happens with a cifs password that has a comma in it? cifs reads a double comma as a real comma, and dropping the empty option here would change the password
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5601383425
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14096?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 ([`a9b2f33`](https://app.codecov.io/gh/apache/cloudstack/commit/a9b2f333d13186adb6ffaf4183eab2134952ce05?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`afdeb67`](https://app.codecov.io/gh/apache/cloudstack/commit/afdeb6751208368c592e12690365df3370564bd2?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 (a9b2f33) and HEAD (afdeb67). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (a9b2f33) | HEAD (afdeb67) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #14096 +/- ##
=============================================
- Coverage 17.86% 4.02% -13.84%
=============================================
Files 5928 449 -5479
Lines 534479 38192 -496287
Branches 65410 7077 -58333
=============================================
- Hits 95468 1538 -93930
+ Misses 428173 36442 -391731
+ Partials 10838 212 -10626
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14096/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/14096/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/14096/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/14096?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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "stag7824 (via GitHub)" <gi...@apache.org>.
stag7824 commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5607347392
Not on a full CloudStack zone, no — I don't have a KVM host with a NAS backup repository, and I didn't want to imply otherwise in the description.
What I could do is exercise the real code path against a real CIFS server, with nothing mocked. I ran `mountBackupDirectory` itself by reflection — real `Script`, real `ProcessBuilder`, real `mount.cifs` — against a samba share holding a file that exists only on the share, and checked whether that file could be read back afterwards:
| mount options | with this PR | without it |
| --- | --- | --- |
| `guest,vers=3.0 ` (trailing space) | mounted, payload read back | **not mounted** |
| `guest,vers=3.0` (no space, control) | mounted, payload read back | mounted, payload read back |
The control row is there to show the harness isn't just reporting success, and that the change is inert for well-formed options.
The command that the unfixed path actually hands to mount, captured from the process table:
```
/sbin/mount.cifs //127.0.0.1/backup /tmp/csbackup.epddp75190 -o rw,vers=3.0 ,nobrl
```
which is the case in the description — `nobrl` is appended after the blank, so it ends up mid-list rather than at the end.
Worth flagging separately, because it changes how this looks in the field: `Script.executeCommand(String...)` returns `null` on failure rather than throwing, and `mountBackupDirectory` ignores the return value. So a failed mount isn't detected — restore carries on against an empty temp directory and fails later with a confusing error rather than a mount error. That's pre-existing and I've deliberately left it out of this PR, but happy to raise it as its own issue if you'd like.
What I still can't cover locally is the full path: management server, agent, a real backup and restore of a VM volume. If that's needed before this can go in, could someone with a NAS repository confirm it? Happy to adjust the approach — including moving the normalisation elsewhere — if you'd rather it not live in the agent.
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "stag7824 (via GitHub)" <gi...@apache.org>.
stag7824 commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5948371667
Synced with 4.22 to resolve the conflict with #14006, which rewrote `mountBackupDirectory` to check the mount's exit value. The normalisation still runs before the cifs `nobrl` append and before the mount, and the seven tests now capture the command through `Script.executeCommandForExitValue(timeout, ...)` instead of `executeCommand`. The diff against 4.22 is still just the two files.
```
mvn test -pl plugins/hypervisors/kvm -Dtest=LibvirtRestoreBackupCommandWrapperTest
Tests run: 27, Failures: 0, Errors: 0, Skipped: 0
```
With the `normalizeMountOptions` call removed, the same four tests fail as before.
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5601248323
@stag7824
have you verified it on a cloudstack environment ?
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "stag7824 (via GitHub)" <gi...@apache.org>.
stag7824 commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-6111725917
For anyone looking at CI: the 8 workflow runs on the current head (`2c9c306`) have been waiting for approval since Oct 2, when I synced with 4.22 after #14006, so nothing has run since that sync. Locally the test class passes (27 tests), and the 4 new tests that fail without the change still do.
@weizhouapache, you had asked about testing earlier: the evidence from the CIFS check is in my comment above. Could you or another committer approve the workflow runs? Thanks.
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#issuecomment-5600940915
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:
- In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
- Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
- Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
- Please follow [ASF Code of Conduct](https://github.com/apache/.github/blob/main/.github/CODE_OF_CONDUCT.md) for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
- Be sure to read the [CloudStack Coding Conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions).
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/
--
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] kvm: trim whitespace around backup repository mount options on restore [cloudstack]
Posted by "stag7824 (via GitHub)" <gi...@apache.org>.
stag7824 commented on code in PR #14096:
URL: https://github.com/apache/cloudstack/pull/14096#discussion_r4297367034
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -217,6 +217,7 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
mountCmd.add(backupRepoType);
mountCmd.add(backupRepoAddress);
mountCmd.add(mountDirectory);
+ mountOptions = normalizeMountOptions(mountOptions);
Review Comment:
Yes, that's the better place. I've moved it to `NASBackupProvider` (4b4dd3f): the five places that pass `backupRepository.getMountOptions()` to a command (take, both restores, delete, stats) now go through one helper, and the change in the KVM restore wrapper is reverted. So backup, delete and stats get the same option list as restore, and old rows keep working once #14009 quotes the options.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -242,6 +243,31 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
return mountDirectory;
}
+ /**
+ * Removes the whitespace around each option of a comma-separated mount option list.
+ *
+ * mount(8) is handed the list as a single argument and libmount splits it on commas only, so a
+ * blank next to a comma, or at either end of the list, stays glued to the neighbouring option
+ * and is rejected as part of its name or value.
+ *
+ * Only the whitespace around the delimiters is removed. The option text itself is passed
+ * through unchanged, as rejecting unsafe or malformed options belongs to the API layer, not
+ * to the agent.
+ */
+ protected static String normalizeMountOptions(String mountOptions) {
+ if (StringUtils.isBlank(mountOptions)) {
+ return mountOptions;
+ }
+ List<String> options = new ArrayList<>();
+ for (String option : mountOptions.split(",")) {
+ String trimmedOption = option.trim();
+ if (!trimmedOption.isEmpty()) {
Review Comment:
Good catch, dropping the empty option would turn `pa,,ss` into `pa,ss`. Now only options that are whitespace are dropped; an empty one is kept, with a test for a password containing a double comma.
For what it's worth, `mount.cifs` itself splits on every comma before the kernel sees the list, and its man page notes that a password containing a comma fails on the command line, so this mostly matters for the kernel path, but the normalisation shouldn't make it worse either way.
--
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