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

Implement automatic port reassignment on Windows #19557

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

n1hility
Copy link
Member

@n1hility n1hility commented Aug 8, 2023

Fixes #19554

We have had reports of strange intermittent/infrequent connectivity problems with podman machine which vanish after reboot. While all OS's have the potential for collision in ports in the dynamic range, Windows is significantly more susceptible to them since it uses a number of them internally for various networking services. Further, they have a non-spec reservation mechanism that system services can use which blocks access to arbitrary port ranges in the dynamic space even when not in use. These registrations do not have standard/fixed addresses, and can be arbitrarily relocated (e.g. after a windows update) so can not be predetermined/avoided.

Since there is only 16K ports in the total range, with potentially random assignments > 1K in use (depending on activity) there could be as good of a 1/16 chance each time podman is started to hit a collision. Further, since as other OSes the kernel prefers to allocate dynamic ports in a sequential ring pattern, prolonged system activity (e.g. using on a laptop where reboots are uncommon), the odds of eventually hitting this are high.

This PR addresses the problem by detecting a conflict and dynamically reassigning the ssh port with no action required from the user. While only leveraged by the WSL backend, this commit also adds core infrastructure for all other backends for future enhancement. To make this easier to manage and review, I plan to propose those changes in separate PRs.

Future PRs I plan to submit:

  1. Enhance containers/common config to have a built-in lock mechanism (PR uses a backend-specific mechanism temporarily) - done - Introduce (global) default config file locking common#1610
  2. Small refactor of this PR to use (1)
  3. Qemu, applehv, and hyperv support for port collisions - these should be fairly lite code changes compared to this change since they both have less to do, and nearly all logic is general/shared.
Automatically reassign podman machine ssh port on Windows when it conflicts with in-use system ports (Fixes #19554)

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 8, 2023
@n1hility n1hility force-pushed the automatic-ports branch 2 times, most recently from ecbbc28 to c09d0d3 Compare August 8, 2023 21:12
pkg/machine/ports_unix.go Outdated Show resolved Hide resolved
@n1hility
Copy link
Member Author

PTAL @Luap99 I addressed some comments but not sure if you had other concerns. Also @baude so you are aware and in case of concerns

Note that there is a discussion @vrothberg and I are having on how the common API should look and the interactions with modules, which will replace an aspect in this PR once right, but if possible I would like to decouple them initially so that the Windows issue is addressed in the meantime. Although of course only if no concerns specific to what this PR introduces.

@TomSweeneyRedHat
Copy link
Member

LGTM
but definitely want a @Luap99 head nod

pkg/machine/wsl/machine.go Outdated Show resolved Hide resolved
@n1hility
Copy link
Member Author

PTAL when you get a chance @Luap99 This has been updated to address your last comments (thanks for those!)

/cc @containers/podman-maintainers

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2023

@n1hility: GitHub didn't allow me to request PR reviews from the following users: containers/podman-maintainers.

Note that only containers members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

PTAL when you get a chance @Luap99 This has been updated to address your last comments (thanks for those!)

/cc @containers/podman-maintainers

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/machine/wsl/machine.go Outdated Show resolved Hide resolved
While only leveraged by the WSL backend, this commit also adds core
infrastructure for all other backends for future enhancement.

- Adds a common port cross backend allocation registry to prevent duplicate
  assignment across multiple machine instances
- Introduces logic in Start() that detects OS port conflicts and scans for a
  viable replacement port
- Updates connection definitions and server configuration accordingly
- Utilizes a coordinated file lock strategy to prevent racing overwrites of port
  and connection registries
- WSL backend coordinates locking for containers.conf until a future common
  enhancement exists to replace it

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
@n1hility
Copy link
Member Author

PTAL @Luap99

@baude
Copy link
Member

baude commented Aug 22, 2023

lgtm

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, n1hility

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit a9c9877 into containers:main Aug 23, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No connection could be made because the target machine actively refused it
5 participants