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