Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move and rename SSH master socket path to avoid path length limit #3196

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

happz
Copy link
Collaborator

@happz happz commented Sep 5, 2024

Putting SSH sockets into plan directory is nice and all, no chance to
re-use socket by mistake, no conflicts, but it turns out SSH will not
accept path longer than 104 characters. Or 108, depending who you ask.

So, moving sockets once again, this time then live in run workdir,
they no longer contain plan name, because that can be very long, and we
have two fallbacks:

  • if the path is too long, we use hashlib and create less readable,
    but hopefully shorter name, and
  • if the path is still too long, SSH multiplexing is disabled
    completely.

Pull Request Checklist

  • implement the feature
  • extend the test coverage

@happz happz added the bug Something isn't working label Sep 5, 2024
@happz happz added this to the 1.37 milestone Sep 5, 2024
@happz happz force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from b2dfe25 to 177d5ec Compare September 5, 2024 20:52
@happz happz added step | provision Stuff related to the provision step ci | full test Pull request is ready for the full test execution labels Sep 5, 2024
@happz
Copy link
Collaborator Author

happz commented Sep 5, 2024

/packit test

@happz happz changed the title Our regular monthly move of ssh master sockets Move and rename SSH master socket path to avoid path length limit Sep 6, 2024
Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if there is a ssh problem with ssh-socket path again with this, there really is something cursed.

Copy link
Collaborator

@seberm seberm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is going to be a bugfix release tmt-1.36.1, you should probably describe the changes in the release notes (releases.rst).

@happz
Copy link
Collaborator Author

happz commented Sep 6, 2024

If this is going to be a bugfix release tmt-1.36.1, you should probably describe the changes in the release notes (releases.rst).

Hmm, I think it does deserve a release note, but for 1.37 - I would expect release notes for 1.36.1 to be constructed in commit(s) creating 1.36.1 out of 1.36.0 tag, I suppose some cherry-picking of needed bugfixes + a commit with release stuff.

Edit: added in 4653adc

@happz happz force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from 7bf1e1c to 4653adc Compare September 9, 2024 09:25
@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Sep 10, 2024
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, just noticed one test nitpick. However, I'm not sure that the master ssh connection actuallly works. Tested with a simple tmt init -t mini && tmt run and it seems that it takes about one second to execute each ssh command:

15:14:38   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... cat /etc/os-release'
15:14:39   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... cat /etc/lsb-release'
15:14:39   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... arch'
15:14:40   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... uname -r'
15:14:42   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... apk --version'
15:14:43   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... apt --version'
15:14:44   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... /bin/bash -c '"'"'
15:14:45   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... dnf5 --version'
15:14:46   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... /bin/bash -c '"'"'
15:14:47   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... stat /run/ostree-booted'
15:14:48   Run command: ssh ... -S/var/tmp/tmt/run-017/ssh-sockets/127.0.0.1-10022-root.socket ... cat /proc/filesystems'

Shouldn't these simple commands run much faster? What would be an easy check to ensure that the master connection was actually used? Anyway, I have a feeling that the problem is there for some time already (not caused by this patch). But I'd say it would be good to get it fixed anyway.

@psss
Copy link
Collaborator

psss commented Sep 10, 2024

So the problem was caused by trying to spawn the master ssh connection before the guest was ready. We need to check that guest is up before creating the master socket. To be addressed in a separate pull request.

@happz happz force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from 4653adc to ecfef1e Compare September 10, 2024 18:30
@happz happz force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from ecfef1e to c4dcb38 Compare September 11, 2024 08:36
@psss psss force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from c4dcb38 to 6c809ca Compare September 11, 2024 11:44
@psss
Copy link
Collaborator

psss commented Sep 11, 2024

Forgot to mention: @happz, I really love your special branch names, like this one ;-)

@happz
Copy link
Collaborator Author

happz commented Sep 12, 2024

/packit test

Putting SSH sockets into plan directory is nice and all, no chance to
re-use socket by mistake, no conflicts, but it turns out SSH will not
accept path longer than 104 characters. Or 108, depending who you ask.

So, moving sockets once again, this time then live in *run* workdir,
they no longer contain plan name, because that can be very long, and we
have two fallbacks:

* if the path is too long, we use `hashlib` and create less readable,
  but hopefully shorter name, and
* if the path is still too long, SSH multiplexing is disabled
  completely.
@happz happz force-pushed the our-regular-monthly-move-of-ssh-master-sockets branch from 6c809ca to 5681f20 Compare September 12, 2024 07:07
@happz
Copy link
Collaborator Author

happz commented Sep 12, 2024

/packit test

1 similar comment
@happz
Copy link
Collaborator Author

happz commented Sep 12, 2024

/packit test

@psss
Copy link
Collaborator

psss commented Sep 12, 2024

Failing test is an irrelevant dhclient avc error, everythin else green, merging.

@psss psss enabled auto-merge (squash) September 12, 2024 10:26
@psss psss disabled auto-merge September 12, 2024 10:26
@psss psss merged commit 1866630 into main Sep 12, 2024
21 of 22 checks passed
@psss psss deleted the our-regular-monthly-move-of-ssh-master-sockets branch September 12, 2024 10:27
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
…emtee#3196)

Putting SSH sockets into plan directory is nice and all, no chance to
re-use socket by mistake, no conflicts, but it turns out SSH will not
accept path longer than 104 characters. Or 108, depending who you ask.

So, moving sockets once again, this time then live in *run* workdir,
they no longer contain plan name, because that can be very long, and we
have two fallbacks:

* if the path is too long, we use `hashlib` and create less readable,
  but hopefully shorter name, and
* if the path is still too long, SSH multiplexing is disabled
  completely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci | full test Pull request is ready for the full test execution status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. step | provision Stuff related to the provision step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants