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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
