From 2fb1511690dba70adf25c36c28ec4bad2af79106 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 15 Feb 2021 11:58:24 -0500 Subject: Fix an issue where copyup could fail with ENOENT This one is rather bizarre because it triggers only on some systems. I've included a CI test, for example, but I'm 99% sure we use images in CI that have volumes over empty directories, and the earlier patch to change copy-up implementation passed CI without complaint. I can reproduce this on a stock F33 VM, but that's the only place I have been able to see it. Regardless, the issue: under certain as-yet-unidentified environmental conditions, the copier.Get method will return an ENOENT attempting to stream a directory that is empty. Work around this by avoiding the copy altogether in this case. Signed-off-by: Matthew Heon --- libpod/container_internal.go | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'libpod/container_internal.go') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 15958471f..ac7c1b2dc 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1615,6 +1615,17 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) if !srcStat.IsDir() { return vol, nil } + // Read contents, do not bother continuing if it's empty. Fixes + // a bizarre issue where something copier.Get will ENOENT on + // empty directories and sometimes it will not. + // RHBZ#1928643 + srcContents, err := ioutil.ReadDir(srcDir) + if err != nil { + return nil, errors.Wrapf(err, "error reading contents of source directory for copy up into volume %s", vol.Name()) + } + if len(srcContents) == 0 { + return vol, nil + } // Buildah Copier accepts a reader, so we'll need a pipe. reader, writer := io.Pipe() -- cgit v1.2.3-54-g00ecf From 59d2eac78a2a8e619d9c1c53b065f34f0e19795c Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 17 Feb 2021 14:09:28 -0500 Subject: Change source path resolution for volume copy-up Instead of using the container's mountpoint as the base of the chroot and indexing from there by the volume directory, instead use the full path of what we want to copy as the base of the chroot and copy everything in it. This resolves the bug, ends up being a bit simpler code-wise (no string concatenation, as we already have the full path calculated for other checks), and seems more understandable than trying to resolve things on the destination side of the copy-up. Fixes #9354 Signed-off-by: Matthew Heon --- libpod/container_internal.go | 2 +- test/e2e/run_volume_test.go | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) (limited to 'libpod/container_internal.go') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index ac7c1b2dc..7aaa002b2 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1642,7 +1642,7 @@ func (c *Container) mountNamedVolume(v *ContainerNamedVolume, mountpoint string) getOptions := copier.GetOptions{ KeepDirectoryNames: false, } - errChan <- copier.Get(mountpoint, "", getOptions, []string{v.Dest + "/."}, writer) + errChan <- copier.Get(srcDir, "", getOptions, []string{"/."}, writer) }() // Copy, volume side: stream what we've written to the pipe, into diff --git a/test/e2e/run_volume_test.go b/test/e2e/run_volume_test.go index 91d8f15f4..d81fb769d 100644 --- a/test/e2e/run_volume_test.go +++ b/test/e2e/run_volume_test.go @@ -334,6 +334,18 @@ RUN sh -c "cd /etc/apk && ln -s ../../testfile"` Expect(outputSession.OutputToString()).To(Equal(baselineOutput)) }) + It("podman named volume copyup of /var", func() { + baselineSession := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", fedoraMinimal, "ls", "/var"}) + baselineSession.WaitWithDefaultTimeout() + Expect(baselineSession.ExitCode()).To(Equal(0)) + baselineOutput := baselineSession.OutputToString() + + outputSession := podmanTest.Podman([]string{"run", "-t", "-i", "-v", "/var", fedoraMinimal, "ls", "/var"}) + outputSession.WaitWithDefaultTimeout() + Expect(outputSession.ExitCode()).To(Equal(0)) + Expect(outputSession.OutputToString()).To(Equal(baselineOutput)) + }) + It("podman read-only tmpfs conflict with volume", func() { session := podmanTest.Podman([]string{"run", "--rm", "-t", "-i", "--read-only", "-v", "tmp_volume:" + dest, ALPINE, "touch", dest + "/a"}) session.WaitWithDefaultTimeout() -- cgit v1.2.3-54-g00ecf From 6470f970154c4d21e591fe18d59d1f471b51b5d7 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 11 Feb 2021 22:28:35 +0100 Subject: Enable golint linter Use the golint linter and fix the reported problems. [NO TESTS NEEDED] Signed-off-by: Paul Holzinger --- .golangci.yml | 1 - cmd/podman/containers/ps.go | 10 +++++----- libpod/container_exec.go | 4 ++-- libpod/container_internal.go | 2 +- libpod/healthcheck.go | 2 +- libpod/image/utils.go | 3 +-- libpod/info.go | 6 +++--- libpod/networking_linux.go | 3 +-- libpod/plugin/volume_api.go | 3 +-- libpod/runtime_img.go | 3 +-- libpod/volume_internal_linux.go | 8 ++++---- pkg/api/handlers/compat/containers_archive.go | 2 +- pkg/api/handlers/utils/containers.go | 8 +++----- pkg/bindings/containers/archive.go | 2 +- pkg/copy/fileinfo.go | 6 +++--- pkg/domain/entities/network.go | 2 +- pkg/domain/infra/abi/containers_stat.go | 4 ++-- pkg/domain/infra/abi/images.go | 7 +++---- 18 files changed, 34 insertions(+), 42 deletions(-) (limited to 'libpod/container_internal.go') diff --git a/.golangci.yml b/.golangci.yml index 85b753ad3..5e98f5fb6 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -51,7 +51,6 @@ linters: - gochecknoinits - goconst - gocyclo - - golint - lll - structcheck - typecheck diff --git a/cmd/podman/containers/ps.go b/cmd/podman/containers/ps.go index 09921b93d..0c6cbab42 100644 --- a/cmd/podman/containers/ps.go +++ b/cmd/podman/containers/ps.go @@ -254,12 +254,12 @@ func ps(cmd *cobra.Command, _ []string) error { // responses will grow to the largest number of processes reported on, but will not thrash the gc var responses []psReporter for ; ; responses = responses[:0] { - if ctnrs, err := getResponses(); err != nil { + ctnrs, err := getResponses() + if err != nil { return err - } else { - for _, r := range ctnrs { - responses = append(responses, psReporter{r}) - } + } + for _, r := range ctnrs { + responses = append(responses, psReporter{r}) } tm.Clear() diff --git a/libpod/container_exec.go b/libpod/container_exec.go index 443d631db..0d18b55ca 100644 --- a/libpod/container_exec.go +++ b/libpod/container_exec.go @@ -78,11 +78,11 @@ type ExecConfig struct { type ExecSession struct { // Id is the ID of the exec session. // Named somewhat strangely to not conflict with ID(). - // nolint:stylecheck + // nolint:stylecheck,golint Id string `json:"id"` // ContainerId is the ID of the container this exec session belongs to. // Named somewhat strangely to not conflict with ContainerID(). - // nolint:stylecheck + // nolint:stylecheck,golint ContainerId string `json:"containerId"` // State is the state of the exec session. diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 7aaa002b2..e02cb201e 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -264,7 +264,7 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr err c.newContainerEvent(events.Restart) // Increment restart count - c.state.RestartCount += 1 + c.state.RestartCount++ logrus.Debugf("Container %s now on retry %d", c.ID(), c.state.RestartCount) if err := c.save(); err != nil { return false, err diff --git a/libpod/healthcheck.go b/libpod/healthcheck.go index f77075893..6c5becd5b 100644 --- a/libpod/healthcheck.go +++ b/libpod/healthcheck.go @@ -190,7 +190,7 @@ func (c *Container) updateHealthCheckLog(hcl define.HealthCheckLog, inStartPerio } if !inStartPeriod { // increment failing streak - healthCheck.FailingStreak += 1 + healthCheck.FailingStreak++ // if failing streak > retries, then status to unhealthy if healthCheck.FailingStreak >= c.HealthCheckConfig().Retries { healthCheck.Status = define.HealthCheckUnhealthy diff --git a/libpod/image/utils.go b/libpod/image/utils.go index 5e7fed5c6..6cc613b28 100644 --- a/libpod/image/utils.go +++ b/libpod/image/utils.go @@ -75,9 +75,8 @@ func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, er } if rwImageCnt > 1 { return nil, errors.Wrapf(define.ErrMultipleImages, "found multiple read/write images %s", strings.Join(keys, ",")) - } else { - return nil, errors.Wrapf(define.ErrMultipleImages, "found multiple read/only images %s", strings.Join(keys, ",")) } + return nil, errors.Wrapf(define.ErrMultipleImages, "found multiple read/only images %s", strings.Join(keys, ",")) } return candidates[0].image.image, nil } diff --git a/libpod/info.go b/libpod/info.go index 1b3550abd..f5bfb122e 100644 --- a/libpod/info.go +++ b/libpod/info.go @@ -222,11 +222,11 @@ func (r *Runtime) getContainerStoreInfo() (define.ContainerStore, error) { } switch state { case define.ContainerStateRunning: - running += 1 + running++ case define.ContainerStatePaused: - paused += 1 + paused++ default: - stopped += 1 + stopped++ } } cs.Paused = paused diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 9edea4fea..03edf7f02 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -480,9 +480,8 @@ func (r *Runtime) setupSlirp4netns(ctr *Container) error { if havePortMapping { if isSlirpHostForward { return r.setupRootlessPortMappingViaSlirp(ctr, cmd, apiSocket) - } else { - return r.setupRootlessPortMappingViaRLK(ctr, netnsPath) } + return r.setupRootlessPortMappingViaRLK(ctr, netnsPath) } return nil } diff --git a/libpod/plugin/volume_api.go b/libpod/plugin/volume_api.go index c5dec651c..79aebed43 100644 --- a/libpod/plugin/volume_api.go +++ b/libpod/plugin/volume_api.go @@ -241,9 +241,8 @@ func (p *VolumePlugin) makeErrorResponse(err, endpoint, volName string) error { } if volName != "" { return errors.Wrapf(errors.New(err), "error on %s on volume %s in volume plugin %s", endpoint, volName, p.Name) - } else { - return errors.Wrapf(errors.New(err), "error on %s in volume plugin %s", endpoint, p.Name) } + return errors.Wrapf(errors.New(err), "error on %s in volume plugin %s", endpoint, p.Name) } // Handle error responses from plugin diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index e6caf2626..fcc52b392 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -318,9 +318,8 @@ func (r *Runtime) LoadImageFromSingleImageArchive(ctx context.Context, writer io if err == nil && src != nil { if newImages, err := r.ImageRuntime().LoadFromArchiveReference(ctx, src, signaturePolicy, writer); err == nil { return getImageNames(newImages), nil - } else { - saveErr = err } + saveErr = err } } return "", errors.Wrapf(saveErr, "error pulling image") diff --git a/libpod/volume_internal_linux.go b/libpod/volume_internal_linux.go index e184505e7..82c01be44 100644 --- a/libpod/volume_internal_linux.go +++ b/libpod/volume_internal_linux.go @@ -45,7 +45,7 @@ func (v *Volume) mount() error { // If the count is non-zero, the volume is already mounted. // Nothing to do. if v.state.MountCount > 0 { - v.state.MountCount += 1 + v.state.MountCount++ logrus.Debugf("Volume %s mount count now at %d", v.Name(), v.state.MountCount) return v.save() } @@ -67,7 +67,7 @@ func (v *Volume) mount() error { return err } - v.state.MountCount += 1 + v.state.MountCount++ v.state.MountPoint = mountPoint return v.save() } @@ -109,7 +109,7 @@ func (v *Volume) mount() error { logrus.Debugf("Mounted volume %s", v.Name()) // Increment the mount counter - v.state.MountCount += 1 + v.state.MountCount++ logrus.Debugf("Volume %s mount count now at %d", v.Name(), v.state.MountCount) return v.save() } @@ -152,7 +152,7 @@ func (v *Volume) unmount(force bool) error { } if !force { - v.state.MountCount -= 1 + v.state.MountCount-- } else { v.state.MountCount = 0 } diff --git a/pkg/api/handlers/compat/containers_archive.go b/pkg/api/handlers/compat/containers_archive.go index 083c72ce8..55bd62f90 100644 --- a/pkg/api/handlers/compat/containers_archive.go +++ b/pkg/api/handlers/compat/containers_archive.go @@ -62,7 +62,7 @@ func handleHeadAndGet(w http.ResponseWriter, r *http.Request, decoder *schema.De w.Header().Add(copy.XDockerContainerPathStatHeader, statHeader) } - if errors.Cause(err) == define.ErrNoSuchCtr || errors.Cause(err) == copy.ENOENT { + if errors.Cause(err) == define.ErrNoSuchCtr || errors.Cause(err) == copy.ErrENOENT { // 404 is returned for an absent container and path. The // clients must deal with it accordingly. utils.Error(w, "Not found.", http.StatusNotFound, err) diff --git a/pkg/api/handlers/utils/containers.go b/pkg/api/handlers/utils/containers.go index 06a668d6f..c3ca6a2fb 100644 --- a/pkg/api/handlers/utils/containers.go +++ b/pkg/api/handlers/utils/containers.go @@ -131,10 +131,9 @@ func WaitContainerLibpod(w http.ResponseWriter, r *http.Request) { if errors.Cause(err) == define.ErrNoSuchCtr { ContainerNotFound(w, name, err) return - } else { - InternalServerError(w, err) - return } + InternalServerError(w, err) + return } WriteResponse(w, http.StatusOK, strconv.Itoa(int(exitCode))) } @@ -201,9 +200,8 @@ func waitRemoved(ctrWait containerWaitFn) (int32, error) { code, err := ctrWait(define.ContainerStateUnknown) if err != nil && errors.Cause(err) == define.ErrNoSuchCtr { return code, nil - } else { - return code, err } + return code, err } func waitNextExit(ctrWait containerWaitFn) (int32, error) { diff --git a/pkg/bindings/containers/archive.go b/pkg/bindings/containers/archive.go index d1bbc0b95..18871cfd1 100644 --- a/pkg/bindings/containers/archive.go +++ b/pkg/bindings/containers/archive.go @@ -30,7 +30,7 @@ func Stat(ctx context.Context, nameOrID string, path string) (*entities.Containe var finalErr error if response.StatusCode == http.StatusNotFound { - finalErr = copy.ENOENT + finalErr = copy.ErrENOENT } else if response.StatusCode != http.StatusOK { finalErr = errors.New(response.Status) } diff --git a/pkg/copy/fileinfo.go b/pkg/copy/fileinfo.go index ddb9b629c..b95bcd90c 100644 --- a/pkg/copy/fileinfo.go +++ b/pkg/copy/fileinfo.go @@ -16,9 +16,9 @@ import ( // base64 encoded JSON payload of stating a path in a container. const XDockerContainerPathStatHeader = "X-Docker-Container-Path-Stat" -// ENOENT mimics the stdlib's ENOENT and can be used to implement custom logic +// ErrENOENT mimics the stdlib's ErrENOENT and can be used to implement custom logic // while preserving the user-visible error message. -var ENOENT = errors.New("No such file or directory") +var ErrENOENT = errors.New("No such file or directory") // FileInfo describes a file or directory and is returned by // (*CopyItem).Stat(). @@ -70,7 +70,7 @@ func ResolveHostPath(path string) (*FileInfo, error) { statInfo, err := os.Stat(resolvedHostPath) if err != nil { if os.IsNotExist(err) { - return nil, ENOENT + return nil, ErrENOENT } return nil, err } diff --git a/pkg/domain/entities/network.go b/pkg/domain/entities/network.go index b41a9997e..95608f01b 100644 --- a/pkg/domain/entities/network.go +++ b/pkg/domain/entities/network.go @@ -31,7 +31,7 @@ type NetworkReloadOptions struct { // NetworkReloadReport describes the results of reloading a container network. type NetworkReloadReport struct { - // nolint:stylecheck + // nolint:stylecheck,golint Id string Err error } diff --git a/pkg/domain/infra/abi/containers_stat.go b/pkg/domain/infra/abi/containers_stat.go index f3d0799a0..1844f4019 100644 --- a/pkg/domain/infra/abi/containers_stat.go +++ b/pkg/domain/infra/abi/containers_stat.go @@ -35,7 +35,7 @@ func (ic *ContainerEngine) containerStat(container *libpod.Container, containerM // ENOENT let's the API handlers return the correct status code // which is crucial for the remote client. if os.IsNotExist(err) || strings.Contains(statInfoErr.Error(), "o such file or directory") { - statInfoErr = copy.ENOENT + statInfoErr = copy.ErrENOENT } // If statInfo is nil, there's nothing we can do anymore. A // non-nil statInfo may indicate a symlink where we must have @@ -129,7 +129,7 @@ func secureStat(root string, path string) (*buildahCopiah.StatForItem, error) { stat, exists := globStats[0].Results[glob] // only one glob passed, so that's okay if !exists { - return nil, copy.ENOENT + return nil, copy.ErrENOENT } var statErr error diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go index f2d0f2c39..cf108ba9c 100644 --- a/pkg/domain/infra/abi/images.go +++ b/pkg/domain/infra/abi/images.go @@ -40,10 +40,9 @@ func (ir *ImageEngine) Exists(_ context.Context, nameOrID string) (*entities.Boo if err != nil { if errors.Cause(err) == define.ErrMultipleImages { return &entities.BoolReport{Value: true}, nil - } else { - if errors.Cause(err) != define.ErrNoSuchImage { - return nil, err - } + } + if errors.Cause(err) != define.ErrNoSuchImage { + return nil, err } } return &entities.BoolReport{Value: err == nil}, nil -- cgit v1.2.3-54-g00ecf