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.

Sign up for GitHub

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

Switch podman stop/kill/wait handlers to use abi #9051

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Switch podman stop/kill/wait handlers to use abi #9051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 13 additions & 2 deletions cmd/podman/containers/kill.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ package containers

import (
"context"
"errors"
"fmt"
"io/ioutil"
"strings"

"github.com/containers/common/pkg/completion"
"github.com/containers/podman/v2/cmd/podman/common"
Expand All @@ -12,6 +13,7 @@ import (
"github.com/containers/podman/v2/cmd/podman/validate"
"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/containers/podman/v2/pkg/signal"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -59,7 +61,7 @@ func killFlags(cmd *cobra.Command) {
flags.StringVarP(&killOptions.Signal, signalFlagName, "s", "KILL", "Signal to send to the container")
_ = cmd.RegisterFlagCompletionFunc(signalFlagName, common.AutocompleteStopSignal)
cidfileFlagName := "cidfile"
flags.StringArrayVar(&killOptions.CIDFiles, cidfileFlagName, []string{}, "Read the container ID from the file")
flags.StringArrayVar(&cidFiles, cidfileFlagName, []string{}, "Read the container ID from the file")
_ = cmd.RegisterFlagCompletionFunc(cidfileFlagName, completion.AutocompleteDefault)
}

Expand Down Expand Up @@ -94,6 +96,15 @@ func kill(_ *cobra.Command, args []string) error {
if sig < 1 || sig > 64 {
return errors.New("valid signals are 1 through 64")
}
for _, cidFile := range cidFiles {
content, err := ioutil.ReadFile(string(cidFile))
if err != nil {
return errors.Wrap(err, "error reading CIDFile")
}
id := strings.Split(string(content), "\n")[0]
args = append(args, id)
}

responses, err := registry.ContainerEngine().ContainerKill(context.Background(), args, killOptions)
if err != nil {
return err
Expand Down
14 changes: 13 additions & 1 deletion cmd/podman/containers/stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package containers
import (
"context"
"fmt"
"io/ioutil"
"strings"

"github.com/containers/common/pkg/completion"
"github.com/containers/podman/v2/cmd/podman/common"
"github.com/containers/podman/v2/cmd/podman/registry"
"github.com/containers/podman/v2/cmd/podman/utils"
"github.com/containers/podman/v2/cmd/podman/validate"
"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/pkg/errors"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -58,7 +61,7 @@ func stopFlags(cmd *cobra.Command) {
flags.BoolVarP(&stopOptions.Ignore, "ignore", "i", false, "Ignore errors when a specified container is missing")

cidfileFlagName := "cidfile"
flags.StringArrayVarP(&stopOptions.CIDFiles, cidfileFlagName, "", nil, "Read the container ID from the file")
flags.StringArrayVar(&cidFiles, cidfileFlagName, nil, "Read the container ID from the file")
_ = cmd.RegisterFlagCompletionFunc(cidfileFlagName, completion.AutocompleteDefault)

timeFlagName := "time"
Expand Down Expand Up @@ -97,6 +100,15 @@ func stop(cmd *cobra.Command, args []string) error {
stopOptions.Timeout = &stopTimeout
}

for _, cidFile := range cidFiles {
content, err := ioutil.ReadFile(string(cidFile))
if err != nil {
return errors.Wrap(err, "error reading CIDFile")
}
id := strings.Split(string(content), "\n")[0]
args = append(args, id)
}

responses, err := registry.ContainerEngine().ContainerStop(context.Background(), args, stopOptions)
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion cmd/podman/containers/wait.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func waitFlags(cmd *cobra.Command) {
flags := cmd.Flags()

intervalFlagName := "interval"
flags.StringVarP(&waitInterval, intervalFlagName, "i", "250ns", "Time Interval to wait before polling for completion")
flags.StringVarP(&waitInterval, intervalFlagName, "i", "250ms", "Time Interval to wait before polling for completion")
_ = cmd.RegisterFlagCompletionFunc(intervalFlagName, completion.AutocompleteNone)

conditionFlagName := "condition"
Expand Down
92 changes: 51 additions & 41 deletions pkg/api/handlers/compat/containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ import (
func RemoveContainer(w http.ResponseWriter, r *http.Request) {
decoder := r.Context().Value("decoder").(*schema.Decoder)
query := struct {
All bool `schema:"all"`
Force bool `schema:"force"`
Ignore bool `schema:"ignore"`
Link bool `schema:"link"`
Volumes bool `schema:"v"`
Force bool `schema:"force"`
Ignore bool `schema:"ignore"`
Link bool `schema:"link"`
DockerVolumes bool `schema:"v"`
LibpodVolumes bool `schema:"volumes"`
}{
// override any golang type defaults
}
Expand All @@ -46,23 +46,26 @@ func RemoveContainer(w http.ResponseWriter, r *http.Request) {
return
}

if query.Link && !utils.IsLibpodRequest(r) {
utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
utils.ErrLinkNotSupport)
return
options := entities.RmOptions{
Force: query.Force,
Ignore: query.Ignore,
}
if utils.IsLibpodRequest(r) {
options.Volumes = query.LibpodVolumes
} else {
if query.Link {
utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
utils.ErrLinkNotSupport)
return
}
options.Volumes = query.DockerVolumes
}

runtime := r.Context().Value("runtime").(*libpod.Runtime)
// Now use the ABI implementation to prevent us from having duplicate
// code.
containerEngine := abi.ContainerEngine{Libpod: runtime}
name := utils.GetName(r)
options := entities.RmOptions{
All: query.All,
Force: query.Force,
Volumes: query.Volumes,
Ignore: query.Ignore,
}
report, err := containerEngine.ContainerRm(r.Context(), []string{name}, options)
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
Expand Down Expand Up @@ -193,45 +196,48 @@ func KillContainer(w http.ResponseWriter, r *http.Request) {
return
}

sig, err := signal.ParseSignalNameOrNumber(query.Signal)
if err != nil {
utils.InternalServerError(w, err)
return
}
// Now use the ABI implementation to prevent us from having duplicate
// code.
containerEngine := abi.ContainerEngine{Libpod: runtime}
name := utils.GetName(r)
con, err := runtime.LookupContainer(name)
if err != nil {
utils.ContainerNotFound(w, name, err)
return
options := entities.KillOptions{
Signal: query.Signal,
}

state, err := con.State()
report, err := containerEngine.ContainerKill(r.Context(), []string{name}, options)
if err != nil {
utils.InternalServerError(w, err)
return
}
if errors.Cause(err) == define.ErrCtrStateInvalid ||
errors.Cause(err) == define.ErrCtrStopped {
utils.Error(w, fmt.Sprintf("Container %s is not running", name), http.StatusConflict, err)
return
}
if errors.Cause(err) == define.ErrNoSuchCtr {
utils.ContainerNotFound(w, name, err)
return
}

// If the Container is stopped already, send a 409
if state == define.ContainerStateStopped || state == define.ContainerStateExited {
utils.Error(w, fmt.Sprintf("Container %s is not running", name), http.StatusConflict, errors.New(fmt.Sprintf("Cannot kill Container %s, it is not running", name)))
Copy link
Member

Choose a reason for hiding this comment

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

We need to retain some form of this check so we can return StatusConflict

Copy link
Member

Choose a reason for hiding this comment

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

I think you can check if ContainerKill returned either ErrCtrStateInvalid or ErrCtrStopped though

utils.InternalServerError(w, err)
return
}

signal := uint(sig)

err = con.Kill(signal)
if err != nil {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrapf(err, "unable to kill Container %s", name))
if len(report) > 0 && report[0].Err != nil {
utils.InternalServerError(w, report[0].Err)
return
}

// Docker waits for the container to stop if the signal is 0 or
// SIGKILL.
if !utils.IsLibpodRequest(r) && (signal == 0 || syscall.Signal(signal) == syscall.SIGKILL) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to retain this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

if _, err = con.Wait(); err != nil {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrapf(err, "failed to wait for Container %s", con.ID()))
if !utils.IsLibpodRequest(r) {
sig, err := signal.ParseSignalNameOrNumber(query.Signal)
if err != nil {
utils.InternalServerError(w, err)
return
}
if sig == 0 || syscall.Signal(sig) == syscall.SIGKILL {
var opts entities.WaitOptions
if _, err := containerEngine.ContainerWait(r.Context(), []string{name}, opts); err != nil {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, err)
return
}
}
}
// Success
utils.WriteResponse(w, http.StatusNoContent, nil)
Expand All @@ -242,6 +248,10 @@ func WaitContainer(w http.ResponseWriter, r *http.Request) {
// /{version}/containers/(name)/wait
exitCode, err := utils.WaitContainer(w, r)
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Waiting on a container that has been removed by --rm will error now

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think we should at least log it. Or just ignore it.

logrus.Warnf("container not found %q: %v", utils.GetName(r), err)
return
}
logrus.Warnf("failed to wait on container %q: %v", mux.Vars(r)["name"], err)
return
}
Expand Down
42 changes: 30 additions & 12 deletions pkg/api/handlers/compat/containers_restart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,42 +4,60 @@ import (
"net/http"

"github.com/containers/podman/v2/libpod"
"github.com/containers/podman/v2/libpod/define"
"github.com/containers/podman/v2/pkg/api/handlers/utils"
"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/containers/podman/v2/pkg/domain/infra/abi"
"github.com/gorilla/schema"
"github.com/pkg/errors"
)

func RestartContainer(w http.ResponseWriter, r *http.Request) {
runtime := r.Context().Value("runtime").(*libpod.Runtime)
decoder := r.Context().Value("decoder").(*schema.Decoder)
// Now use the ABI implementation to prevent us from having duplicate
// code.
containerEngine := abi.ContainerEngine{Libpod: runtime}

// /{version}/containers/(name)/restart
query := struct {
Timeout int `schema:"t"`
All bool `schema:"all"`
DockerTimeout uint `schema:"t"`
LibpodTimeout uint `schema:"timeout"`
}{
// Override golang default values for types
// override any golang type defaults
}
if err := decoder.Decode(&query, r.URL.Query()); err != nil {
utils.BadRequest(w, "url", r.URL.String(), errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
return
}

name := utils.GetName(r)
con, err := runtime.LookupContainer(name)
if err != nil {
utils.ContainerNotFound(w, name, err)
return
}

timeout := con.StopTimeout()
if _, found := r.URL.Query()["t"]; found {
timeout = uint(query.Timeout)
options := entities.RestartOptions{
All: query.All,
Copy link
Member

Choose a reason for hiding this comment

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

All is still hazardous to use, we're going to lose every error except the first one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope to fix this in the future. But it will be a breaking change in the API.

Timeout: &query.DockerTimeout,
}
if utils.IsLibpodRequest(r) {
options.Timeout = &query.LibpodTimeout
}
report, err := containerEngine.ContainerRestart(r.Context(), []string{name}, options)
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
utils.ContainerNotFound(w, name, err)
return
}

if err := con.RestartWithTimeout(r.Context(), timeout); err != nil {
utils.InternalServerError(w, err)
return
}

if len(report) > 0 && report[0].Err != nil {
utils.InternalServerError(w, report[0].Err)
return
}

// Success
utils.WriteResponse(w, http.StatusNoContent, nil)
}
44 changes: 33 additions & 11 deletions pkg/api/handlers/compat/containers_stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,24 @@ import (
"github.com/containers/podman/v2/libpod"
"github.com/containers/podman/v2/libpod/define"
"github.com/containers/podman/v2/pkg/api/handlers/utils"
"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/containers/podman/v2/pkg/domain/infra/abi"
"github.com/gorilla/schema"
"github.com/pkg/errors"
)

func StopContainer(w http.ResponseWriter, r *http.Request) {
runtime := r.Context().Value("runtime").(*libpod.Runtime)
decoder := r.Context().Value("decoder").(*schema.Decoder)
// Now use the ABI implementation to prevent us from having duplicate
// code.
containerEngine := abi.ContainerEngine{Libpod: runtime}

// /{version}/containers/(name)/stop
query := struct {
Timeout int `schema:"t"`
Ignore bool `schema:"ignore"`
DockerTimeout uint `schema:"t"`
LibpodTimeout uint `schema:"timeout"`
}{
// override any golang type defaults
}
Expand All @@ -27,31 +34,46 @@ func StopContainer(w http.ResponseWriter, r *http.Request) {
}

name := utils.GetName(r)

options := entities.StopOptions{
Ignore: query.Ignore,
}
if utils.IsLibpodRequest(r) {
if query.LibpodTimeout > 0 {
options.Timeout = &query.LibpodTimeout
}
} else {
if query.DockerTimeout > 0 {
options.Timeout = &query.DockerTimeout
}
}
con, err := runtime.LookupContainer(name)
if err != nil {
utils.ContainerNotFound(w, name, err)
return
}

state, err := con.State()
if err != nil {
utils.InternalServerError(w, errors.Wrapf(err, "unable to get state for Container %s", name))
utils.InternalServerError(w, err)
return
}
// If the Container is stopped already, send a 304
if state == define.ContainerStateStopped || state == define.ContainerStateExited {
Copy link
Member

Choose a reason for hiding this comment

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

Need to retain this check so we can return StatusNotModified

Copy link
Member Author

Choose a reason for hiding this comment

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

Yuck, Fixed.

utils.WriteResponse(w, http.StatusNotModified, nil)
return
}
report, err := containerEngine.ContainerStop(r.Context(), []string{name}, options)
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
utils.ContainerNotFound(w, name, err)
return
}

var stopError error
if query.Timeout > 0 {
stopError = con.StopWithTimeout(uint(query.Timeout))
} else {
stopError = con.Stop()
utils.InternalServerError(w, err)
return
}
if stopError != nil {
utils.InternalServerError(w, errors.Wrapf(stopError, "failed to stop %s", name))

if len(report) > 0 && report[0].Err != nil {
utils.InternalServerError(w, report[0].Err)
return
}

Expand Down
Loading