Make all custom flag.Value implementations idempotent.

Set(String()) should be an idempotent operation. This is a useful property
which allows us to generate args while re-execing the same process. Setting
`--flag-name=val.String()` should work.

PiperOrigin-RevId: 552598313
This commit is contained in:
Ayush Ranjan
2023-07-31 14:53:21 -07:00
committed by gVisor bot
parent ef95be6e1c
commit 7981df85f3
12 changed files with 297 additions and 162 deletions
+6 -12
View File
@@ -57,7 +57,11 @@ type OverlayMediumFlags []OverlayMedium
// String implements flag.Value.
func (o *OverlayMediumFlags) String() string {
return fmt.Sprintf("%v", *o)
mediumVals := make([]string, 0, len(*o))
for _, medium := range *o {
mediumVals = append(mediumVals, strconv.Itoa(int(medium)))
}
return strings.Join(mediumVals, ",")
}
// Get implements flag.Value.
@@ -71,7 +75,7 @@ func (o *OverlayMediumFlags) GetArray() []OverlayMedium {
}
// Set implements flag.Value and appends an overlay medium from the command
// line to the mediums array.
// line to the mediums array. Set(String()) should be idempotent.
func (o *OverlayMediumFlags) Set(s string) error {
mediums := strings.Split(s, ",")
for _, medium := range mediums {
@@ -86,13 +90,3 @@ func (o *OverlayMediumFlags) Set(s string) error {
}
return nil
}
// ToOverlayMediumFlags converts []OverlayMedium to string format which can be
// unpacked by OverlayMediumFlags.Set().
func ToOverlayMediumFlags(mediums []OverlayMedium) string {
mediumVals := make([]string, 0, len(mediums))
for _, medium := range mediums {
mediumVals = append(mediumVals, strconv.Itoa(int(medium)))
}
return strings.Join(mediumVals, ",")
}
+2 -2
View File
@@ -51,9 +51,9 @@ func TestOverlayMedium(t *testing.T) {
}
func TestOverlayMediumFlags(t *testing.T) {
want := []OverlayMedium{MemoryMedium, SelfMedium, AnonDirMedium, NoOverlay}
want := OverlayMediumFlags{MemoryMedium, SelfMedium, AnonDirMedium, NoOverlay}
var got OverlayMediumFlags
got.Set(ToOverlayMediumFlags(want))
got.Set(want.String())
if len(got) != len(want) {
t.Fatalf("overlay medium flags is incorrect length: want = %d, got = %d", len(want), len(got))
}
+11 -1
View File
@@ -1,4 +1,4 @@
load("//tools:defs.bzl", "go_library")
load("//tools:defs.bzl", "go_library", "go_test")
package(
default_applicable_licenses = ["//:license"],
@@ -29,3 +29,13 @@ go_library(
"@org_golang_x_sys//unix:go_default_library",
],
)
go_test(
name = "cli_test",
srcs = ["cli_test.go"],
library = ":cli",
deps = [
"//runsc/flag",
"@com_github_google_subcommands//:go_default_library",
],
)
+103
View File
@@ -0,0 +1,103 @@
// Copyright 2023 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package cli
import (
"reflect"
"testing"
"github.com/google/subcommands"
"gvisor.dev/gvisor/runsc/flag"
)
var fakeFlagValues = [...]string{
"1",
"2:2",
"foo",
"none",
"1,2,3",
"2h45m",
"1:1,2:2",
"0 0 1,100000 100000 65536",
}
func dupFlag(t *testing.T, cmd subcommands.Command, flagName string) *flag.Flag {
// To create a true duplicate of the flag, we need to duplicate the command
// and its FlagSet.
var cmd2 subcommands.Command
var fs2 flag.FlagSet
cmd2 = reflect.New(reflect.TypeOf(cmd).Elem()).Interface().(subcommands.Command)
cmd2.SetFlags(&fs2)
flag2 := fs2.Lookup(flagName)
if flag2 == nil {
t.Fatalf("duplicate FlagSet does not contain flag %q for cmd %q", flagName, cmd.Name())
}
return flag2
}
// Tests that all the flags in all commands are idempotent; i.e. Set(String())
// should be an idempotent operation.
func TestFlagSetIdempotent(t *testing.T) {
cmds := make(map[string][]subcommands.Command)
forEachCmd(func(cmd subcommands.Command, group string) {
if cmdList, ok := cmds[group]; ok {
cmds[group] = append(cmdList, cmd)
} else {
cmds[group] = []subcommands.Command{cmd}
}
})
for group, cmdList := range cmds {
t.Run(group, func(t *testing.T) {
for _, cmd := range cmdList {
t.Run(cmd.Name(), func(t *testing.T) {
var fs flag.FlagSet
cmd.SetFlags(&fs)
// Iterate through all flags configured by this command.
fs.VisitAll(func(flag *flag.Flag) {
// Try a list of possible values for this flag.
matchedOneFlag := false
for _, v := range fakeFlagValues {
// Set() may have side effects even when it fails. So create a new
// flag for each try.
curFlag := dupFlag(t, cmd, flag.Name)
if err := curFlag.Value.Set(v); err != nil {
continue
}
// Worked. Now test that this flag is idempotent.
oldValue := curFlag.Value.String()
// Get a fresh flag.Flag to Set() this old value on.
newFlag := dupFlag(t, cmd, flag.Name)
if err := newFlag.Value.Set(oldValue); err != nil {
t.Errorf("flag %q from cmd %q is not idempotent: oldValue = %q, err = %v", flag.Name, cmd.Name(), oldValue, err)
return
}
// Compare this new flag value with old value.
if newValue := newFlag.Value.String(); newValue != oldValue {
t.Errorf("flag %q from cmd %q is not idempotent: oldValue = %q, newValue = %q", flag.Name, cmd.Name(), oldValue, newValue)
return
}
matchedOneFlag = true
}
if !matchedOneFlag {
t.Fatalf("none of the fake flag values work for flag %q from cmd %q", flag.Name, cmd.Name())
}
})
})
}
})
}
}
+58 -52
View File
@@ -59,58 +59,8 @@ var (
// Main is the main entrypoint.
func Main() {
// Help and flags commands are generated automatically.
help := cmd.NewHelp(subcommands.DefaultCommander)
help.Register(new(cmd.Platforms))
help.Register(new(cmd.Syscalls))
subcommands.Register(help, "")
subcommands.Register(subcommands.FlagsCommand(), "")
// Register OCI user-facing runsc commands.
subcommands.Register(new(cmd.Checkpoint), "")
subcommands.Register(new(cmd.Create), "")
subcommands.Register(new(cmd.Delete), "")
subcommands.Register(new(cmd.Do), "")
subcommands.Register(new(cmd.Events), "")
subcommands.Register(new(cmd.Exec), "")
subcommands.Register(new(cmd.Kill), "")
subcommands.Register(new(cmd.List), "")
subcommands.Register(new(cmd.PS), "")
subcommands.Register(new(cmd.Pause), "")
subcommands.Register(new(cmd.PortForward), "")
subcommands.Register(new(cmd.Restore), "")
subcommands.Register(new(cmd.Resume), "")
subcommands.Register(new(cmd.Run), "")
subcommands.Register(new(cmd.Spec), "")
subcommands.Register(new(cmd.Start), "")
subcommands.Register(new(cmd.State), "")
subcommands.Register(new(cmd.Wait), "")
// Helpers.
const helperGroup = "helpers"
subcommands.Register(new(cmd.Install), helperGroup)
subcommands.Register(new(cmd.Mitigate), helperGroup)
subcommands.Register(new(cmd.Uninstall), helperGroup)
subcommands.Register(new(trace.Trace), helperGroup)
const debugGroup = "debug"
subcommands.Register(new(cmd.Debug), debugGroup)
subcommands.Register(new(cmd.Statefile), debugGroup)
subcommands.Register(new(cmd.Symbolize), debugGroup)
subcommands.Register(new(cmd.Usage), debugGroup)
subcommands.Register(new(cmd.ReadControl), debugGroup)
subcommands.Register(new(cmd.WriteControl), debugGroup)
const metricGroup = "metrics"
subcommands.Register(new(cmd.MetricMetadata), metricGroup)
subcommands.Register(new(cmd.MetricExport), metricGroup)
subcommands.Register(new(cmd.MetricServer), metricGroup)
// Internal commands.
const internalGroup = "internal use only"
subcommands.Register(new(cmd.Boot), internalGroup)
subcommands.Register(new(cmd.Gofer), internalGroup)
subcommands.Register(new(cmd.Umount), internalGroup)
// Register all commands.
forEachCmd(subcommands.Register)
// Register with the main command line.
config.RegisterFlags(flag.CommandLine)
@@ -275,6 +225,62 @@ func Main() {
os.Exit(128)
}
// forEachCmd invokes the passed callback for each command supported by runsc.
func forEachCmd(cb func(cmd subcommands.Command, group string)) {
// Help and flags commands are generated automatically.
help := cmd.NewHelp(subcommands.DefaultCommander)
help.Register(new(cmd.Platforms))
help.Register(new(cmd.Syscalls))
cb(help, "")
cb(subcommands.FlagsCommand(), "")
// Register OCI user-facing runsc commands.
cb(new(cmd.Checkpoint), "")
cb(new(cmd.Create), "")
cb(new(cmd.Delete), "")
cb(new(cmd.Do), "")
cb(new(cmd.Events), "")
cb(new(cmd.Exec), "")
cb(new(cmd.Kill), "")
cb(new(cmd.List), "")
cb(new(cmd.PS), "")
cb(new(cmd.Pause), "")
cb(new(cmd.PortForward), "")
cb(new(cmd.Restore), "")
cb(new(cmd.Resume), "")
cb(new(cmd.Run), "")
cb(new(cmd.Spec), "")
cb(new(cmd.Start), "")
cb(new(cmd.State), "")
cb(new(cmd.Wait), "")
// Helpers.
const helperGroup = "helpers"
cb(new(cmd.Install), helperGroup)
cb(new(cmd.Mitigate), helperGroup)
cb(new(cmd.Uninstall), helperGroup)
cb(new(trace.Trace), helperGroup)
const debugGroup = "debug"
cb(new(cmd.Debug), debugGroup)
cb(new(cmd.Statefile), debugGroup)
cb(new(cmd.Symbolize), debugGroup)
cb(new(cmd.Usage), debugGroup)
cb(new(cmd.ReadControl), debugGroup)
cb(new(cmd.WriteControl), debugGroup)
const metricGroup = "metrics"
cb(new(cmd.MetricMetadata), metricGroup)
cb(new(cmd.MetricExport), metricGroup)
cb(new(cmd.MetricServer), metricGroup)
// Internal commands.
const internalGroup = "internal use only"
cb(new(cmd.Boot), internalGroup)
cb(new(cmd.Gofer), internalGroup)
cb(new(cmd.Umount), internalGroup)
}
func newEmitter(format string, logFile io.Writer) log.Emitter {
switch format {
case "text":
+18 -10
View File
@@ -20,6 +20,7 @@ import (
"os"
"runtime"
"strconv"
"strings"
specs "github.com/opencontainers/runtime-spec/specs-go"
"golang.org/x/sys/unix"
@@ -27,12 +28,17 @@ import (
"gvisor.dev/gvisor/runsc/specutils"
)
// intFlags can be used with int flags that appear multiple times.
// intFlags can be used with int flags that appear multiple times. It supports
// comma-separated lists too.
type intFlags []int
// String implements flag.Value.
func (i *intFlags) String() string {
return fmt.Sprintf("%v", *i)
sInts := make([]string, 0, len(*i))
for _, fd := range *i {
sInts = append(sInts, strconv.Itoa(fd))
}
return strings.Join(sInts, ",")
}
// Get implements flag.Value.
@@ -45,16 +51,18 @@ func (i *intFlags) GetArray() []int {
return *i
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (i *intFlags) Set(s string) error {
fd, err := strconv.Atoi(s)
if err != nil {
return fmt.Errorf("invalid flag value: %v", err)
for _, sFD := range strings.Split(s, ",") {
fd, err := strconv.Atoi(sFD)
if err != nil {
return fmt.Errorf("invalid flag value: %v", err)
}
if fd < -1 {
return fmt.Errorf("flag value must be >= -1: %d", fd)
}
*i = append(*i, fd)
}
if fd < -1 {
return fmt.Errorf("flag value must be >= -1: %d", fd)
}
*i = append(*i, fd)
return nil
}
+28 -22
View File
@@ -79,7 +79,11 @@ type idMapSlice []specs.LinuxIDMapping
// String implements flag.Value.String.
func (is *idMapSlice) String() string {
return fmt.Sprintf("%#v", is)
idMappings := make([]string, 0, len(*is))
for _, m := range *is {
idMappings = append(idMappings, fmt.Sprintf("%d %d %d", m.ContainerID, m.HostID, m.Size))
}
return strings.Join(idMappings, ",")
}
// Get implements flag.Value.Get.
@@ -87,29 +91,31 @@ func (is *idMapSlice) Get() any {
return is
}
// Set implements flag.Value.Set.
// Set implements flag.Value.Set. Set(String()) should be idempotent.
func (is *idMapSlice) Set(s string) error {
fs := strings.Fields(s)
if len(fs) != 3 {
return fmt.Errorf("invalid mapping: %s", s)
for _, idMap := range strings.Split(s, ",") {
fs := strings.Fields(idMap)
if len(fs) != 3 {
return fmt.Errorf("invalid mapping: %s", idMap)
}
var cid, hid, size int
var err error
if cid, err = strconv.Atoi(fs[0]); err != nil {
return fmt.Errorf("invalid mapping: %s", idMap)
}
if hid, err = strconv.Atoi(fs[1]); err != nil {
return fmt.Errorf("invalid mapping: %s", idMap)
}
if size, err = strconv.Atoi(fs[2]); err != nil {
return fmt.Errorf("invalid mapping: %s", idMap)
}
m := specs.LinuxIDMapping{
ContainerID: uint32(cid),
HostID: uint32(hid),
Size: uint32(size),
}
*is = append(*is, m)
}
var cid, hid, size int
var err error
if cid, err = strconv.Atoi(fs[0]); err != nil {
return fmt.Errorf("invalid mapping: %s", s)
}
if hid, err = strconv.Atoi(fs[1]); err != nil {
return fmt.Errorf("invalid mapping: %s", s)
}
if size, err = strconv.Atoi(fs[2]); err != nil {
return fmt.Errorf("invalid mapping: %s", s)
}
m := specs.LinuxIDMapping{
ContainerID: uint32(cid),
HostID: uint32(hid),
Size: uint32(size),
}
*is = append(*is, m)
return nil
}
+7 -4
View File
@@ -463,7 +463,7 @@ type stringSlice []string
// String implements flag.Value.String.
func (ss *stringSlice) String() string {
return fmt.Sprintf("%v", *ss)
return strings.Join(*ss, ",")
}
// Get implements flag.Value.Get.
@@ -471,9 +471,9 @@ func (ss *stringSlice) Get() any {
return ss
}
// Set implements flag.Value.Set.
// Set implements flag.Value.Set. Set(String()) should be idempotent.
func (ss *stringSlice) Set(s string) error {
*ss = append(*ss, s)
*ss = append(*ss, strings.Split(s, ",")...)
return nil
}
@@ -484,14 +484,17 @@ type user struct {
kgid auth.KGID
}
// String implements flag.Value.String.
func (u *user) String() string {
return fmt.Sprintf("%+v", *u)
return fmt.Sprintf("%d:%d", u.kuid, u.kgid)
}
// Get implements flag.Value.Get.
func (u *user) Get() any {
return u
}
// Set implements flag.Value.Set. Set(String()) should be idempotent.
func (u *user) Set(s string) error {
parts := strings.SplitN(s, ":", 2)
kuid, err := strconv.Atoi(parts[0])
+41 -35
View File
@@ -27,7 +27,11 @@ type fdMappings []boot.FDMapping
// String implements flag.Value.
func (i *fdMappings) String() string {
return fmt.Sprintf("%v", *i)
var mappings []string
for _, m := range *i {
mappings = append(mappings, fmt.Sprintf("%v:%v", m.Host, m.Guest))
}
return strings.Join(mappings, ",")
}
// Get implements flag.Value.
@@ -41,44 +45,46 @@ func (i *fdMappings) GetArray() []boot.FDMapping {
}
// Set implements flag.Value and appends a mapping from the command line to the
// mappings array.
// mappings array. Set(String()) should be idempotent.
func (i *fdMappings) Set(s string) error {
split := strings.Split(s, ":")
if len(split) != 2 {
// Split returns a slice of length 1 if its first argument does not
// contain the separator. An additional length check is not necessary.
// In case no separator is used and the argument is a valid integer, we
// assume that host FD and guest FD should be identical.
fd, err := strconv.Atoi(split[0])
if err != nil {
return fmt.Errorf("invalid flag value: must be an integer or a mapping of format M:N")
for _, m := range strings.Split(s, ",") {
split := strings.Split(m, ":")
if len(split) != 2 {
// Split returns a slice of length 1 if its first argument does not
// contain the separator. An additional length check is not necessary.
// In case no separator is used and the argument is a valid integer, we
// assume that host FD and guest FD should be identical.
fd, err := strconv.Atoi(split[0])
if err != nil {
return fmt.Errorf("invalid flag value: must be an integer or a mapping of format M:N")
}
*i = append(*i, boot.FDMapping{
Host: fd,
Guest: fd,
})
return nil
}
fdHost, err := strconv.Atoi(split[0])
if err != nil {
return fmt.Errorf("invalid flag host value: %v", err)
}
if fdHost < 0 {
return fmt.Errorf("flag host value must be >= 0: %d", fdHost)
}
fdGuest, err := strconv.Atoi(split[1])
if err != nil {
return fmt.Errorf("invalid flag guest value: %v", err)
}
if fdGuest < 0 {
return fmt.Errorf("flag guest value must be >= 0: %d", fdGuest)
}
*i = append(*i, boot.FDMapping{
Host: fd,
Guest: fd,
Host: fdHost,
Guest: fdGuest,
})
return nil
}
fdHost, err := strconv.Atoi(split[0])
if err != nil {
return fmt.Errorf("invalid flag host value: %v", err)
}
if fdHost < 0 {
return fmt.Errorf("flag host value must be >= 0: %d", fdHost)
}
fdGuest, err := strconv.Atoi(split[1])
if err != nil {
return fmt.Errorf("invalid flag guest value: %v", err)
}
if fdGuest < 0 {
return fmt.Errorf("flag guest value must be >= 0: %d", fdGuest)
}
*i = append(*i, boot.FDMapping{
Host: fdHost,
Guest: fdGuest,
})
return nil
}
+19 -20
View File
@@ -462,7 +462,7 @@ func fileAccessTypePtr(v FileAccessType) *FileAccessType {
return &v
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (f *FileAccessType) Set(v string) error {
switch v {
case "shared":
@@ -509,7 +509,7 @@ func networkTypePtr(v NetworkType) *NetworkType {
return &v
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (n *NetworkType) Set(v string) error {
switch v {
case "sandbox":
@@ -558,7 +558,7 @@ func queueingDisciplinePtr(v QueueingDiscipline) *QueueingDiscipline {
return &v
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (q *QueueingDiscipline) Set(v string) error {
switch v {
case "none":
@@ -616,7 +616,7 @@ func hostUDSPtr(v HostUDS) *HostUDS {
return &v
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (g *HostUDS) Set(v string) error {
switch v {
case "", "none":
@@ -640,20 +640,18 @@ func (g *HostUDS) Get() any {
// String implements flag.Value.
func (g HostUDS) String() string {
// Note: the order of operations is important given that HostUDS is a bitmap.
if g == HostUDSNone {
switch g {
case HostUDSNone:
return "none"
}
if g == HostUDSAll {
return "all"
}
if g == HostUDSOpen {
case HostUDSOpen:
return "open"
}
if g == HostUDSCreate {
case HostUDSCreate:
return "create"
case HostUDSAll:
return "all"
default:
panic(fmt.Sprintf("Invalid host UDS type %d", g))
}
panic(fmt.Sprintf("Invalid host UDS type %d", g))
}
// AllowOpen returns true if it can consume UDS from the host.
@@ -682,7 +680,7 @@ func hostFifoPtr(v HostFifo) *HostFifo {
return &v
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (g *HostFifo) Set(v string) error {
switch v {
case "", "none":
@@ -702,13 +700,14 @@ func (g *HostFifo) Get() any {
// String implements flag.Value.
func (g HostFifo) String() string {
if g == HostFifoNone {
switch g {
case HostFifoNone:
return "none"
}
if g == HostFifoOpen {
case HostFifoOpen:
return "open"
default:
panic(fmt.Sprintf("Invalid host fifo type %d", g))
}
panic(fmt.Sprintf("Invalid host fifo type %d", g))
}
// AllowOpen returns true if it can consume FIFOs from the host.
@@ -729,7 +728,7 @@ func defaultOverlay2() *Overlay2 {
return &Overlay2{rootMount: true, subMounts: false, medium: "self"}
}
// Set implements flag.Value.
// Set implements flag.Value. Set(String()) should be idempotent.
func (o *Overlay2) Set(v string) error {
if v == "none" {
o.rootMount = false
+2 -2
View File
@@ -140,7 +140,7 @@ type Container struct {
// OverlayMediums contains information about how the gofer mounts have been
// overlaid. The first entry is for rootfs and the following entries are for
// bind mounts in Spec.Mounts (in the same order).
OverlayMediums []boot.OverlayMedium `json:"overlayMediums"`
OverlayMediums boot.OverlayMediumFlags `json:"overlayMediums"`
//
// Fields below this line are not saved in the state file and will not
@@ -1117,7 +1117,7 @@ func (c *Container) createGoferProcess(spec *specs.Spec, conf *config.Config, bu
nextFD := donations.Transfer(cmd, 3)
cmd.Args = append(cmd.Args, "gofer", "--bundle", bundleDir)
cmd.Args = append(cmd.Args, "--overlay-mediums="+boot.ToOverlayMediumFlags(c.OverlayMediums))
cmd.Args = append(cmd.Args, "--overlay-mediums="+c.OverlayMediums.String())
// Open the spec file to donate to the sandbox.
specFile, err := specutils.OpenSpec(bundleDir)
+2 -2
View File
@@ -233,7 +233,7 @@ type Args struct {
// OverlayMediums contains information about how the gofer mounts have been
// overlaid. The first entry is for rootfs and the following entries are for
// bind mounts in Spec.Mounts (in the same order).
OverlayMediums []boot.OverlayMedium
OverlayMediums boot.OverlayMediumFlags
// MountHints provides extra information about containers mounts that apply
// to the entire pod.
@@ -737,7 +737,7 @@ func (s *Sandbox) createSandboxProcess(conf *config.Config, args *Args, startSyn
}
// Pass overlay mediums.
cmd.Args = append(cmd.Args, "--overlay-mediums="+boot.ToOverlayMediumFlags(args.OverlayMediums))
cmd.Args = append(cmd.Args, "--overlay-mediums="+args.OverlayMediums.String())
// Create a socket for the control server and donate it to the sandbox.
controlAddress, sockFD, err := createControlSocket(conf.RootDir, s.ID)