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

Add support for file descriptor passing #1553

Merged

Conversation

mgjm
Copy link
Contributor

@mgjm mgjm commented Aug 1, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds support for file descriptors:

  • New StartFdSocket RPC command
  • additionalFds passed to the OCI runtime
  • leakFds kept open by conmon as long as the container is running

I haven't found a good way to pass file descriptors via SCM_RIGHTS on the existing RPC connection. Therefore an additional socket is created for file descriptor passing. The protocol is documented in conmon-rs/server/src/fd_socket.rs.

RPC level workflow:

  • The client calls StartFdSocket to start the fd socket server (if not already running) and gets the socket path
  • The client send the file descriptors to the server via SCM_RIGHTS and gets slot numbers in return
  • The client needs to keep the fd socket connection open until the fds are used
  • The client calls CreateContainer with additionalFds or leakFds
  • The client closes the fd socket connection (unused fds get closed at this point, the slot numbers are no longer valid)

GO client workflow:

  • The user gets a RemoteFDs instance via the client.RemoteFDs method
  • The user sends file descriptors via the remoteFDs.Send method and gets slot numbers in return
  • The user calls client.CreateContainer with AdditionalFDs or LeakFDs
  • The user calls remoteFDs.Close to close the connection

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

An alternative approach would be to hide the file descriptor logic from the public go client interface and just accept an file descriptor array in CreateContainerConfig and send the file descriptors in the client.CreateContainer method.

This would result in a cleaner API but the current design is simpler and allows reusing the RemoteFDs instance. But we create a new connection for each RPC request anyways, so does that performance improvement even matter?

Does this PR introduce a user-facing change?

Add support for file descriptor passing

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #1553 (bfc3a05) into main (7214897) will increase coverage by 0.13%.
The diff coverage is 14.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
+ Coverage   34.30%   34.44%   +0.13%     
==========================================
  Files          13       13              
  Lines        1134     1144      +10     
  Branches      389      392       +3     
==========================================
+ Hits          389      394       +5     
  Misses        493      493              
- Partials      252      257       +5     

@haircommander
Copy link
Collaborator

a couple of notes, otherwise LGTM

@haircommander
Copy link
Collaborator

@saschagrunert PTAL

conmon-rs/server/src/child_reaper.rs Outdated Show resolved Hide resolved
conmon-rs/common/proto/conmon.capnp Show resolved Hide resolved
conmon-rs/server/src/fd_socket.rs Outdated Show resolved Hide resolved
conmon-rs/server/src/fd_socket.rs Outdated Show resolved Hide resolved
conmon-rs/server/src/fd_socket.rs Outdated Show resolved Hide resolved
conmon-rs/server/src/rpc.rs Show resolved Hide resolved
@saschagrunert
Copy link
Member

We probably should exclude the capnp file from the typos test: https://github.com/containers/conmon-rs/actions/runs/5815034612/job/15765845509?pr=1553

@mgjm
Copy link
Contributor Author

mgjm commented Aug 10, 2023

@saschagrunert Which option to exclude capnp file from the typos test do you prefer? I don't think the typos tool is used outside of CI, therefore I would lean towards option 3.

@saschagrunert
Copy link
Member

@saschagrunert Which option to exclude capnp file from the typos test do you prefer? I don't think the typos tool is used outside of CI, therefore I would lean towards option 3.

Yeah I'm happy with the config file (option 3), too!

@haircommander
Copy link
Collaborator

option 3 works for me!

Signed-off-by: Martin Michaelis <code@mgjm.de>
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

Signed-off-by: Martin Michaelis <code@mgjm.de>
@mgjm
Copy link
Contributor Author

mgjm commented Aug 15, 2023

@saschagrunert the integration test finally succeeds. I needed to increase the MAX_RSS and the max allowed diff in the memory leak test

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

I see, let's get that change in.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 15, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgjm, saschagrunert

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 7d7e415 into containers:main Aug 15, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants