Use curl for ssh dumps #59
Conversation
Checking for the Opalcore currently requires duplicate code. Simplify it by always checking if an Opalcore exists at the beginning of the script. Signed-off-by: Philipp Rudo <prudo@redhat.com>
Move the ssh host to a global variable to make it available for new functions introduced in following commits. While at it rename $SSH_KEY_LOCATION to $SSH_KEY to shorten the name and be consistent with the naming schema of the other variables. Signed-off-by: Philipp Rudo <prudo@redhat.com>
In dump_ssh the _ssh_opts need to be word split, which triggers ShellChecks SC2086 warning. This warning is currently disabled for all calls to ssh/scp. However in the ShellCheck wiki [1] the suggested solution for POSIX compatible code is to make the call in a separate function. This also makes disabling of SC2029 obsolete. One big benefit of this solution is that now calls to ssh/scp can also be made outside of dump_ssh without redefining _ssh_opts. One small downside is that the options passed to ssh/scp must now be maintained in each new function separately. Compared to the benefit described above this is a small price to pay. Note: Currently ssh/scp option -q,--quiet is used inconsistently. With this commit it will always be set. [1] https://www.shellcheck.net/wiki/SC2086 Signed-off-by: Philipp Rudo <prudo@redhat.com>
The mount point for file system dumps is currently passed as an argument to dump_fs. But there are only two possible values for the mount point. Its either $NEWROOT for failure_action dump_to_rootfs or whatever the user provided in kdump.conf. Thus instead of passing the mount point as argument use a global variable and update it when parsing the config and use that in dump_fs. Reuse $NEWROOT for this. Signed-off-by: Philipp Rudo <prudo@redhat.com>
Both dump_fs and dump_ssh currently define their own, almost identical,
directories they use to write the dump to. This not only adds duplicate
code but also prevents any code outside of dump_{fs,ssh} to access the
dump directory. Thus use a global definition for said directory. Reuse
the already existing $KDUMP_PATH for that.
Signed-off-by: Philipp Rudo <prudo@redhat.com>
This commit contains two major changes for dumps via ssh. One is the
switch from scp to sftp as protocol to transfer files. Using sftp has
two big advantages
1. It allows a wide variety of file operations, e.g. renaming files,
on the remote host.
2. It can work with files of unknown size, i.e. read from a pipe.
sftp is fully supported by OpenSSH, in fact since OpenSSH 9.0 (released
April 2022) the 'scp' command uses sftp internally by default.
The other big change is to make use of curl rather than ssh/scp/sftp
directly. This is mainly because curl provides a cleaner user interface
for now. But curl also supports a big variety of different protocols and
thus could be used to extend support to dump via e.g. http in the future.
Signed-off-by: Philipp Rudo <prudo@redhat.com>
When fetching the test status from the target a missing status file is
always considered a 'failure'. So there is no need to explicitly setting
the status to 'failure' in the initrd. This allows simplifying the code
a bit. For example we can now assume that the directory for $KDUMP_PATH
always exists (otherwise dump_{fs,ssh} would have returned with an
error).
Signed-off-by: Philipp Rudo <prudo@redhat.com>
prudo1
force-pushed
the
features/curl/main
branch
from
77987c3 to
e6e421a
Compare
|
Rebased to current main as PR #53 was merged. Updated the description accordingly. No code changes made. |
|
Hi @prudo1, I like current approach as it also significantly simplifies the code! I notice one problem when testing the code. Since this PR makes scp and ssh obsolete, I remove the dependency on the ssh-client dracut module, diff --git a/dracut/99kdumpbase/module-setup.sh b/dracut/99kdumpbase/module-setup.sh
index 2a61a4ce..ae782d59 100755
--- a/dracut/99kdumpbase/module-setup.sh
+++ b/dracut/99kdumpbase/module-setup.sh
@@ -49,7 +49,7 @@ depends() {
fi
if is_ssh_dump_target; then
- _dep="$_dep ssh-client"
+ _dep="$_dep network"
fi
if is_lvm2_thinp_dump_target; thenunfortunately then the ssh dumping test fails, Note the test works without removing the dependency on the ssh-client dracut module. Do you know why? I'm also curious to ask why do you think switching to curl is controversial. Do you foresee any potential issue? It seems using curl only bring benefits and curl is always available to RHEL/Fedora. |
|
Hi @coiby,
I'm not entirely sure how/if
It's mainly because the code for dumping via |
Thanks for the clarification! Indeed, ssh keys and known_hosts needs to be installed in order for curl to work. And the exitcode 60 means curl can't verify the legitimacy of the server because of missing. If we don't call with
I thought this change is internal and doesn't require any change from users. Or do I miss something?
ssh/curl depends on a working network but setting up kdump network is not done by ssh/curl. So I think this concern can be safely dismissed. |
|
Hi @coiby,
The change is purely internal. There shouldn't be any visible change for users (besides a few slightly different error/info messages).
True, but there could still be firewalls settings that prevent a connection to the server to be setup. Even with an otherwise working network connection. But as Anyway, I'm dropping the RFC. Feel free to merge if you like. |
| -o StrictHostKeyChecking=yes \ | ||
| "$SSH_HOST" "$@" | ||
| _curl() { | ||
| curl --silent \ |
There was a problem hiding this comment.
I don't know why ssh/scp option -q was used before. But I think it's better to drop --silent in case in some corners cases curl does fail.
There was a problem hiding this comment.
Hi @coiby,
yeah, what confused me most was the inconsistent use of -q...
You are right that --silent should be replaced here as it also masks error messages. But I don't think we should simply drop it. If we do so the scripts output will be quite cluttered as every file transferred will print it's progress. Which is quite annoying, when the progress printed by curl and makedumpfile are interleaved. That's why I think replacing --silent with --no-progress-meter makes more sense here.
| derror "Unknown test status $_status" | ||
| return 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Hi @liutgnu, does this commit looks good to you? I'm not sure if checking "Unknown test status" is necessary.
There was a problem hiding this comment.
Hi @coiby,
the "Unknown test status" was only a safeguard for typos within the script, i.e. to make it obvious when the function got called with an improper $_status.
|
Hi @coiby @prudo1 hi, can you invite some networking people to have a look (mainly see if any corener cases about curl/ssh differences, there was a report here although original post deleted: https://www.reddit.com/r/linux4noobs/comments/dwsugz/curl_much_slower_over_ssh/ and curl/curl#9336)? Otherwise any difference of installed initramfs size with the change? Another thing is the runtime memory footprint also need consideration. Enough test and review will be better before making the change formally. About test, I'd suggest to test on large memory systems eg. over 1T, compare the saving time, and another case is to measure and compare the memory use |
|
Hi @jacekmigacz, I see you maintain RHEL's curl. We want to switch from ssh/scp to curl because of its cleaner interface and its rich features. Do you think it is a good idea? Or do you have any concern in mind? Thanks! |
|
Hi @daveyoung,
the initrd get's slightly bigger by adding libcurl and the curl binary by almost exactly 1MB. For the runtime memory usage and transfere time I need to run some additional tests. |
4 participants
Hi everybody,
This PR started out as a cleanup for ssh dumps by using
curlbut picked up a few other cleanups to support thekdumpctl testchanges made by @liutgnu in PR #53 on its way. The rational behind usingcurlis that it has a much cleaner UI compared to the home grown mix ofsshandscp. In addition it also supports a wide range of other protocols which could be leveraged to extend support to dump via e.g. http in the future.I can imagine that the switch to
curlcan be controversial. So I'm posting this as a RFC for now.