-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Expose Podman named pipe in Inspect output #17303
Conversation
Added "[NO NEW TESTS NEEDED]" because I could not figure out any tests for Also, I'm still to test this locally end-to-end with Podman Desktop. |
It doesn't look like |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restarted the flaked job
pkg/machine/machine_windows.go
Outdated
@@ -18,3 +20,24 @@ func GetProcessState(pid int) (active bool, exitCode int) { | |||
syscall.GetExitCodeProcess(handle, &code) | |||
return code == 259, int(code) | |||
} | |||
|
|||
func PipeAvailable(pipeName string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function really doing what it's supposed to?
It returns true
if the path does not exist (and false
if it exists) but the name/callers suggest differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept original name. There are 2 different semantics here. Pipe available for bind vs Pipe available for connection. May be it is time to improve the naming if this one is promoted from private to shared. I will take a look later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes, that would help me brain parse the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to PipeNameAvailable
. Was thinking about PipeAddress
, because it is more common (at least from my experience) for everything network/connection like, but MS uses Name it their docs https://learn.microsoft.com/en-us/windows/win32/ipc/pipe-names
PipeNameAvailable
doesn't look like a masterpiece of naming, but it at least makes it clear what is actually checked by the function.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arixmkii, vrothberg 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 |
Signed-off-by: Arthur Sengileyev <arthur.sengileyev@gmail.com>
According to line numbers in error output the failure is inside From logs:
This doesn't look like something expected. So, if not a fluke, then potentially something with the infra. |
Just noticed that previous failure was "int podman fedora-36 root host Failing after 4m", but now it is "int podman fedora-36 root container Failing after 6m ", so, this are different failures using some similar versions. Then probably they are not related to changes in this PR after all. |
This is a very stubborn and frequent flake at the moment, sorry for that. I restarted the failed jobs. |
/lgtm |
Fixes #16860
PodmanPipe
structure added toConnectionConfig
. Named pipe address is not full equivalent of a file, but 1) it can be represented as a UNC path; 2) internally in code it used withos.Stat
API similar to files; 3) it is logically to add it with the same shape asPodmanSocket
; 4) it is OS specific, butPodmanSocket
in the moment of introduction was also OS specific and was present in Windows builds, where it was not used before, I considered that this sort of overhead is acceptable, so, it is modeled with VMFile and unconditionally added.Additional refactoring was applied to promote some utility functions for named pipe to the machine common part instead of WSL:
PipeAvailable
andWaitPipeExists
. There 100% will be need to use former in QEMU machine, but the latter was just logical to keep next to the former.Important note - why it is not possible to use
PodmanSocket
for this purpose. First of all there is no way to communicate transport type with VMFile structure and this would put additional complexity on clients to understand how to use this value. Another reason is that to me it looks desirable to keepPodmanSocket
reserved for QEMU machine and unix domain socket to match QEMU on other hosts, but it will still need to expose named pipe, because NodeJS clients (most notable Podman Desktop) can't use unix domain sockets on Windows (because of the lack of support in libuv).Signed-off-by: Arthur Sengileyev arthur.sengileyev@gmail.com
[NO NEW TESTS NEEDED]
Does this PR introduce a user-facing change?