From c05888b62603ce8a8c8aadfe948d1a63994902fd Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Thu, 9 Sep 2021 15:10:50 -0500 Subject: [PATCH 01/10] Split ParseBindPath from config Get/Setters & add basic tests --- pkg/runtime/engine/singularity/config/bind.go | 259 ++++++++++++++++++ .../engine/singularity/config/bind_test.go | 202 ++++++++++++++ .../engine/singularity/config/config.go | 237 ---------------- 3 files changed, 461 insertions(+), 237 deletions(-) create mode 100644 pkg/runtime/engine/singularity/config/bind.go create mode 100644 pkg/runtime/engine/singularity/config/bind_test.go diff --git a/pkg/runtime/engine/singularity/config/bind.go b/pkg/runtime/engine/singularity/config/bind.go new file mode 100644 index 0000000000..3656a55991 --- /dev/null +++ b/pkg/runtime/engine/singularity/config/bind.go @@ -0,0 +1,259 @@ +// Copyright (c) 2019-2021, Sylabs Inc. All rights reserved. +// This software is licensed under a 3-clause BSD license. Please consult the +// LICENSE.md file distributed with the sources of this project regarding your +// rights to use or distribute this software. + +package singularity + +import ( + "fmt" + "regexp" + "strings" +) + +// BindOption represents a bind option with its associated +// value if any. +type BindOption struct { + Value string `json:"value,omitempty"` +} + +const ( + flagOption = true + valueOption = false +) + +// bindOptions is a map of option strings valid in bind specifications. +// If true, the option is a flag. If false, the option takes a value. +var bindOptions = map[string]bool{ + "ro": flagOption, + "rw": flagOption, + "image-src": valueOption, + "id": valueOption, +} + +// BindPath stores a parsed bind path specification. Source and Destination +// paths are required. +type BindPath struct { + Source string `json:"source"` + Destination string `json:"destination"` + Options map[string]*BindOption `json:"options"` +} + +// ImageSrc returns the value of the option image-src for a BindPath, or an +// empty string if the option wasn't set. +func (b *BindPath) ImageSrc() string { + if b.Options != nil && b.Options["image-src"] != nil { + src := b.Options["image-src"].Value + if src == "" { + return "/" + } + return src + } + return "" +} + +// ID returns the value of the option id for a BindPath, or an empty string if +// the option wasn't set. +func (b *BindPath) ID() string { + if b.Options != nil && b.Options["id"] != nil { + return b.Options["id"].Value + } + return "" +} + +// Readonly returns true if the ro option was set for a BindPath. +func (b *BindPath) Readonly() bool { + return b.Options != nil && b.Options["ro"] != nil +} + +// ParseBindPath parses a string specifying one or more (comma separated) bind +// paths in src[:dst[:options]] format, and returns all encountered bind paths +// as a slice. Options may be simple flags, e.g. 'rw', or take a value, e.g. +// 'id=2'. Multiple options are separated with commas. Note that multiple binds +// are also separated with commas, so the logic must distinguish. +func ParseBindPath(paths []string) ([]BindPath, error) { + var binds []BindPath + + // there is a better regular expression to handle + // that directly without all the logic below ... + // we need to parse various syntax: + // source1 + // source1:destination1 + // source1:destination1:option1 + // source1:destination1:option1,option2 + // source1,source2 + // source1:destination1:option1,source2 + re := regexp.MustCompile(`([^,^:]+:?)`) + + // with the regex above we get string array: + // - source1 -> [source1] + // - source1:destination1 -> [source1:, destination1] + // - source1:destination1:option1 -> [source1:, destination1:, option1] + // - source1:destination1:option1,option2 -> [source1:, destination1:, option1, option2] + + for _, path := range paths { + concatComma := false + concatColon := false + bind := "" + elem := 0 + + for _, m := range re.FindAllString(path, -1) { + s := strings.TrimSpace(m) + + isOption := false + + for option, flag := range bindOptions { + if flag { + if s == option { + isOption = true + break + } + } else { + if strings.HasPrefix(s, option+"=") { + isOption = true + break + } + } + } + + if elem == 2 && !isOption { + bp, err := newBindPath(bind) + if err != nil { + return nil, fmt.Errorf("while getting bind path: %s", err) + } + binds = append(binds, bp) + elem = 0 + bind = "" + } + + if elem == 0 { + // escaped commas and colons + if (len(s) > 0 && s[len(s)-1] == '\\') || concatComma { + if !concatComma { + bind += s[:len(s)-1] + "," + } else { + bind += s + elem++ + } + concatComma = !concatComma + continue + } else if (len(s) >= 2 && s[len(s)-2] == '\\' && s[len(s)-1] == ':') || concatColon { + bind += s + if concatColon { + elem++ + } + concatColon = !concatColon + continue + } else if bind == "" { + bind = s + } + } + + isColon := bind != "" && bind[len(bind)-1] == ':' + + // options are taken only if the bind has a source + // and a destination + if elem == 2 && isOption { + if !isColon { + bind += "," + } + bind += s + continue + } else if elem > 2 { + return nil, fmt.Errorf("wrong bind syntax: %s", bind) + } + + if bind != "" { + if isColon { + if elem > 0 { + bind += s + } + elem++ + continue + } + bp, err := newBindPath(bind) + if err != nil { + return nil, fmt.Errorf("while getting bind path: %s", err) + } + binds = append(binds, bp) + elem = 0 + bind = "" + continue + } + // new bind path + bind = s + elem++ + } + + if bind != "" { + bp, err := newBindPath(bind) + if err != nil { + return nil, fmt.Errorf("while getting bind path: %s", err) + } + binds = append(binds, bp) + } + } + + return binds, nil +} + +func splitBy(str string, sep byte) []string { + var list []string + + re := regexp.MustCompile(fmt.Sprintf(`(?m)([^\\]%c)`, sep)) + cursor := 0 + + indexes := re.FindAllStringIndex(str, -1) + for i, index := range indexes { + list = append(list, str[cursor:index[1]-1]) + cursor = index[1] + if len(indexes)-1 == i { + return append(list, str[cursor:]) + } + } + + return append(list, str) +} + +// newBindPath returns BindPath record based on the provided bind +// string argument and ensures that the options are valid. +func newBindPath(bind string) (BindPath, error) { + var bp BindPath + + splitted := splitBy(bind, ':') + + bp.Source = strings.ReplaceAll(splitted[0], "\\:", ":") + if bp.Source == "" { + return bp, fmt.Errorf("empty bind source for bind path %q", bind) + } + + bp.Destination = bp.Source + + if len(splitted) > 1 { + bp.Destination = splitted[1] + } + + if len(splitted) > 2 { + bp.Options = make(map[string]*BindOption) + + for _, value := range strings.Split(splitted[2], ",") { + valid := false + for optName, isFlag := range bindOptions { + if isFlag && optName == value { + bp.Options[optName] = &BindOption{} + valid = true + break + } else if strings.HasPrefix(value, optName+"=") { + bp.Options[optName] = &BindOption{Value: value[len(optName+"="):]} + valid = true + break + } + } + if !valid { + return bp, fmt.Errorf("%s is not a valid bind option", value) + } + } + } + + return bp, nil +} diff --git a/pkg/runtime/engine/singularity/config/bind_test.go b/pkg/runtime/engine/singularity/config/bind_test.go new file mode 100644 index 0000000000..c95b324350 --- /dev/null +++ b/pkg/runtime/engine/singularity/config/bind_test.go @@ -0,0 +1,202 @@ +// Copyright (c) 2021, Sylabs Inc. All rights reserved. +// This software is licensed under a 3-clause BSD license. Please consult the +// LICENSE.md file distributed with the sources of this project regarding your +// rights to use or distribute this software. + +package singularity + +import ( + "reflect" + "testing" +) + +func TestParseBindPath(t *testing.T) { + tests := []struct { + name string + paths []string + want []BindPath + wantErr bool + }{ + { + name: "srcOnly", + paths: []string{"/opt"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + }, + }, + }, + { + name: "srcOnlyMultiple", + paths: []string{"/opt,/tmp"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + }, + { + Source: "/tmp", + Destination: "/tmp", + }, + }, + }, + { + name: "srcDst", + paths: []string{"/opt:/other"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + }, + }, + }, + { + name: "srcDstMultiple", + paths: []string{"/opt:/other,/tmp:/other2,"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + }, + { + Source: "/tmp", + Destination: "/other2", + }, + }, + }, + { + name: "srcDstRO", + paths: []string{"/opt:/other:ro"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + Options: map[string]*BindOption{ + "ro": {}, + }, + }, + }, + }, + { + name: "srcDstROMultiple", + paths: []string{"/opt:/other:ro,/tmp:/other2:ro"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + Options: map[string]*BindOption{ + "ro": {}, + }, + }, + { + Source: "/tmp", + Destination: "/other2", + Options: map[string]*BindOption{ + "ro": {}, + }, + }, + }, + }, + { + // This doesn't make functional sense (ro & rw), but is testing + // parsing multiple simple options. + name: "srcDstRORW", + paths: []string{"/opt:/other:ro,rw"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + Options: map[string]*BindOption{ + "ro": {}, + "rw": {}, + }, + }, + }, + }, + { + // This doesn't make functional sense (ro & rw), but is testing + // parsing multiple binds, with multiple options each. Note the + // complex parsing here that has to distinguish between comma + // delimiting an additional option, vs an additional bind. + name: "srcDstRORWMultiple", + paths: []string{"/opt:/other:ro,rw,/tmp:/other2:ro,rw"}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/other", + Options: map[string]*BindOption{ + "ro": {}, + "rw": {}, + }, + }, + { + Source: "/tmp", + Destination: "/other2", + Options: map[string]*BindOption{ + "ro": {}, + "rw": {}, + }, + }, + }, + }, + { + name: "srcDstImageSrc", + paths: []string{"test.sif:/other:image-src=/opt"}, + want: []BindPath{ + { + Source: "test.sif", + Destination: "/other", + Options: map[string]*BindOption{ + "image-src": {"/opt"}, + }, + }, + }, + }, + { + // Can't use image-src without a value + name: "srcDstImageSrcNoVal", + paths: []string{"test.sif:/other:image-src"}, + want: []BindPath{}, + wantErr: true, + }, + { + name: "srcDstId", + paths: []string{"test.sif:/other:image-src=/opt,id=2"}, + want: []BindPath{ + { + Source: "test.sif", + Destination: "/other", + Options: map[string]*BindOption{ + "image-src": {"/opt"}, + "id": {"2"}, + }, + }, + }, + }, + { + name: "invalidOption", + paths: []string{"/opt:/other:invalid"}, + want: []BindPath{}, + wantErr: true, + }, + { + name: "invalidSpec", + paths: []string{"/opt:/other:rw:invalid"}, + want: []BindPath{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseBindPath(tt.paths) + if (err != nil) != tt.wantErr { + t.Errorf("ParseBindPath() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && !reflect.DeepEqual(got, tt.want) { + t.Errorf("ParseBindPath() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/runtime/engine/singularity/config/config.go b/pkg/runtime/engine/singularity/config/config.go index 874b488b4b..7e141c2578 100644 --- a/pkg/runtime/engine/singularity/config/config.go +++ b/pkg/runtime/engine/singularity/config/config.go @@ -8,7 +8,6 @@ package singularity import ( "fmt" "os/exec" - "regexp" "strings" "github.com/hpcng/singularity/internal/pkg/runtime/engine/config/oci" @@ -60,46 +59,6 @@ type FuseMount struct { Cmd *exec.Cmd `json:"-"` // holds the process exec command when FUSE driver run in foreground mode } -// BindOption represents a bind option with its associated -// value if any. -type BindOption struct { - Value string `json:"value,omitempty"` -} - -// BindPath stores bind path. -type BindPath struct { - Source string `json:"source"` - Destination string `json:"destination"` - Options map[string]*BindOption `json:"options"` -} - -// ImageSrc returns the value of option image-src or an empty -// string if the option wasn't set. -func (b *BindPath) ImageSrc() string { - if b.Options != nil && b.Options["image-src"] != nil { - src := b.Options["image-src"].Value - if src == "" { - return "/" - } - return src - } - return "" -} - -// ID returns the value of option id or an empty -// string if the option wasn't set. -func (b *BindPath) ID() string { - if b.Options != nil && b.Options["id"] != nil { - return b.Options["id"].Value - } - return "" -} - -// Readonly returns the option ro was set or not. -func (b *BindPath) Readonly() bool { - return b.Options != nil && b.Options["ro"] != nil -} - // JSONConfig stores engine specific confguration that is allowed to be set by the user. type JSONConfig struct { ScratchDir []string `json:"scratchdir,omitempty"` @@ -293,202 +252,6 @@ func (e *EngineConfig) GetCustomHome() bool { return e.JSON.CustomHome } -// ParseBindPath parses a string and returns all encountered -// bind paths as array. -func ParseBindPath(paths []string) ([]BindPath, error) { - var binds []BindPath - - validOptions := map[string]bool{ - "ro": true, - "rw": true, - "image-src": false, - "id": false, - } - - // there is a better regular expression to handle - // that directly without all the logic below ... - // we need to parse various syntax: - // source1 - // source1:destination1 - // source1:destination1:option1 - // source1:destination1:option1,option2 - // source1,source2 - // source1:destination1:option1,source2 - re := regexp.MustCompile(`([^,^:]+:?)`) - - // with the regex above we get string array: - // - source1 -> [source1] - // - source1:destination1 -> [source1:, destination1] - // - source1:destination1:option1 -> [source1:, destination1:, option1] - // - source1:destination1:option1,option2 -> [source1:, destination1:, option1, option2] - - for _, path := range paths { - concatComma := false - concatColon := false - bind := "" - elem := 0 - - for _, m := range re.FindAllString(path, -1) { - s := strings.TrimSpace(m) - - isOption := false - - for option, flag := range validOptions { - if flag { - if s == option { - isOption = true - break - } - } else { - if strings.HasPrefix(s, option+"=") { - isOption = true - break - } - } - } - - if elem == 2 && !isOption { - bp, err := newBindPath(bind, validOptions) - if err != nil { - return nil, fmt.Errorf("while getting bind path: %s", err) - } - binds = append(binds, bp) - elem = 0 - bind = "" - } - - if elem == 0 { - // escaped commas and colons - if (len(s) > 0 && s[len(s)-1] == '\\') || concatComma { - if !concatComma { - bind += s[:len(s)-1] + "," - } else { - bind += s - elem++ - } - concatComma = !concatComma - continue - } else if (len(s) >= 2 && s[len(s)-2] == '\\' && s[len(s)-1] == ':') || concatColon { - bind += s - if concatColon { - elem++ - } - concatColon = !concatColon - continue - } else if bind == "" { - bind = s - } - } - - isColon := bind != "" && bind[len(bind)-1] == ':' - - // options are taken only if the bind has a source - // and a destination - if elem == 2 && isOption { - if !isColon { - bind += "," - } - bind += s - continue - } else if elem > 2 { - return nil, fmt.Errorf("wrong bind syntax: %s", bind) - } - - if bind != "" { - if isColon { - if elem > 0 { - bind += s - } - elem++ - continue - } - bp, err := newBindPath(bind, validOptions) - if err != nil { - return nil, fmt.Errorf("while getting bind path: %s", err) - } - binds = append(binds, bp) - elem = 0 - bind = "" - continue - } - // new bind path - bind = s - elem++ - } - - if bind != "" { - bp, err := newBindPath(bind, validOptions) - if err != nil { - return nil, fmt.Errorf("while getting bind path: %s", err) - } - binds = append(binds, bp) - } - } - - return binds, nil -} - -func splitBy(str string, sep byte) []string { - var list []string - - re := regexp.MustCompile(fmt.Sprintf(`(?m)([^\\]%c)`, sep)) - cursor := 0 - - indexes := re.FindAllStringIndex(str, -1) - for i, index := range indexes { - list = append(list, str[cursor:index[1]-1]) - cursor = index[1] - if len(indexes)-1 == i { - return append(list, str[cursor:]) - } - } - - return append(list, str) -} - -// newBindPath returns BindPath record based on the provided bind -// string argument and ensures that the options are valid. -func newBindPath(bind string, validOptions map[string]bool) (BindPath, error) { - var bp BindPath - - splitted := splitBy(bind, ':') - - bp.Source = strings.ReplaceAll(splitted[0], "\\:", ":") - if bp.Source == "" { - return bp, fmt.Errorf("empty bind source for bind path %q", bind) - } - - bp.Destination = bp.Source - - if len(splitted) > 1 { - bp.Destination = splitted[1] - } - - if len(splitted) > 2 { - bp.Options = make(map[string]*BindOption) - - for _, value := range strings.Split(splitted[2], ",") { - valid := false - for optName, optFlag := range validOptions { - if optFlag && optName == value { - bp.Options[optName] = &BindOption{} - valid = true - break - } else if strings.HasPrefix(value, optName+"=") { - bp.Options[optName] = &BindOption{Value: value[len(optName+"="):]} - valid = true - break - } - } - if !valid { - return bp, fmt.Errorf("%s is not a valid bind option", value) - } - } - } - - return bp, nil -} - // SetBindPath sets the paths to bind into container. func (e *EngineConfig) SetBindPath(bindpath []BindPath) { e.JSON.BindPath = bindpath From 83f6282aecadb7a11a28a392dd906cceb60bb615 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 12:29:39 -0500 Subject: [PATCH 02/10] Add ParseMountString for long-form mount specs --- .../engine/singularity/config/mount.go | 94 ++++++++ .../engine/singularity/config/mount_test.go | 208 ++++++++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 pkg/runtime/engine/singularity/config/mount.go create mode 100644 pkg/runtime/engine/singularity/config/mount_test.go diff --git a/pkg/runtime/engine/singularity/config/mount.go b/pkg/runtime/engine/singularity/config/mount.go new file mode 100644 index 0000000000..0791773ebe --- /dev/null +++ b/pkg/runtime/engine/singularity/config/mount.go @@ -0,0 +1,94 @@ +// Copyright (c) 2021, Sylabs Inc. All rights reserved. +// This software is licensed under a 3-clause BSD license. Please consult the +// LICENSE.md file distributed with the sources of this project regarding your +// rights to use or distribute this software. + +package singularity + +import ( + "encoding/csv" + "fmt" + "strings" +) + +// Parse a --mount string into a BindPath struct. +// +// Our intention is to support common docker --mount strings, but have +// additional fields for singularity specific concepts (image-src, id when +// binding out of a SIF partition). +// +// We use a CSV reader to parsing the fields in a mount string according to CSV +// escaping rules. This is the approach docker uses to allow special characters +// in source / dest etc., and we wish to be as compatible as possible. +// +// The fields are in key[=value] format. Flag options have no value, e.g.: +// type=bind,source=/opt,destination=/other,rw +// +// We only support type=bind at present, so assume this if type is missing and +// error for other types. +func ParseMountString(mount string) (bp BindPath, err error) { + r := strings.NewReader(mount) + c := csv.NewReader(r) + records, err := c.ReadAll() + if err != nil { + return BindPath{}, fmt.Errorf("error parsing mount: %v", err) + } + if len(records) != 1 { + return BindPath{}, fmt.Errorf("each mount string must specify exactly one mount") + } + + bp = BindPath{ + Options: map[string]*BindOption{}, + } + + for _, f := range records[0] { + kv := strings.SplitN(f, "=", 2) + key := kv[0] + val := "" + if len(kv) > 1 { + val = kv[1] + } + + switch key { + // TODO - Eventually support volume and tmpfs? Requires structural changes to engine mount functionality. + case "type": + if val != "bind" { + return BindPath{}, fmt.Errorf("unsupported mount type %q, only 'bind' is supported", val) + } + case "source", "src": + if val == "" { + return BindPath{}, fmt.Errorf("mount source cannot be empty") + } + bp.Source = val + case "destination", "dst", "target": + if val == "" { + return BindPath{}, fmt.Errorf("mount destination cannot be empty") + } + bp.Destination = val + case "ro", "readonly": + bp.Options["ro"] = &BindOption{} + // Singularity only - directory inside an image file source to mount from + case "image-src": + if val == "" { + return BindPath{}, fmt.Errorf("img-src cannot be empty") + } + bp.Options["image-src"] = &BindOption{Value: val} + // Singularity only - id of the descriptor in a SIF image source to mount from + case "id": + if val == "" { + return BindPath{}, fmt.Errorf("id cannot be empty") + } + bp.Options["id"] = &BindOption{Value: val} + case "bind-propagation": + return BindPath{}, fmt.Errorf("bind-propagation not supported for individual mounts, check singularity.conf for global setting") + default: + return BindPath{}, fmt.Errorf("invalid key %q in mount specification", key) + } + } + + if bp.Source == "" || bp.Destination == "" { + return BindPath{}, fmt.Errorf("mounts must specify a source and a destination") + } + + return bp, nil +} diff --git a/pkg/runtime/engine/singularity/config/mount_test.go b/pkg/runtime/engine/singularity/config/mount_test.go new file mode 100644 index 0000000000..a5da84d05c --- /dev/null +++ b/pkg/runtime/engine/singularity/config/mount_test.go @@ -0,0 +1,208 @@ +// Copyright (c) 2021, Sylabs Inc. All rights reserved. +// This software is licensed under a 3-clause BSD license. Please consult the +// LICENSE.md file distributed with the sources of this project regarding your +// rights to use or distribute this software. + +package singularity + +import ( + "reflect" + "testing" +) + +func TestParseMountString(t *testing.T) { + tests := []struct { + name string + mountString string + want BindPath + wantErr bool + }{ + { + name: "sourceOnly", + mountString: "type=bind,source=/opt", + want: BindPath{}, + wantErr: true, + }, + { + name: "destinationOnly", + mountString: "type=bind,destination=/opt", + want: BindPath{}, + wantErr: true, + }, + { + name: "emptySource", + mountString: "type=bind,source=,destination=/opt", + want: BindPath{}, + wantErr: true, + }, + { + name: "emptyDestination", + mountString: "type=bind,source=/opt,destination=", + want: BindPath{}, + wantErr: true, + }, + { + name: "invalidType", + mountString: "type=potato,source=/opt,destination=/opt", + want: BindPath{}, + wantErr: true, + }, + { + name: "invalidField", + mountString: "type=bind,source=/opt,destination=/opt,colour=turquoise", + want: BindPath{}, + wantErr: true, + }, + { + name: "simple", + mountString: "type=bind,source=/opt,destination=/opt", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + { + name: "simpleSrc", + mountString: "type=bind,src=/opt,destination=/opt", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + { + name: "simpleDst", + mountString: "type=bind,source=/opt,dst=/opt", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + { + name: "simpleTarget", + mountString: "type=bind,source=/opt,target=/opt", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + { + name: "noType", + mountString: "source=/opt,destination=/opt", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + { + name: "ro", + mountString: "type=bind,source=/opt,destination=/opt,ro", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{ + "ro": {}, + }, + }, + wantErr: false, + }, + { + name: "readonly", + mountString: "type=bind,source=/opt,destination=/opt,readonly", + want: BindPath{ + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{ + "ro": {}, + }, + }, + wantErr: false, + }, + { + name: "imagesrc", + mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt", + want: BindPath{ + Source: "test.sif", + Destination: "/opt", + Options: map[string]*BindOption{ + "image-src": {Value: "/opt"}, + }, + }, + wantErr: false, + }, + { + name: "imagesrcNoValue", + mountString: "type=bind,source=test.sif,destination=/opt,image-src", + want: BindPath{}, + wantErr: true, + }, + { + name: "imagesrcEmpty", + mountString: "type=bind,source=test.sif,destination=/opt,image-src=", + want: BindPath{}, + wantErr: true, + }, + { + name: "id", + mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id=2", + want: BindPath{ + Source: "test.sif", + Destination: "/opt", + Options: map[string]*BindOption{ + "image-src": {Value: "/opt"}, + "id": {Value: "2"}, + }, + }, + wantErr: false, + }, + { + name: "idNoValue", + mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id", + want: BindPath{}, + wantErr: true, + }, + { + name: "idEmpty", + mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id=", + want: BindPath{}, + wantErr: true, + }, + { + name: "bindpropagation", + mountString: "type=bind,source=/opt,destination=/opt,bind-propagation=shared", + want: BindPath{}, + wantErr: true, + }, + { + name: "csvEscaped", + mountString: `type=bind,"source=/comma,dir","destination=/quote""dir"`, + want: BindPath{ + Source: "/comma,dir", + Destination: "/quote\"dir", + Options: map[string]*BindOption{}, + }, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseMountString(tt.mountString) + if (err != nil) != tt.wantErr { + t.Errorf("ParseMountString() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && !reflect.DeepEqual(got, tt.want) { + t.Errorf("ParseMountString() = %v, want %v", got, tt.want) + } + }) + } +} From a65db47024dd20910391a232cf39f0164b488866 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 12:58:24 -0500 Subject: [PATCH 03/10] Add StringArray flag We need a flag type that doesn't split on commas in the value, for our `--mount` values. --- pkg/cmdline/flag.go | 18 +++++++++++------- pkg/cmdline/flag_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/pkg/cmdline/flag.go b/pkg/cmdline/flag.go index 201267445a..af71f2273d 100644 --- a/pkg/cmdline/flag.go +++ b/pkg/cmdline/flag.go @@ -13,8 +13,6 @@ import ( "github.com/spf13/pflag" ) -type StringArray []string - // Flag holds information about a command flag type Flag struct { ID string @@ -29,6 +27,10 @@ type Flag struct { Required bool EnvKeys []string EnvHandler EnvHandler + // When Value is a []String: + // If true, will use pFlag StringArrayVar(P) type, where values are not split on comma. + // If false, will use pFlag StringSliceVar(P) type, where a single value is split on commas. + StringArray bool } // flagManager manages cobra command flags and store them @@ -78,9 +80,11 @@ func (m *flagManager) registerFlagForCmd(flag *Flag, cmds ...*cobra.Command) err case string: m.registerStringVar(flag, cmds) case []string: - m.registerStringSliceVar(flag, cmds) - case StringArray: - m.registerStringArrayVar(flag, cmds) + if flag.StringArray { + m.registerStringArrayVar(flag, cmds) + } else { + m.registerStringSliceVar(flag, cmds) + } case bool: m.registerBoolVar(flag, cmds) case int: @@ -121,9 +125,9 @@ func (m *flagManager) registerStringSliceVar(flag *Flag, cmds []*cobra.Command) func (m *flagManager) registerStringArrayVar(flag *Flag, cmds []*cobra.Command) error { for _, c := range cmds { if flag.ShortHand != "" { - c.Flags().StringArrayVarP(flag.Value.(*[]string), flag.Name, flag.ShortHand, ([]string)(flag.DefaultValue.(StringArray)), flag.Usage) + c.Flags().StringArrayVarP(flag.Value.(*[]string), flag.Name, flag.ShortHand, flag.DefaultValue.([]string), flag.Usage) } else { - c.Flags().StringArrayVar(flag.Value.(*[]string), flag.Name, ([]string)(flag.DefaultValue.(StringArray)), flag.Usage) + c.Flags().StringArrayVar(flag.Value.(*[]string), flag.Name, flag.DefaultValue.([]string), flag.Usage) } m.setFlagOptions(flag, c) } diff --git a/pkg/cmdline/flag_test.go b/pkg/cmdline/flag_test.go index 13b6530420..1907344acc 100644 --- a/pkg/cmdline/flag_test.go +++ b/pkg/cmdline/flag_test.go @@ -17,6 +17,7 @@ var ( testString string testBool bool testStringSlice []string + testStringArray []string testInt int testUint32 uint32 ) @@ -151,6 +152,34 @@ var ttData = []struct { }, cmd: parentCmd, }, + { + desc: "string array flag", + flag: &Flag{ + ID: "testStringArrayFlag", + Value: &testStringArray, + DefaultValue: testStringArray, + Name: "string-array", + Usage: "a string array flag", + EnvKeys: []string{"STRING_ARRAY"}, + StringArray: true, + }, + cmd: parentCmd, + envValue: "arg1,arg2", + // Should not split on commas + matchValue: "[\"arg1,arg2\"]", + }, + { + desc: "string array flag (short)", + flag: &Flag{ + ID: "testStringArrayShortFlag", + Value: &testStringArray, + DefaultValue: testStringArray, + Name: "string-array-short", + ShortHand: "a", + Usage: "a string array flag (short)", + }, + cmd: parentCmd, + }, { desc: "int flag", flag: &Flag{ From 7e8a50432bb39d90705e282e538b9c2a2ed2367e Mon Sep 17 00:00:00 2001 From: Edita Kizinevic Date: Thu, 4 Nov 2021 15:48:16 +0100 Subject: [PATCH 04/10] Update DefaultValue --- cmd/internal/cli/action_flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/internal/cli/action_flags.go b/cmd/internal/cli/action_flags.go index e6f4830a81..5ba7bff509 100644 --- a/cmd/internal/cli/action_flags.go +++ b/cmd/internal/cli/action_flags.go @@ -83,7 +83,7 @@ var actionAppFlag = cmdline.Flag{ var actionBindFlag = cmdline.Flag{ ID: "actionBindFlag", Value: &BindPaths, - DefaultValue: cmdline.StringArray{}, // to allow commas in bind path + DefaultValue: []string{}, Name: "bind", ShortHand: "B", Usage: "a user-bind path specification. spec has the format src[:dest[:opts]], where src and dest are outside and inside paths. If dest is not given, it is set equal to src. Mount options ('opts') may be specified as 'ro' (read-only) or 'rw' (read/write, which is the default). Multiple bind paths can be given by a comma separated list.", From 5177f7214d55325e30ae57061bdc2eaf50937eb8 Mon Sep 17 00:00:00 2001 From: Edita Kizinevic Date: Thu, 4 Nov 2021 15:49:23 +0100 Subject: [PATCH 05/10] Use color instead of colour --- pkg/runtime/engine/singularity/config/mount_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/runtime/engine/singularity/config/mount_test.go b/pkg/runtime/engine/singularity/config/mount_test.go index a5da84d05c..ccd69259f8 100644 --- a/pkg/runtime/engine/singularity/config/mount_test.go +++ b/pkg/runtime/engine/singularity/config/mount_test.go @@ -49,7 +49,7 @@ func TestParseMountString(t *testing.T) { }, { name: "invalidField", - mountString: "type=bind,source=/opt,destination=/opt,colour=turquoise", + mountString: "type=bind,source=/opt,destination=/opt,color=turquoise", want: BindPath{}, wantErr: true, }, From f77a1d46fb187b335b1bc6469b431dcc1822efcb Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 13:25:05 -0500 Subject: [PATCH 06/10] Handle multiple newline delimited mounts in ParseMountString --- .../engine/singularity/config/mount.go | 112 ++++++------- .../engine/singularity/config/mount_test.go | 151 +++++++++++------- 2 files changed, 151 insertions(+), 112 deletions(-) diff --git a/pkg/runtime/engine/singularity/config/mount.go b/pkg/runtime/engine/singularity/config/mount.go index 0791773ebe..f7c025ec04 100644 --- a/pkg/runtime/engine/singularity/config/mount.go +++ b/pkg/runtime/engine/singularity/config/mount.go @@ -11,84 +11,86 @@ import ( "strings" ) -// Parse a --mount string into a BindPath struct. +// ParseMountString converts a --mount string into one or more BindPath structs. // // Our intention is to support common docker --mount strings, but have // additional fields for singularity specific concepts (image-src, id when -// binding out of a SIF partition). +// binding out of an image file). // -// We use a CSV reader to parsing the fields in a mount string according to CSV +// We use a CSV reader to parse the fields in a mount string according to CSV // escaping rules. This is the approach docker uses to allow special characters -// in source / dest etc., and we wish to be as compatible as possible. +// in source / dest etc., and we wish to be as compatible as possible. It also +// allows us to handle multiple newline separated mounts, which is convenient +// for specifying multiple mounts in a single env var. // // The fields are in key[=value] format. Flag options have no value, e.g.: // type=bind,source=/opt,destination=/other,rw // // We only support type=bind at present, so assume this if type is missing and // error for other types. -func ParseMountString(mount string) (bp BindPath, err error) { +func ParseMountString(mount string) (bindPaths []BindPath, err error) { r := strings.NewReader(mount) c := csv.NewReader(r) records, err := c.ReadAll() if err != nil { - return BindPath{}, fmt.Errorf("error parsing mount: %v", err) - } - if len(records) != 1 { - return BindPath{}, fmt.Errorf("each mount string must specify exactly one mount") - } - - bp = BindPath{ - Options: map[string]*BindOption{}, + return []BindPath{}, fmt.Errorf("error parsing mount: %v", err) } - for _, f := range records[0] { - kv := strings.SplitN(f, "=", 2) - key := kv[0] - val := "" - if len(kv) > 1 { - val = kv[1] + for _, r := range records { + bp := BindPath{ + Options: map[string]*BindOption{}, } - switch key { - // TODO - Eventually support volume and tmpfs? Requires structural changes to engine mount functionality. - case "type": - if val != "bind" { - return BindPath{}, fmt.Errorf("unsupported mount type %q, only 'bind' is supported", val) - } - case "source", "src": - if val == "" { - return BindPath{}, fmt.Errorf("mount source cannot be empty") - } - bp.Source = val - case "destination", "dst", "target": - if val == "" { - return BindPath{}, fmt.Errorf("mount destination cannot be empty") + for _, f := range r { + kv := strings.SplitN(f, "=", 2) + key := kv[0] + val := "" + if len(kv) > 1 { + val = kv[1] } - bp.Destination = val - case "ro", "readonly": - bp.Options["ro"] = &BindOption{} - // Singularity only - directory inside an image file source to mount from - case "image-src": - if val == "" { - return BindPath{}, fmt.Errorf("img-src cannot be empty") - } - bp.Options["image-src"] = &BindOption{Value: val} - // Singularity only - id of the descriptor in a SIF image source to mount from - case "id": - if val == "" { - return BindPath{}, fmt.Errorf("id cannot be empty") + + switch key { + // TODO - Eventually support volume and tmpfs? Requires structural changes to engine mount functionality. + case "type": + if val != "bind" { + return []BindPath{}, fmt.Errorf("unsupported mount type %q, only 'bind' is supported", val) + } + case "source", "src": + if val == "" { + return []BindPath{}, fmt.Errorf("mount source cannot be empty") + } + bp.Source = val + case "destination", "dst", "target": + if val == "" { + return []BindPath{}, fmt.Errorf("mount destination cannot be empty") + } + bp.Destination = val + case "ro", "readonly": + bp.Options["ro"] = &BindOption{} + // Singularity only - directory inside an image file source to mount from + case "image-src": + if val == "" { + return []BindPath{}, fmt.Errorf("img-src cannot be empty") + } + bp.Options["image-src"] = &BindOption{Value: val} + // Singularity only - id of the descriptor in a SIF image source to mount from + case "id": + if val == "" { + return []BindPath{}, fmt.Errorf("id cannot be empty") + } + bp.Options["id"] = &BindOption{Value: val} + case "bind-propagation": + return []BindPath{}, fmt.Errorf("bind-propagation not supported for individual mounts, check singularity.conf for global setting") + default: + return []BindPath{}, fmt.Errorf("invalid key %q in mount specification", key) } - bp.Options["id"] = &BindOption{Value: val} - case "bind-propagation": - return BindPath{}, fmt.Errorf("bind-propagation not supported for individual mounts, check singularity.conf for global setting") - default: - return BindPath{}, fmt.Errorf("invalid key %q in mount specification", key) } - } - if bp.Source == "" || bp.Destination == "" { - return BindPath{}, fmt.Errorf("mounts must specify a source and a destination") + if bp.Source == "" || bp.Destination == "" { + return []BindPath{}, fmt.Errorf("mounts must specify a source and a destination") + } + bindPaths = append(bindPaths, bp) } - return bp, nil + return bindPaths, nil } diff --git a/pkg/runtime/engine/singularity/config/mount_test.go b/pkg/runtime/engine/singularity/config/mount_test.go index ccd69259f8..5c0e0b82fc 100644 --- a/pkg/runtime/engine/singularity/config/mount_test.go +++ b/pkg/runtime/engine/singularity/config/mount_test.go @@ -14,103 +14,115 @@ func TestParseMountString(t *testing.T) { tests := []struct { name string mountString string - want BindPath + want []BindPath wantErr bool }{ { name: "sourceOnly", mountString: "type=bind,source=/opt", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "destinationOnly", mountString: "type=bind,destination=/opt", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "emptySource", mountString: "type=bind,source=,destination=/opt", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "emptyDestination", mountString: "type=bind,source=/opt,destination=", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "invalidType", mountString: "type=potato,source=/opt,destination=/opt", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "invalidField", mountString: "type=bind,source=/opt,destination=/opt,color=turquoise", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "simple", mountString: "type=bind,source=/opt,destination=/opt", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, { name: "simpleSrc", mountString: "type=bind,src=/opt,destination=/opt", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, { name: "simpleDst", mountString: "type=bind,source=/opt,dst=/opt", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, { name: "simpleTarget", mountString: "type=bind,source=/opt,target=/opt", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, { name: "noType", mountString: "source=/opt,destination=/opt", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, { name: "ro", mountString: "type=bind,source=/opt,destination=/opt,ro", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{ - "ro": {}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{ + "ro": {}, + }, }, }, wantErr: false, @@ -118,11 +130,13 @@ func TestParseMountString(t *testing.T) { { name: "readonly", mountString: "type=bind,source=/opt,destination=/opt,readonly", - want: BindPath{ - Source: "/opt", - Destination: "/opt", - Options: map[string]*BindOption{ - "ro": {}, + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{ + "ro": {}, + }, }, }, wantErr: false, @@ -130,11 +144,13 @@ func TestParseMountString(t *testing.T) { { name: "imagesrc", mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt", - want: BindPath{ - Source: "test.sif", - Destination: "/opt", - Options: map[string]*BindOption{ - "image-src": {Value: "/opt"}, + want: []BindPath{ + { + Source: "test.sif", + Destination: "/opt", + Options: map[string]*BindOption{ + "image-src": {Value: "/opt"}, + }, }, }, wantErr: false, @@ -142,24 +158,26 @@ func TestParseMountString(t *testing.T) { { name: "imagesrcNoValue", mountString: "type=bind,source=test.sif,destination=/opt,image-src", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "imagesrcEmpty", mountString: "type=bind,source=test.sif,destination=/opt,image-src=", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "id", mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id=2", - want: BindPath{ - Source: "test.sif", - Destination: "/opt", - Options: map[string]*BindOption{ - "image-src": {Value: "/opt"}, - "id": {Value: "2"}, + want: []BindPath{ + { + Source: "test.sif", + Destination: "/opt", + Options: map[string]*BindOption{ + "image-src": {Value: "/opt"}, + "id": {Value: "2"}, + }, }, }, wantErr: false, @@ -167,28 +185,47 @@ func TestParseMountString(t *testing.T) { { name: "idNoValue", mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "idEmpty", mountString: "type=bind,source=test.sif,destination=/opt,image-src=/opt,id=", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "bindpropagation", mountString: "type=bind,source=/opt,destination=/opt,bind-propagation=shared", - want: BindPath{}, + want: []BindPath{}, wantErr: true, }, { name: "csvEscaped", mountString: `type=bind,"source=/comma,dir","destination=/quote""dir"`, - want: BindPath{ - Source: "/comma,dir", - Destination: "/quote\"dir", - Options: map[string]*BindOption{}, + want: []BindPath{ + { + Source: "/comma,dir", + Destination: "/quote\"dir", + Options: map[string]*BindOption{}, + }, + }, + wantErr: false, + }, + { + name: "multiple", + mountString: "type=bind,source=/opt,destination=/opt\ntype=bind,source=/srv,destination=/srv", + want: []BindPath{ + { + Source: "/opt", + Destination: "/opt", + Options: map[string]*BindOption{}, + }, + { + Source: "/srv", + Destination: "/srv", + Options: map[string]*BindOption{}, + }, }, wantErr: false, }, From da85ca0354cac2ee7d7d29ec41cf042b78708b07 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 13:28:04 -0500 Subject: [PATCH 07/10] Add `--mount` flag for long form mount specifications --- cmd/internal/cli/action_flags.go | 15 +++++++++++++++ cmd/internal/cli/actions_linux.go | 11 +++++++++++ 2 files changed, 26 insertions(+) diff --git a/cmd/internal/cli/action_flags.go b/cmd/internal/cli/action_flags.go index 5ba7bff509..fee88271bf 100644 --- a/cmd/internal/cli/action_flags.go +++ b/cmd/internal/cli/action_flags.go @@ -15,6 +15,7 @@ import ( var ( AppName string BindPaths []string + Mounts []string HomePath string OverlayPath []string ScratchPath []string @@ -92,6 +93,19 @@ var actionBindFlag = cmdline.Flag{ EnvHandler: cmdline.EnvAppendValue, } +// --mount +var actionMountFlag = cmdline.Flag{ + ID: "actionMountFlag", + Value: &Mounts, + DefaultValue: []string{}, + Name: "mount", + Usage: "a mount specification e.g. 'type=bind,source=/opt,destination=/hostopt'.", + EnvKeys: []string{"MOUNT"}, + Tag: "", + EnvHandler: cmdline.EnvAppendValue, + StringArray: true, +} + // -H|--home var actionHomeFlag = cmdline.Flag{ ID: "actionHomeFlag", @@ -656,6 +670,7 @@ func init() { cmdManager.RegisterFlagForCmd(&actionHostnameFlag, actionsInstanceCmd...) cmdManager.RegisterFlagForCmd(&actionIpcNamespaceFlag, actionsInstanceCmd...) cmdManager.RegisterFlagForCmd(&actionKeepPrivsFlag, actionsInstanceCmd...) + cmdManager.RegisterFlagForCmd(&actionMountFlag, actionsInstanceCmd...) cmdManager.RegisterFlagForCmd(&actionNetNamespaceFlag, actionsInstanceCmd...) cmdManager.RegisterFlagForCmd(&actionNetworkArgsFlag, actionsInstanceCmd...) cmdManager.RegisterFlagForCmd(&actionNetworkFlag, actionsInstanceCmd...) diff --git a/cmd/internal/cli/actions_linux.go b/cmd/internal/cli/actions_linux.go index 4ebcc8d92a..a5cae903a9 100644 --- a/cmd/internal/cli/actions_linux.go +++ b/cmd/internal/cli/actions_linux.go @@ -411,10 +411,21 @@ func execStarter(cobraCmd *cobra.Command, image string, args []string, name stri img.File.Close() } + // First get binds from -B/--bind and env var binds, err := singularityConfig.ParseBindPath(BindPaths) if err != nil { sylog.Fatalf("while parsing bind path: %s", err) } + + // Now add binds from one or more --mount and env var. + for _, m := range Mounts { + bps, err := singularityConfig.ParseMountString(m) + if err != nil { + sylog.Fatalf("while parsing mount %q: %s", m, err) + } + binds = append(binds, bps...) + } + engineConfig.SetBindPath(binds) generator.AddProcessEnv("SINGULARITY_BIND", strings.Join(BindPaths, ",")) From 17a769314754e2c5fe33b5162eb1460e309606e3 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 14:12:09 -0500 Subject: [PATCH 08/10] Add --mount flag to build command --- cmd/internal/cli/build.go | 15 +++++++++++++++ cmd/internal/cli/build_linux.go | 6 ++++++ internal/pkg/build/stage.go | 4 ++-- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/cmd/internal/cli/build.go b/cmd/internal/cli/build.go index 73643211b8..af79122923 100644 --- a/cmd/internal/cli/build.go +++ b/cmd/internal/cli/build.go @@ -31,6 +31,7 @@ import ( var buildArgs struct { sections []string bindPaths []string + mounts []string arch string builderURL string libraryURL string @@ -240,6 +241,19 @@ var buildBindFlag = cmdline.Flag{ "Multiple bind paths can be given by a comma separated list. (not supported with remote build)", } +// --mount +var buildMountFlag = cmdline.Flag{ + ID: "buildMountFlag", + Value: &buildArgs.mounts, + DefaultValue: []string{}, + Name: "mount", + Usage: "a mount specification e.g. 'type=bind,source=/opt,destination=/hostopt'.", + EnvKeys: []string{"MOUNT"}, + Tag: "", + EnvHandler: cmdline.EnvAppendValue, + StringArray: true, +} + // --writable-tmpfs var buildWritableTmpfsFlag = cmdline.Flag{ ID: "buildWritableTmpfsFlag", @@ -283,6 +297,7 @@ func init() { cmdManager.RegisterFlagForCmd(&buildNvFlag, buildCmd) cmdManager.RegisterFlagForCmd(&buildRocmFlag, buildCmd) cmdManager.RegisterFlagForCmd(&buildBindFlag, buildCmd) + cmdManager.RegisterFlagForCmd(&buildMountFlag, buildCmd) cmdManager.RegisterFlagForCmd(&buildWritableTmpfsFlag, buildCmd) }) } diff --git a/cmd/internal/cli/build_linux.go b/cmd/internal/cli/build_linux.go index 1342957362..f9bb5001a9 100644 --- a/cmd/internal/cli/build_linux.go +++ b/cmd/internal/cli/build_linux.go @@ -115,6 +115,12 @@ func runBuild(cmd *cobra.Command, args []string) { } os.Setenv("SINGULARITY_BINDPATH", strings.Join(buildArgs.bindPaths, ",")) } + if len(buildArgs.mounts) > 0 { + if buildArgs.remote { + sylog.Fatalf("--mount option is not supported for remote build") + } + os.Setenv("SINGULARITY_MOUNT", strings.Join(buildArgs.mounts, "\n")) + } if buildArgs.writableTmpfs { if buildArgs.remote { sylog.Fatalf("--writable-tmpfs option is not supported for remote build") diff --git a/internal/pkg/build/stage.go b/internal/pkg/build/stage.go index a5757a3a1d..4ce4c3533d 100644 --- a/internal/pkg/build/stage.go +++ b/internal/pkg/build/stage.go @@ -109,7 +109,7 @@ func (s *stage) runPostScript(configFile, sessionResolv, sessionHosts string) er cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Dir = "/" - cmd.Env = currentEnvNoSingularity([]string{"NV", "ROCM", "BINDPATH"}) + cmd.Env = currentEnvNoSingularity([]string{"NV", "ROCM", "BINDPATH", "MOUNT"}) sylog.Infof("Running post scriptlet") return cmd.Run() @@ -135,7 +135,7 @@ func (s *stage) runTestScript(configFile, sessionResolv, sessionHosts string) er cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr cmd.Dir = "/" - cmd.Env = currentEnvNoSingularity([]string{"NV", "ROCM", "BINDPATH", "WRITABLE_TMPFS"}) + cmd.Env = currentEnvNoSingularity([]string{"NV", "ROCM", "BINDPATH", "MOUNT", "WRITABLE_TMPFS"}) sylog.Infof("Running testscript") return cmd.Run() From b8d9b25a8552419f6b9d7b16c34add76a388e8bc Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 14:15:36 -0500 Subject: [PATCH 09/10] e2e: add build test cases for `--mount` flag --- e2e/imgbuild/imgbuild.go | 57 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/e2e/imgbuild/imgbuild.go b/e2e/imgbuild/imgbuild.go index 1a0eda8686..fd9b98d89b 100644 --- a/e2e/imgbuild/imgbuild.go +++ b/e2e/imgbuild/imgbuild.go @@ -1333,6 +1333,63 @@ func (c imgBuildTests) buildBindMount(t *testing.T) { }, exit: 255, }, + { + name: "Mount test dir to /mnt", + buildOption: []string{ + "--mount", "type=bind,source=" + dir + ",destination=/mnt", + }, + buildPost: []string{ + "cat /mnt/canary", + }, + buildTest: []string{ + "cat /mnt/canary", + }, + exit: 0, + }, + { + name: "Mount test dir to multiple directory", + buildOption: []string{ + "--mount", "type=bind,source=" + dir + ",destination=/mnt", + "--mount", "type=bind,source=" + dir + ",destination=/opt", + }, + buildPost: []string{ + "cat /mnt/canary", + "cat /opt/canary", + }, + buildTest: []string{ + "cat /mnt/canary", + "cat /opt/canary", + }, + exit: 0, + }, + { + name: "Mount test dir to /mnt read-only", + buildOption: []string{ + "--mount", "type=bind,source=" + dir + ",destination=/mnt,ro", + }, + buildPost: []string{ + "mkdir /mnt/should_fail", + }, + exit: 255, + }, + { + name: "Mount test dir to non-existent image directory", + buildOption: []string{ + "--mount", "type=bind,source=" + dir + ",destination=/fake/dir", + }, + buildPost: []string{ + "cat /mnt/canary", + }, + exit: 255, + }, + { + name: "Mount test dir with remote", + buildOption: []string{ + "--mount", "type=bind,source=" + dir + ",destination=/mnt", + "--remote", + }, + exit: 255, + }, } sandboxImage := filepath.Join(tmpdir, "build-sandbox") From 28c754c07e5b72d59889625b3b22d2ad693800e5 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 10 Sep 2021 14:36:28 -0500 Subject: [PATCH 10/10] e2e: add actions test cases for `--mount` flag --- e2e/actions/actions.go | 58 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/e2e/actions/actions.go b/e2e/actions/actions.go index c8f55b9d33..f6c5be3c31 100644 --- a/e2e/actions/actions.go +++ b/e2e/actions/actions.go @@ -1070,7 +1070,9 @@ func (c actionTests) actionBinds(t *testing.T) { hostCanaryFileWithColon := filepath.Join(hostCanaryDir, "file:colon") canaryFileBind := hostCanaryFile + ":" + contCanaryFile + canaryFileMount := "type=bind,source=" + hostCanaryFile + ",destination=" + contCanaryFile canaryDirBind := hostCanaryDir + ":" + contCanaryDir + canaryDirMount := "type=bind,source=" + hostCanaryDir + ",destination=" + contCanaryDir hostHomeDir := filepath.Join(workspace, "home") hostWorkDir := filepath.Join(workspace, "workdir") @@ -1512,6 +1514,31 @@ func (c actionTests) actionBinds(t *testing.T) { }, exit: 255, }, + // For the --mount variants we are really just verifying the CLI + // acceptance of one or more --mount flags. Translation from --mount + // strings to BindPath structs is checked in unit tests. The + // functionality of bind mounts of various kinds is already checked + // above, with --bind flags. No need to duplicate all of these. + { + name: "MountSingle", + args: []string{ + "--mount", canaryFileMount, + sandbox, + "test", "-f", contCanaryFile, + }, + exit: 0, + }, + { + name: "MountNested", + args: []string{ + "--mount", canaryDirMount, + "--mount", "source=" + hostCanaryFile + ",destination=" + filepath.Join(contCanaryDir, "file3"), + sandbox, + "test", "-f", "/canary/file3", + }, + postRun: checkHostFile(filepath.Join(hostCanaryDir, "file3")), + exit: 0, + }, } for _, profile := range e2e.Profiles { @@ -2071,6 +2098,33 @@ func (c actionTests) bindImage(t *testing.T) { }, exit: 0, }, + // For the --mount variants we are really just verifying the CLI + // acceptance of one or more image bind mount strings. Translation from + // --mount strings to BindPath structs is checked in unit tests. The + // functionality of image mounts of various kinds is already checked + // above, with --bind flags. No need to duplicate all of these. + { + name: "MountSifWithID", + profile: e2e.UserProfile, + args: []string{ + // rootfs ID is now '4' + "--mount", "type=bind,source=" + c.env.ImagePath + ",destination=/rootfs,id=4", + c.env.ImagePath, + "test", "-d", "/rootfs/etc", + }, + exit: 0, + }, + { + name: "MountSifDataExt3AndSquash", + profile: e2e.UserProfile, + args: []string{ + "--mount", "type=bind,source=" + sifExt3Image + ",destination=/ext3,image-src=/", + "--mount", "type=bind,source=" + sifSquashImage + ",destination=/squash,image-src=/", + c.env.ImagePath, + "test", "-f", filepath.Join("/squash", squashMarkerFile), "-a", "-f", "/ext3/ext3_marker", + }, + exit: 0, + }, } for _, tt := range tests { @@ -2327,10 +2381,10 @@ func E2ETests(env e2e.TestEnv) testhelper.Tests { "issue 5690": c.issue5690, // https://github.com/hpcng/singularity/issues/5690 "issue 6165": c.issue6165, // https://github.com/hpcng/singularity/issues/6165 "network": c.actionNetwork, // test basic networking - "binds": c.actionBinds, // test various binds + "binds": c.actionBinds, // test various binds with --bind and --mount "exit and signals": c.exitSignals, // test exit and signals propagation "fuse mount": c.fuseMount, // test fusemount option - "bind image": c.bindImage, // test bind image + "bind image": c.bindImage, // test bind image with --bind and --mount "umask": c.actionUmask, // test umask propagation "no-mount": c.actionNoMount, // test --no-mount "compat": c.actionCompat, // test --compat