Skip to content

Commit

Permalink
Simplify the interface of GetCacheMount and getCacheMount
Browse files Browse the repository at this point in the history 
It can return at most one lock, so don't return an array.

Should not change behavior right now, but it will simplify
cleanup.

Signed-off-by: Miloslav Trmač <[email protected]>
  • Loading branch information
@mtrmac
mtrmac committed Oct 19, 2022
1 parent c2adfd5 commit 0c716ff
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 33 deletions.
54 changes: 28 additions & 26 deletions internal/parse/parse.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,10 @@ func GetBindMount(ctx *types.SystemContext, args []string, contextDir string, st

// GetCacheMount parses a single cache mount entry from the --mount flag.
//
// The caller should eventually unlock all of the returned lockfile.Locker locks, even if this function fails.
func GetCacheMount(args []string, store storage.Store, imageMountLabel string, additionalMountPoints map[string]internal.StageMountDetails) (specs.Mount, []lockfile.Locker, error) {
// The caller should eventually unlock returned lockfile.Locker, if it is non-nil, even if this function fails.
func GetCacheMount(args []string, store storage.Store, imageMountLabel string, additionalMountPoints map[string]internal.StageMountDetails) (specs.Mount, lockfile.Locker, error) {
var err error
var mode uint64
targetLocks := make([]lockfile.Locker, 0)
var (
setDest bool
setShared bool
Expand Down Expand Up @@ -237,59 +236,59 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
sharing = kv[1]
case "bind-propagation":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
newMount.Options = append(newMount.Options, kv[1])
case "id":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
id = kv[1]
case "from":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
fromStage = kv[1]
case "target", "dst", "destination":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
if err := parse.ValidateVolumeCtrDir(kv[1]); err != nil {
return newMount, targetLocks, err
return newMount, nil, err
}
newMount.Destination = kv[1]
setDest = true
case "src", "source":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
newMount.Source = kv[1]
case "mode":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
mode, err = strconv.ParseUint(kv[1], 8, 32)
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to parse cache mode: %w", err)
return newMount, nil, fmt.Errorf("unable to parse cache mode: %w", err)
}
case "uid":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
uid, err = strconv.Atoi(kv[1])
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to parse cache uid: %w", err)
return newMount, nil, fmt.Errorf("unable to parse cache uid: %w", err)
}
case "gid":
if len(kv) == 1 {
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadOptionArg)
}
gid, err = strconv.Atoi(kv[1])
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to parse cache gid: %w", err)
return newMount, nil, fmt.Errorf("unable to parse cache gid: %w", err)
}
default:
return newMount, targetLocks, fmt.Errorf("%v: %w", kv[0], errBadMntOption)
return newMount, nil, fmt.Errorf("%v: %w", kv[0], errBadMntOption)
}
}

Expand All @@ -300,7 +299,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
}

if !setDest {
return newMount, targetLocks, errBadVolDest
return newMount, nil, errBadVolDest
}

if fromStage != "" {
Expand All @@ -317,7 +316,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
// Cache does not supports using image so if not stage found
// return with error
if mountPoint == "" {
return newMount, targetLocks, fmt.Errorf("no stage found with name %s", fromStage)
return newMount, nil, fmt.Errorf("no stage found with name %s", fromStage)
}
// path should be /contextDir/specified path
newMount.Source = filepath.Join(mountPoint, filepath.Clean(string(filepath.Separator)+newMount.Source))
Expand All @@ -333,7 +332,7 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
// create cache on host if not present
err = os.MkdirAll(cacheParent, os.FileMode(0755))
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to create build cache directory: %w", err)
return newMount, nil, fmt.Errorf("unable to create build cache directory: %w", err)
}

if id != "" {
Expand All @@ -348,26 +347,27 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a
//buildkit parity: change uid and gid if specified otheriwise keep `0`
err = idtools.MkdirAllAndChownNew(newMount.Source, os.FileMode(mode), idPair)
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to change uid,gid of cache directory: %w", err)
return newMount, nil, fmt.Errorf("unable to change uid,gid of cache directory: %w", err)
}
}

var targetLock lockfile.Locker // = nil
switch sharing {
case "locked":
// lock parent cache
lockfile, err := lockfile.GetLockfile(filepath.Join(newMount.Source, BuildahCacheLockfile))
if err != nil {
return newMount, targetLocks, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err)
return newMount, targetLock, fmt.Errorf("unable to acquire lock when sharing mode is locked: %w", err)
}
// Will be unlocked after the RUN step is executed.
lockfile.Lock()
targetLocks = append(targetLocks, lockfile)
targetLock = lockfile
case "shared":
// do nothing since default is `shared`
break
default:
// error out for unknown values
return newMount, targetLocks, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err)
return newMount, targetLock, fmt.Errorf("unrecognized value %q for field `sharing`: %w", sharing, err)
}

// buildkit parity: default sharing should be shared
Expand All @@ -385,11 +385,11 @@ func GetCacheMount(args []string, store storage.Store, imageMountLabel string, a

opts, err := parse.ValidateVolumeOpts(newMount.Options)
if err != nil {
return newMount, targetLocks, err
return newMount, targetLock, err
}
newMount.Options = opts

return newMount, targetLocks, nil
return newMount, targetLock, nil
}

// ValidateVolumeMountHostDir validates the host path of buildah --volume
Expand Down Expand Up @@ -547,7 +547,9 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mounts []string, c
mountedImages = append(mountedImages, image)
case TypeCache:
mount, tl, err := GetCacheMount(tokens, store, "", nil)
targetLocks = append(targetLocks, tl...)
if tl != nil {
targetLocks = append(targetLocks, tl)
}
if err != nil {
return nil, mountedImages, targetLocks, err
}
Expand Down
4 changes: 3 additions & 1 deletion run_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1519,7 +1519,9 @@ func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMap
}
finalMounts = append(finalMounts, *mount)
mountTargets = append(mountTargets, mount.Destination)
targetLocks = append(targetLocks, tl...)
if tl != nil {
targetLocks = append(targetLocks, tl)
}
default:
return nil, nil, fmt.Errorf("invalid mount type %q", mountType)
}
Expand Down
2 changes: 1 addition & 1 deletion run_freebsd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func setupSpecialMountSpecChanges(spec *spec.Spec, shmSize string) ([]specs.Moun
return spec.Mounts, nil
}

func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, []lockfile.Locker, error) {
func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) {
return nil, nil, errors.New("cache mounts not supported on freebsd")
}

Expand Down
11 changes: 6 additions & 5 deletions run_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1196,18 +1196,19 @@ func checkIdsGreaterThan5(ids []spec.LinuxIDMapping) bool {
return false
}

func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, []lockfile.Locker, error) {
// The caller should eventually unlock returned lockfile.Locker, if it is non-nil, even if this function fails.
func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, lockfile.Locker, error) {
var optionMounts []specs.Mount
mount, targetLocks, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints)
mount, targetLock, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints)
if err != nil {
return nil, targetLocks, err
return nil, targetLock, err
}
optionMounts = append(optionMounts, mount)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
if err != nil {
return nil, targetLocks, err
return nil, targetLock, err
}
return &volumes[0], targetLocks, nil
return &volumes[0], targetLock, nil
}

// setPdeathsig sets a parent-death signal for the process
Expand Down

0 comments on commit 0c716ff

Please sign in to comment.