Update libssh to 0.11.1 #3735

Sploder12 · 2024-10-16T19:26:56Z

Sets the timeout to infinite since no messages for extended periods of time is expected behavior. Libssh updated to fix timeout being ignored.
Sets the pty mode explicitly to default since Libssh updated to inherit settings from stdin.

Fixes #3745

MULTI-1528

codecov · 2024-10-16T19:50:23Z

Codecov Report

Attention: Patch coverage is 67.85714% with 9 lines in your changes missing coverage. Please review.

Project coverage is 89.00%. Comparing base (7074116) to head (ada20ce).

Files with missing lines	Patch %	Lines
src/platform/platform_unix.cpp	64.70%	6 Missing ⚠️
src/sshfs_mount/sshfs_mount.cpp	75.00%	2 Missing ⚠️
src/platform/console/unix_console.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3735      +/-   ##
==========================================
+ Coverage   88.94%   89.00%   +0.05%     
==========================================
  Files         256      255       -1     
  Lines       14584    14599      +15     
==========================================
+ Hits        12972    12994      +22     
+ Misses       1612     1605       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ricab

Nice work Trevor, thanks. I ask only for some unit tests for the new code. Other than that, +1 on my secondary review.

Sploder12 · 2024-10-21T14:40:10Z

Some reproduction steps for the issues.

Formatting:

Run multipass launch -n <instance-name>
Run multipass shell <instance-name>
Observe incorrectly formatted shell and no ECHO

Mounts:

Run multipass launch -n <instance-name>
Run multipass mount <source> <target>
Wait ~30 seconds after mount finishes mounting
Run multipass shell <instance-name> (GUI doesn't have shell formatting issues)
Attempt to navigate to the files in <target>
Run ls or cd
Observe shell hang

georgeliao

Hi @Sploder12

Thanks for the good work, the fix seems to be solid. I only have a few minor comments and questions.

src/platform/console/unix_console.cpp

src/sshfs_mount/sshfs_mount.cpp

georgeliao · 2024-12-03T09:24:55Z

src/sshfs_mount/sshfs_mount.cpp

+                           const std::string& target,
+                           const mp::id_mappings& gid_mappings,
+                           const mp::id_mappings& uid_mappings)
+    : running{true},


Should setting running to be true be in the sftp_thread as a way of localizing the variable?

It should not be true until the sftp_server is running, so maybe before sftp_server->run(); is more appropriate. What do you think?

I had the same intuition, but I believe this needs to be alive right after construction, so that the watchdog doesn't quit before the thread starts.

To avoid confusion, one alternative would be to have an enum to distinguish the running state, instead of a bool. Something like "unstarted, running, stopped". Then in the watchdog condition, look for stopped. The logic would be the same, but it would help readers and future devs not move this and introduce a bug. WDYT?

Ricardo's belief is correct. The watchdog would immediately exit if the thread takes a while to start. I think the enum approach would work well though.

ok, i see, then the enum approach is fine.

src/sshfs_mount/sshfs_mount.h

georgeliao · 2024-12-03T09:44:24Z

