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

bugfix: removing image by ID should conflict with running container. #2926

Closed
wants to merge 2 commits into from
Closed
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
13 changes: 11 additions & 2 deletions apis/server/image_bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,17 @@ func (s *Server) removeImage(ctx context.Context, rw http.ResponseWriter, req *h
}(time.Now())

isForce := httputils.BoolValue(req, "force")
// We only should check the image whether used by container when there is only one primary reference.
if len(refs) == 1 {

isImageIDPrefix := func(imageID string, name string) bool {
if strings.HasPrefix(imageID, name) || strings.HasPrefix(digest.Digest(imageID).Hex(), name) {
return true
}
return false
}

// We should check the image whether used by container when there is only one primary reference
// or the image is removed by image ID.
if len(refs) == 1 || isImageIDPrefix(image.ID, name) {
containers, err := s.ContainerMgr.List(ctx, &mgr.ContainerListOption{
All: true,
FilterFunc: func(c *mgr.Container) bool {
Expand Down
2 changes: 1 addition & 1 deletion daemon/mgr/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -1351,7 +1351,7 @@ func (mgr *ContainerManager) updateContainerDiskQuota(ctx context.Context, c *Co
}

// set mount point disk quota
if err = mgr.setDiskQuota(ctx, c, false); err != nil {
if err = mgr.setDiskQuota(ctx, c, false, true); err != nil {
return errors.Wrapf(err, "failed to set mount point disk quota")
}

Expand Down
6 changes: 3 additions & 3 deletions daemon/mgr/container_storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ func checkDupQuotaMap(qms []*quota.QMap, qm *quota.QMap) *quota.QMap {
return nil
}

func (mgr *ContainerManager) setDiskQuota(ctx context.Context, c *Container, mounted bool) error {
func (mgr *ContainerManager) setDiskQuota(ctx context.Context, c *Container, mounted bool, update bool) error {
var (
err error
globalQuotaID uint32
Expand Down Expand Up @@ -629,7 +629,7 @@ func (mgr *ContainerManager) setDiskQuota(ctx context.Context, c *Container, mou
for _, qm := range qms {
if qm.Destination == "/" {
// set rootfs quota
_, err = quota.SetRootfsDiskQuota(qm.Source, qm.Size, qm.QuotaID)
_, err = quota.SetRootfsDiskQuota(qm.Source, qm.Size, qm.QuotaID, update)
if err != nil {
logrus.Warnf("failed to set rootfs quota, mountfs(%s), size(%s), quota id(%d), err(%v)",
qm.Source, qm.Size, qm.QuotaID, err)
Expand Down Expand Up @@ -763,7 +763,7 @@ func (mgr *ContainerManager) initContainerStorage(ctx context.Context, c *Contai
}

// set mount point disk quota
if err = mgr.setDiskQuota(ctx, c, true); err != nil {
if err = mgr.setDiskQuota(ctx, c, true, false); err != nil {
// just ignore failed to set disk quota
logrus.Warnf("failed to set disk quota, err(%v)", err)
}
Expand Down
6 changes: 4 additions & 2 deletions storage/quota/quota.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func GetQuotaID(dir string) (uint32, error) {
}

// SetRootfsDiskQuota is to set container rootfs dir disk quota.
func SetRootfsDiskQuota(basefs, size string, quotaID uint32) (uint32, error) {
func SetRootfsDiskQuota(basefs, size string, quotaID uint32, update bool) (uint32, error) {
overlayMountInfo, err := getOverlayMountInfo(basefs)
if err != nil {
return 0, errors.Wrapf(err, "failed to get overlay(%s) mount info", basefs)
Expand All @@ -211,7 +211,9 @@ func SetRootfsDiskQuota(basefs, size string, quotaID uint32) (uint32, error) {
return 0, errors.Wrapf(err, "failed to set dir(%s) disk quota", dir)
}

if err := SetQuotaForDir(dir, quotaID); err != nil {
if update {
go SetQuotaForDir(dir, quotaID)
} else if err := SetQuotaForDir(dir, quotaID); err != nil {
return 0, errors.Wrapf(err, "failed to set dir(%s) quota recursively", dir)
}
}
Expand Down
51 changes: 50 additions & 1 deletion test/cli_rmi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,31 @@ func (suite *PouchRmiSuite) TestRmiWorks(c *check.C) {
}
}

// TestRmiWithRunningContainer tests "pouch rmi" won't work without force
func (suite *PouchRmiSuite) TestRmiWithRunningContainer(c *check.C) {
name := "TestRmiWithRunningContainer"
command.PouchRun("pull", busyboxImage).Assert(c, icmd.Success)
command.PouchRun("run", "-d", "--name", name, busyboxImage, "top").Assert(c, icmd.Success)
defer DelContainerForceMultyTime(c, name)

// Can't remove without "--force"
command.PouchRun("rmi", busyboxImage)
res := command.PouchRun("images").Assert(c, icmd.Success)
if out := res.Combined(); !strings.Contains(out, busyboxImage) {
c.Fatalf("unexpected output %s: shouldn't rm image %s without force\n", out, busyboxImage)
}

command.PouchRun("rmi", "-f", busyboxImage).Assert(c, icmd.Success)
res = command.PouchRun("images").Assert(c, icmd.Success)
if out := res.Combined(); strings.Contains(out, busyboxImage) {
c.Fatalf("unexpected output %s: should rm image %s\n", out, busyboxImage)
}
}

// TestRmiForce tests "pouch rmi -f" work
func (suite *PouchRmiSuite) TestRmiForce(c *check.C) {
command.PouchRun("pull", helloworldImage).Assert(c, icmd.Success)

// TODO: rmi -f after create/start containers.
command.PouchRun("rmi", "-f", helloworldImage).Assert(c, icmd.Success)

res := command.PouchRun("images").Assert(c, icmd.Success)
Expand Down Expand Up @@ -102,6 +122,35 @@ func (suite *PouchRmiSuite) TestRmiByImageIDWithTwoPrimaryReferences(c *check.C)
}
}

// TestRmiByImageIDWithPrimaryReferencesAndRunningContainers tests "pouch rmi {ID}" won't work without force.
func (suite *PouchRmiSuite) TestRmiByImageIDWithPrimaryReferencesAndRunningContainers(c *check.C) {
busyboxImageAlias := busyboxImage + "Alias"
name := "TestRmiByImageIDWithRunningContainer"

command.PouchRun("pull", busyboxImage).Assert(c, icmd.Success)
command.PouchRun("tag", busyboxImage, busyboxImageAlias).Assert(c, icmd.Success)
res := command.PouchRun("images")
res.Assert(c, icmd.Success)
imageID := imagesListToKV(res.Combined())[busyboxImage][0]

command.PouchRun("run", "-d", "--name", name, busyboxImage, "top").Assert(c, icmd.Success)
defer DelContainerForceMultyTime(c, name)

// Can't remove without "--force"
res = command.PouchRun("rmi", imageID)
c.Assert(res.Stderr(), check.NotNil, check.Commentf("Unable to remove the image"))
res = command.PouchRun("images").Assert(c, icmd.Success)
if out := res.Combined(); !strings.Contains(out, busyboxImage) {
c.Fatalf("unexpected output %s: shouldn't rm image %s without force\n", out, busyboxImage)
}

command.PouchRun("rmi", "-f", imageID).Assert(c, icmd.Success)
res = command.PouchRun("images").Assert(c, icmd.Success)
if out := res.Combined(); strings.Contains(out, busyboxImage) {
c.Fatalf("unexpected output %s: should rm image %s\n", out, busyboxImage)
}
}

// TestRmiInWrongWay run rmi in wrong ways.
func (suite *PouchRmiSuite) TestRmiInWrongWay(c *check.C) {
for _, tc := range []struct {
Expand Down
2 changes: 1 addition & 1 deletion test/environment/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func PruneAllContainers(apiClient client.ContainerAPIClient) error {
ctx := context.Background()
containers, err := apiClient.ContainerList(ctx, types.ContainerListOptions{All: true})
if err != nil {
return errors.Wrap(err, "fail to list images")
return errors.Wrap(err, "fail to list containers")
}

for _, ctr := range containers {
Expand Down