Move the HostConfig portion of Inspect inside libpod #3522
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
|
Still to do: There are a few things that I need to add as spec annotations so we can still retrieve them. I also need to remove the calls to create the artifact. |
|
Alright, this one's shaping up nicely. I need to hit a few TODOs tomorrow, but it's basically ready for review. |
|
Downsides of this:
|
|
Build fails on windows... I'll fix it tomorrow. |
libpod/container_inspect.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, cap := range g.Config.Process.Capabilities.Bounding { |
There was a problem hiding this comment.
@rhatdan PTAL at this section. I'm thinking Bounding is the safest set of capabilities to compute against, but I could be wrong.
pkg/util/utils.go
Outdated
| // We are a device node. Get major/minor. | ||
| sysstat, ok := info.Sys().(*syscall.Stat_t) | ||
| if !ok { | ||
| return errors.Errorf("Could not convert stat output for use") |
There was a problem hiding this comment.
Need a better error message, this means nothing to me.
libpod/container_inspect.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, cap := range g.Config.Process.Capabilities.Bounding { |
|
Overall LGTM |
|
Everything except |
|
The last thing I wanted to check was blkio-weight-device, but it seems like Fedora kernels don't even support that... I think we're good here. |
|
Oh, wait, one more thing to do - need to retrieve the side of the TTY, if one is created. |
|
Note to self: Conmon PID inside of inspect is wrong. Conmon double-forks to daemonize, so we can't use the PID of our direct child. |
|
Conclusion on TTY size: I don't think it's possible without Conmon changes. Should be good to merge without it - the old version of Inspect didn't include it either. |
cmd/podman/inspect.go
Outdated
| @@ -188,21 +176,11 @@ func iterateInput(ctx context.Context, size bool, args []string, runtime *adapte | |||
| break | |||
| } | |||
| } else { | |||
| libpodInspectData, err := ctr.Inspect(size) | |||
| data, err = ctr.Inspect(size) | |||
| if err != nil { | |||
| inspectError = errors.Wrapf(err, "error getting libpod container inspect data %s", ctr.ID()) | |||
There was a problem hiding this comment.
we prob should drop the libpod bits here ... container should suffice?
|
have you compared before and after performance to make sure we didnt regress? LGTM |
|
It seemed identical, but my system is fast enough that both finish in well under a second. We could get better numbers on something slower, I think. |
|
☔ The latest upstream changes (presumably #3521) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased |
2e180e2 to
922c747
Compare
|
@vrothberg @giuseppe @rhatdan @baude Any chance I can get a held LGTM here? Need this landed to do some further work on inspect |
| @@ -461,8 +462,21 @@ func (r *OCIRuntime) createOCIContainer(ctr *Container, cgroupParent string, res | |||
| return errors.Wrapf(define.ErrInternal, "container create failed") | |||
| } | |||
| ctr.state.PID = ss.si.Pid | |||
| if cmd.Process != nil { | |||
| ctr.state.ConmonPID = cmd.Process.Pid | |||
There was a problem hiding this comment.
do we still need cmd.Process.Pid when ctr.config.ConmonPidFile == ""?
There was a problem hiding this comment.
We didn't get a correct PID in this case (it was always off because of the double-fork). We won't be able to get a PID for old containers without ConmonPidFile set, but no PID is better than reporting an incorrect PID.
| // This is very expensive to initialize. | ||
| // So we don't want to initialize it unless we absolutely have to - IE, | ||
| // there are things that require a major:minor to path translation. | ||
| var deviceNodes map[string]string |
There was a problem hiding this comment.
Nit of a nit: sync.Once can help hiding such details in the implementation.
There was a problem hiding this comment.
That actually breaks error propagation - Do can't return errors that occur during the function. We can wire up manual error handling with closures, but at that point we're at roughly the same line count.
|
Pushed a fresh version adding --security-opt in. We're now basically completely compatible except for terminal size, which is... difficult, and requires conmon changes. And probably isn't particularly important. |
When we first began writing Podman, we ran into a major issue when implementing Inspect. Libpod deliberately does not tie its internal data structures to Docker, and stores most information about containers encoded within the OCI spec. However, Podman must present a CLI compatible with Docker, which means it must expose all the information in 'docker inspect' - most of which is not contained in the OCI spec or libpod's Config struct. Our solution at the time was the create artifact. We JSON'd the complete CreateConfig (a parsed form of the CLI arguments to 'podman run') and stored it with the container, restoring it when we needed to run commands that required the extra info. Over the past month, I've been looking more at Inspect, and refactored large portions of it into Libpod - generating them from what we know about the OCI config and libpod's (now much expanded, versus previously) container configuration. This path comes close to completing the process, moving the last part of inspect into libpod and removing the need for the create artifact. This improves libpod's compatability with non-Podman containers. We no longer require an arbitrarily-formatted JSON blob to be present to run inspect. Fixes: containers#3500 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
Our previous method (just read the PID that we spawned) doesn't work - Conmon double-forks to daemonize, so we end up with a PID pointing to the first process, which dies almost immediately. Reading from the PID file gets us the real PID. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
We can infer no-new-privileges. For now, manually populate seccomp (can't infer what file we sourced from) and SELinux/Apparmor (hard to tell if they're enabled or not). Signed-off-by: Matthew Heon <mheon@redhat.com>
|
If anyone wants to merge once it's green, would be appreciated. |
|
/retest |
|
/test images |
|
/lgtm |
When we first began writing Podman, we ran into a major issue when implementing Inspect. Libpod deliberately does not tie its internal data structures to Docker, and stores most information about containers encoded within the OCI spec. However, Podman must present a CLI compatible with Docker, which means it must expose all the information in 'docker inspect' - most of which is not contained in the OCI spec or libpod's Config struct.
Our solution at the time was the create artifact. We JSON'd the complete CreateConfig (a parsed form of the CLI arguments to 'podman run') and stored it with the container, restoring it when we needed to run commands that required the extra info.
Over the past month, I've been looking more at Inspect, and refactored large portions of it into Libpod - generating them from what we know about the OCI config and libpod's (now much expanded, versus previously) container configuration. This path comes close to completing the process, moving the last part of inspect into libpod and removing the need for the create artifact.
This improves libpod's compatability with non-Podman containers. We no longer require an arbitrarily-formatted JSON blob to be present to run inspect.
Fixes: #3500