src/ssh/ssh_session.cpp

    : session{ssh_new(), ssh_free}, mut{}
 {
    if (session == nullptr)
        throw mp::SSHException("could not allocate ssh session");

-    const long timeout_secs = std::chrono::duration_cast<std::chrono::seconds>(timeout).count();
+    const long timeout_secs = std::numeric_limits<long>::max();


About this change, I think the timeout_secs is used to set the SSH_OPTIONS_TIMEOUT, and this option is the timeout value for ssh session connection establishment wait time (not entire sure about this, hard to find a clear documentation as well). Therefore, having a reasonable value (like the original 20 seconds) seems to makes sense here.

Besides, with your watchdog fix ( which is essentially checking the sftp_threading running together with other SIGQUIT, SIGTERM, SIGHUP signals), the program already behaves correctly. So I am doubting whether we still make the time out change here. What do you think?

Wasn't this a keep-alive timeout? A better var name might help here.

The program will not behave correctly with a reasonable timeout unfortunately. After the timeout expires, the sshfs_server process will exit, the watchdog fix makes it so that it doesn't hang. It behaves like a keep-alive timeout, but libssh has no keep-alive messages.

ok, so it looks like a keep-alive timeout as opposed to ssh session connection establishment wait time, interesting. It is a weird option offered to users though.

I do have a wondering here though, the SSH_OPTIONS_TIMEOUT is the a ssh option. If it were a keep-alive timeout, should it be the timeout of ssh session alive? If so, should the multipass shell hang without mount?

multipass shell doesn't hang since it's setup to automatically reconnect. You can see this behavior in #3810 where the SSH session is restarting every 20 seconds. Also with normal use it's much more likely there is some SSH traffic within the 20 seconds compared to SFTP which is likely to not get any traffic for long periods of time.

src/platform/platform_unix.cpp

ricab

Great PR @Sploder12! And nice review as well @georgeliao 😃

A few comments on my secondary review.

src/platform/platform_unix.cpp

include/multipass/platform.h

ricab · 2024-12-03T11:55:45Z

src/sshfs_mount/sshfs_mount.cpp

+                           const std::string& target,
+                           const mp::id_mappings& gid_mappings,
+                           const mp::id_mappings& uid_mappings)
+    : running{true},


I had the same intuition, but I believe this needs to be alive right after construction, so that the watchdog doesn't quit before the thread starts.

To avoid confusion, one alternative would be to have an enum to distinguish the running state, instead of a bool. Something like "unstarted, running, stopped". Then in the watchdog condition, look for stopped. The logic would be the same, but it would help readers and future devs not move this and introduce a bug. WDYT?

ricab · 2024-12-03T12:00:32Z

src/ssh/ssh_session.cpp

    : session{ssh_new(), ssh_free}, mut{}
 {
    if (session == nullptr)
        throw mp::SSHException("could not allocate ssh session");

-    const long timeout_secs = std::chrono::duration_cast<std::chrono::seconds>(timeout).count();
+    const long timeout_secs = std::numeric_limits<long>::max();


Wasn't this a keep-alive timeout? A better var name might help here.

src/platform/platform_unix.cpp

georgeliao · 2024-12-11T16:59:34Z

src/platform/platform_unix.cpp

+        int sig = SIGUSR2;
+
+        // A signal generator that triggers after `timeout`
+        AutoJoinThread signaler([&sig_mtx, &sig_cv, &sig, &timeout, signalee = pthread_self()] {


Ok, we need to replace this in house utility when std::jthread is available.

src/platform/platform_unix.cpp

include/multipass/platform.h

ricab

Hey Trevor, the code looks good to me, but I would still love to see some unit tests on that make_quit_watchdog. That stuff is quite complex and you made a nice job of it :) However, the logic is inherently tricky and error prone (now and in the future).

An option to make testing easier would be to use the Timer class, which is already tested (at least partially). Let me know what you think.

src/platform/platform_unix.cpp

ricab · 2024-12-17T12:24:24Z

src/platform/platform_unix.cpp

+        });
+
+        // wait on signals and condition
+        int ret = SIGUSR2;


Can this name be improved a little, please? This is not returned anywhere. I know it is used for the output of sigwait, but I would expect ret to be reserved for something we return, not the return of a function we call. Perhaps latest_sig or something of the sort. Feel free to come up with something better.

src/platform/platform_unix.cpp

Sploder12 · 2024-12-17T15:23:37Z

@ricab I can try to write unit tests. Timer won't help much since the main issue is the signals. Integration tests would be needed to properly test this too, sending SIGTERM is a bit too dangerous for a unit test. Where/How would I write an integration test?

Sploder12 force-pushed the update-libssh branch from da5d0fc to 79996f8 Compare October 18, 2024 16:49

Sploder12 marked this pull request as ready for review October 18, 2024 18:54

ricab requested a review from georgeliao October 21, 2024 09:06

ricab reviewed Oct 21, 2024

View reviewed changes

sharder996 added this to the 1.15.0 milestone Oct 30, 2024

lars18th mentioned this pull request Nov 15, 2024

multipass.exe in Windows is not redirecting STDIN #3749

Open

Sploder12 force-pushed the update-libssh branch 2 times, most recently from de690f1 to 96d1a01 Compare November 27, 2024 20:17

