-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Parse grpc requests on the service socket #27395
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
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
1 similar comment
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
e98f387 to
7472926
Compare
|
Added bloat_approved because we pull in |
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.
LGTM, it seems that Swagger isn't happy.
Luap99
left a comment
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.
What is the use case for this? This is bloating podman for "nothing" AFAICT
bin/podman grew by 260056 bytes; max allowed is 51200.
Debloating podman is quite a difficult task so I am very vary to add bloat for stuff that adds no functionality whatsoever.
I don't see why we would need a grpc endpoint at all, is there no way to just close the gprc request directly without all these protobuf requirements?
I am not really familiar with grpc but if that just routes based of the Content-Type header could we not just close the requests with such headers? Or return the corresponding 400/404 return codes or whatever the equivalent in grpc is without pulling in protobuf?
test/system/251-system-service.bats
Outdated
| _podman_system_service $URL --time=0 | ||
| wait_for_file $PODMAN_TMPDIR/myunix.sock | ||
|
|
||
| run grpcurl -unix -plaintext -d '{"ignored":"test"}' $URL io.podman.v1.Noop.Noop |
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.
Where and how does one get this new test dependency.
I don't see how this can pass in the test env without this being present, i.e. tmt tests run of podman-tests rpm so this needs a dep on whatever package this would provide.
As tmt tests passed I took a look and they are broken badly, they seem to test the @main rpm not the PR build
@lsm5 PTAL!
https://artifacts.dev.testing-farm.io/fb24d06d-ef07-4211-87a1-42a42d79bb33
https://artifacts.dev.testing-farm.io/fb24d06d-ef07-4211-87a1-42a42d79bb33/work-remote-rootdx4znvlo/plans/system/remote-root/execute/data/guest/default-0/test/tmt/system/remote-root-1/output.txt
podman-6.0.0~dev-1.20251028195230397831.main.1810.386c8f3fe9.fc43.x86_64
podman-tests-6.0.0~dev-1.20251028195230397831.main.1810.386c8f3fe9.fc43.x86_64
I would really like to avoid new dependencies like that, it seem to be there as package for fedora but I doubt it will be packages on RHEL making gating tests there more difficult
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.
RE: fetching copr rpm for tests, #27488 .
|
If we ever want to implement any GRPC APIs (including those that are expected by compose), this is part of how we'll most likely end up having to do it. The no-op service is mainly there so that we have something to test, I could take or leave that. |
Ack I guess for buildkit that will be inevitable. However for now I don't get what the dump noop service offers, coudln't we just wait until we actually implement a proper API endpoint?
system tests are run as part of gating with no access to the full source tree, they only run with the content from test/system everything else is not shipped in podman-tests rpm. Any I doubt we want to build and bundle our own grpcurl and ship that with podman-tests. These tests inb general must work without access to the Makefile. Also building it every time makes the tests slower. Looking at one test log I see ~50s build time for just this one util so I really like to avoid that. |
|
+600,000 lines makes me scream a little inside; shocking this only adds a quarter of a megabyte to binary size. I guess it's mostly test dependencies, but still... |
Because that is test/tools/vendor, the main go module doesn't seem to import anything new. Though on the topic of new code do we have the CNCF license scanner running somewhere? I guess even if it is test code we need to be sure to only use CNCF approved licenses here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nalind 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 |
e1d44f0 to
313f36b
Compare
Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Add a bare minimum GRPC service to the podman system service socket. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
LGTM |
|
@containers/podman-maintainers PTAL, I think this is ready. |
Honny1
left a comment
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.
LGTM
|
/lgtm |
Parse and dispatch grpc requests on the service socket, allowing clients to cleanly determine that a given service is not implemented instead of timing out after attempting to upgrade their connection to HTTP/2, failing, and perhaps retrying.
Add a "no-op" service that we can call in a system test to exercise this.
Does this PR introduce a user-facing change?