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

Improvements to how runc exec is handled #4592

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,7 @@ func execProcess(context *cli.Context) (int, error) {
if status == libcontainer.Paused && !context.Bool("ignore-paused") {
return -1, errors.New("cannot exec in a paused container (use --ignore-paused to override)")
}
path := context.String("process")
if path == "" && len(context.Args()) == 1 {
return -1, errors.New("process args cannot be empty")
}
state, err := container.State()
if err != nil {
return -1, err
}
bundle, ok := utils.SearchLabels(state.Config.Labels, "bundle")
if !ok {
return -1, errors.New("bundle not found in labels")
}
Comment on lines -165 to -172
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is replaced with

bundle, ok := utils.SearchLabels(c.Config().Labels, "bundle")
if !ok {
	return nil, errors.New("bundle not found in labels")
}

(see below)

p, err := getProcess(context, bundle)
p, err := getProcess(context, container)
if err != nil {
return -1, err
}
Expand All @@ -196,7 +184,7 @@ func execProcess(context *cli.Context) (int, error) {
return r.run(p)
}

func getProcess(context *cli.Context, bundle string) (*specs.Process, error) {
func getProcess(context *cli.Context, c *libcontainer.Container) (*specs.Process, error) {
if path := context.String("process"); path != "" {
f, err := os.Open(path)
if err != nil {
Expand All @@ -209,7 +197,11 @@ func getProcess(context *cli.Context, bundle string) (*specs.Process, error) {
}
return &p, validateProcessSpec(&p)
}
// process via cli flags
// Process from config.json and CLI flags.
bundle, ok := utils.SearchLabels(c.Config().Labels, "bundle")
if !ok {
return nil, errors.New("bundle not found in labels")
}
if err := os.Chdir(bundle); err != nil {
return nil, err
}
Expand All @@ -218,7 +210,11 @@ func getProcess(context *cli.Context, bundle string) (*specs.Process, error) {
return nil, err
}
p := spec.Process
p.Args = context.Args()[1:]
args := context.Args()
if len(args) < 2 {
return nil, errors.New("exec args cannot be empty")
}
p.Args = args[1:]
// Override the cwd, if passed.
if cwd := context.String("cwd"); cwd != "" {
p.Cwd = cwd
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/container_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,11 +432,11 @@ func (c *Container) signal(s os.Signal) error {
}

func (c *Container) createExecFifo() (retErr error) {
rootuid, err := c.Config().HostRootUID()
rootuid, err := c.config.HostRootUID()
if err != nil {
return err
}
rootgid, err := c.Config().HostRootGID()
rootgid, err := c.config.HostRootGID()
if err != nil {
return err
}
Expand Down
30 changes: 16 additions & 14 deletions utils_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,8 @@ func getDefaultImagePath() string {
return filepath.Join(cwd, "checkpoint")
}

// newProcess returns a new libcontainer Process with the arguments from the
// spec and stdio from the current process.
func newProcess(p specs.Process) (*libcontainer.Process, error) {
// newProcess converts [specs.Process] to [libcontainer.Process].
func newProcess(p *specs.Process) (*libcontainer.Process, error) {
Copy link
Contributor Author

@kolyshkin kolyshkin Feb 1, 2025

Choose a reason for hiding this comment

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

Maybe it also makes sense to make this a public method (and move to libcontainer).

Something like libcontainer.ProcessFromSpec maybe

lp := &libcontainer.Process{
Args: p.Args,
Env: p.Env,
Expand Down Expand Up @@ -89,7 +88,7 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) {
}

// setupIO modifies the given process config according to the options.
func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, detach bool, sockpath string) (*tty, error) {
func setupIO(process *libcontainer.Process, container *libcontainer.Container, createTTY, detach bool, sockpath string) (*tty, error) {
if createTTY {
process.Stdin = nil
process.Stdout = nil
Expand Down Expand Up @@ -135,6 +134,17 @@ func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, det
inheritStdio(process)
return &tty{}, nil
}

config := container.Config()
rootuid, err := config.HostRootUID()
if err != nil {
return nil, err
}
rootgid, err := config.HostRootGID()
if err != nil {
return nil, err
}

return setupProcessPipes(process, rootuid, rootgid)
}

Expand Down Expand Up @@ -210,7 +220,7 @@ func (r *runner) run(config *specs.Process) (int, error) {
if err = r.checkTerminal(config); err != nil {
return -1, err
}
process, err := newProcess(*config)
process, err := newProcess(config)
if err != nil {
return -1, err
}
Expand All @@ -232,20 +242,12 @@ func (r *runner) run(config *specs.Process) (int, error) {
}
process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i)))
}
rootuid, err := r.container.Config().HostRootUID()
if err != nil {
return -1, err
}
rootgid, err := r.container.Config().HostRootGID()
if err != nil {
return -1, err
}
detach := r.detach || (r.action == CT_ACT_CREATE)
// Setting up IO is a two stage process. We need to modify process to deal
// with detaching containers, and then we get a tty after the container has
// started.
handler := newSignalHandler(r.enableSubreaper, r.notifySocket)
tty, err := setupIO(process, rootuid, rootgid, config.Terminal, detach, r.consoleSocket)
tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket)
if err != nil {
return -1, err
}
Expand Down