ricab modified the milestones: 1.15.0, 1.15.1 Nov 29, 2024

georgeliao reviewed Dec 3, 2024

View reviewed changes

ricab reviewed Dec 3, 2024

View reviewed changes

Sploder12 marked this pull request as draft December 4, 2024 19:28

Sploder12 force-pushed the update-libssh branch from 16562f3 to 3df817f Compare December 5, 2024 15:56

Sploder12 marked this pull request as ready for review December 9, 2024 21:41

georgeliao reviewed Dec 11, 2024

View reviewed changes

src/platform/platform_unix.cpp Outdated Show resolved Hide resolved

georgeliao reviewed Dec 11, 2024

View reviewed changes

src/platform/platform_unix.cpp Outdated Show resolved Hide resolved

georgeliao reviewed Dec 12, 2024

View reviewed changes

src/platform/platform_unix.cpp Outdated Show resolved Hide resolved

ricab reviewed Dec 13, 2024

View reviewed changes

include/multipass/platform.h Outdated Show resolved Hide resolved

include/multipass/platform.h Show resolved Hide resolved

ricab reviewed Dec 17, 2024

View reviewed changes

ricab mentioned this pull request Dec 17, 2024

Unix drive-by tweaks #3847

Open

Sploder12 marked this pull request as draft December 17, 2024 15:28

Sploder12 marked this pull request as ready for review December 17, 2024 20:23

Sploder12 added 3 commits December 17, 2024 16:51

[libssh] Reapply previous version

25c3b38

[ssh] Set timeout to infinite

8298caf

[ssh] Do not inherit settings from stdin

0e2e120

Sploder12 added 15 commits December 17, 2024 16:51

[ssh] Apply missed formatting

1da0449

[ssh] Set stdin to Raw only after libssh inherits sane settings

bc304c7

[ssh] Fix sshfs process ignoring child not running anymore

895c11d

[ssh] Treat stfp thread exit as failure

74267c2

[ssh] Formatted [[nodiscard]]

8e76021

[sshfs] Use optional for watchdog

e186630

[sshfs] Use state enum instead of running bool

2cc9974

[sshfs] Change watchdog to not use sigtimedwait

055d2fe

[sshfs] Use seperate signaling thread

c5d9200

[libssh] Remove stray include

c9f2ff6

[sshfs] Address PR reviews

92c8746

[sshfs] Address some review comments

a3e240e

[sshfs] Use Timer and wrap posix signals

4d1c44a

[sshfs] Add platform unix tests

3e8b0e7

[sshfs] Yield and move include

ada20ce

Sploder12 force-pushed the update-libssh branch from 5a8e954 to ada20ce Compare December 17, 2024 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update libssh to 0.11.1 #3735

Update libssh to 0.11.1 #3735

Sploder12 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

ricab left a comment

Sploder12 commented Oct 21, 2024

georgeliao left a comment

georgeliao Dec 3, 2024

ricab Dec 3, 2024

Sploder12 Dec 3, 2024

georgeliao Dec 4, 2024 •

edited

Loading

georgeliao Dec 3, 2024

ricab Dec 3, 2024

Sploder12 Dec 3, 2024

georgeliao Dec 3, 2024

georgeliao Dec 4, 2024 •

edited

Loading

Sploder12 Dec 4, 2024

ricab left a comment

ricab Dec 3, 2024

ricab Dec 3, 2024

georgeliao Dec 11, 2024 •

edited

Loading

ricab left a comment

ricab Dec 17, 2024

Sploder12 commented Dec 17, 2024

Update libssh to 0.11.1 #3735

Are you sure you want to change the base?

Update libssh to 0.11.1 #3735

Conversation

Sploder12 commented Oct 16, 2024 • edited Loading

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

ricab left a comment

Choose a reason for hiding this comment

Sploder12 commented Oct 21, 2024

Formatting:

Mounts:

georgeliao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeliao Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

ricab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sploder12 commented Dec 17, 2024

Sploder12 commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

georgeliao Dec 4, 2024 •

edited

Loading

georgeliao Dec 4, 2024 •

edited

Loading

georgeliao Dec 11, 2024 •

edited

Loading