Skip to content

Commit f2e9965

Browse files
fvoznikagvisor-bot
authored andcommitted
Consolidate metadata configuration during S/R
S/R requires certain metadata objects to be set to the state file. The code doing that was spreadout, duplicating some of the logic (spec) and missing metadata in others (container count). This change, moves the code to a single location that is called from the multiple places that trigger saving. Fixes #1956 PiperOrigin-RevId: 813319242
1 parent e1d2468 commit f2e9965

File tree

6 files changed

+95
-74
lines changed

6 files changed

+95
-74
lines changed

pkg/sentry/control/state.go

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -87,72 +87,101 @@ type SaveOpts struct {
8787
// after checkpointing.
8888
Resume bool
8989

90-
// SaveRestoreExecArgv is the argv of the save/restore binary split by spaces.
90+
// ExecOpts contains options for executing a binary during save/restore.
91+
ExecOpts SaveRestoreExecOpts
92+
}
93+
94+
// SaveRestoreExecOpts contains options for executing a binary
95+
// during save/restore.
96+
type SaveRestoreExecOpts struct {
97+
// Argv is the argv of the save/restore binary split by spaces.
9198
// The first element is the path to the binary.
92-
SaveRestoreExecArgv string
99+
Argv string
93100

94-
// SaveRestoreExecTimeout is the timeout for waiting for the save/restore
95-
// binary.
96-
SaveRestoreExecTimeout time.Duration
101+
// Timeout is the timeout for waiting for the save/restore binary.
102+
Timeout time.Duration
97103

98-
// SaveRestoreExecContainerID is the ID of the container that the
99-
// save/restore binary executes in.
100-
SaveRestoreExecContainerID string
104+
// ContainerID is the ID of the container that the save/restore binary executes in.
105+
ContainerID string
101106
}
102107

103-
// Save saves the running system.
104-
func (s *State) Save(o *SaveOpts, _ *struct{}) error {
108+
// ConvertToStateSaveOpts converts a control.SaveOpts to a state.SaveOpts.
109+
// Returns a cleanup function that must be called after the SaveOpts is no longer
110+
// needed.
111+
func ConvertToStateSaveOpts(o *SaveOpts) (*state.SaveOpts, func(), error) {
105112
wantFiles := 1
106113
if o.HavePagesFile {
107114
wantFiles += 2
108115
}
109116
if gotFiles := len(o.FilePayload.Files); gotFiles != wantFiles {
110-
return fmt.Errorf("got %d files, wanted %d", gotFiles, wantFiles)
117+
return nil, nil, fmt.Errorf("got %d files, wanted %d", gotFiles, wantFiles)
111118
}
112119

113120
// Save to the first provided stream.
114121
stateFile, err := o.ReleaseFD(0)
115122
if err != nil {
116-
return err
123+
return nil, nil, err
117124
}
118-
defer stateFile.Close()
119-
saveOpts := state.SaveOpts{
120-
Destination: stateFile,
125+
cu := cleanup.Make(func() { stateFile.Close() })
126+
defer cu.Clean()
127+
128+
saveOpts := &state.SaveOpts{
129+
Destination: o.FilePayload.Files[0],
121130
Key: o.Key,
122131
Metadata: o.Metadata,
123132
MemoryFileSaveOpts: o.MemoryFileSaveOpts,
124133
Resume: o.Resume,
125134
}
135+
126136
if o.HavePagesFile {
127137
saveOpts.PagesMetadata, err = o.ReleaseFD(1)
128138
if err != nil {
129-
return err
139+
return nil, nil, err
130140
}
131-
defer saveOpts.PagesMetadata.Close()
141+
cu.Add(func() { saveOpts.PagesMetadata.Close() })
132142

133143
saveOpts.PagesFile, err = o.ReleaseFD(2)
134144
if err != nil {
135-
return err
145+
return nil, nil, err
136146
}
137-
defer saveOpts.PagesFile.Close()
147+
cu.Add(func() { saveOpts.PagesFile.Close() })
138148
}
139-
if err := PreSave(s.Kernel, o); err != nil {
149+
150+
return saveOpts, cu.Release(), nil
151+
}
152+
153+
// Save saves the running system.
154+
func (s *State) Save(o *SaveOpts, _ *struct{}) error {
155+
saveOpts, cleanup, err := ConvertToStateSaveOpts(o)
156+
if err != nil {
157+
return err
158+
}
159+
defer cleanup()
160+
161+
return s.SaveWithOpts(saveOpts, &o.ExecOpts)
162+
}
163+
164+
// SaveWithOpts saves the running system with the given options.
165+
func (s *State) SaveWithOpts(saveOpts *state.SaveOpts, execOpts *SaveRestoreExecOpts) error {
166+
if err := preSave(s.Kernel, saveOpts, execOpts); err != nil {
140167
return err
141168
}
142169
if err := saveOpts.Save(s.Kernel.SupervisorContext(), s.Kernel, s.Watchdog); err != nil {
143170
return err
144171
}
145-
if o.Resume {
146-
err = PostResume(s.Kernel, nil)
172+
if saveOpts.Resume {
173+
if err := PostResume(s.Kernel, nil); err != nil {
174+
return err
175+
}
147176
}
148-
return err
177+
return nil
149178
}
150179

151-
// PreSave is called before saving the kernel.
152-
func PreSave(k *kernel.Kernel, o *SaveOpts) error {
153-
if o.SaveRestoreExecArgv != "" {
154-
saveRestoreExecArgv := strings.Split(o.SaveRestoreExecArgv, " ")
155-
if err := ConfigureSaveRestoreExec(k, saveRestoreExecArgv, o.SaveRestoreExecTimeout, o.SaveRestoreExecContainerID); err != nil {
180+
// preSave is called before saving the kernel.
181+
func preSave(k *kernel.Kernel, o *state.SaveOpts, execOpts *SaveRestoreExecOpts) error {
182+
if execOpts.Argv != "" {
183+
argv := strings.Split(execOpts.Argv, " ")
184+
if err := ConfigureSaveRestoreExec(k, argv, execOpts.Timeout, execOpts.ContainerID); err != nil {
156185
return fmt.Errorf("failed to configure save/restore binary: %w", err)
157186
}
158187
if err := SaveRestoreExec(k, SaveRestoreExecSave); err != nil {

pkg/sentry/control/state_impl.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,11 @@ package control
1919

2020
import (
2121
"gvisor.dev/gvisor/pkg/sentry/kernel"
22+
"gvisor.dev/gvisor/pkg/sentry/state"
2223
"gvisor.dev/gvisor/pkg/timing"
2324
)
2425

25-
func preSaveImpl(k *kernel.Kernel, o *SaveOpts) error {
26+
func preSaveImpl(k *kernel.Kernel, o *state.SaveOpts) error {
2627
return nil
2728
}
2829

runsc/boot/autosave.go

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,11 @@ import (
2121
"gvisor.dev/gvisor/pkg/fd"
2222
"gvisor.dev/gvisor/pkg/log"
2323
"gvisor.dev/gvisor/pkg/sentry/arch"
24+
"gvisor.dev/gvisor/pkg/sentry/control"
2425
"gvisor.dev/gvisor/pkg/sentry/kernel"
2526
"gvisor.dev/gvisor/pkg/sentry/state"
2627
"gvisor.dev/gvisor/pkg/sentry/strace"
2728
"gvisor.dev/gvisor/pkg/sync"
28-
"gvisor.dev/gvisor/runsc/specutils"
29-
"gvisor.dev/gvisor/runsc/version"
3029
)
3130

3231
func getTargetForSaveResume(l *Loader) func(k *kernel.Kernel) {
@@ -39,14 +38,8 @@ func getTargetForSaveResume(l *Loader) func(k *kernel.Kernel) {
3938
Autosave: true,
4039
Resume: true,
4140
Destination: &buf,
42-
Metadata: map[string]string{VersionKey: version.Version()},
4341
}
44-
specsStr, err := specutils.ConvertSpecsToString(l.GetContainerSpecs())
45-
if err != nil {
46-
panic(fmt.Sprintf("error to get container specs from metadata, %v", err))
47-
}
48-
saveOpts.Metadata[ContainerSpecsKey] = specsStr
49-
saveOpts.Save(k.SupervisorContext(), k, l.watchdog)
42+
l.saveWithOpts(&saveOpts, &control.SaveRestoreExecOpts{})
5043
}
5144
}
5245

@@ -62,18 +55,12 @@ func getTargetForSaveRestore(l *Loader, files []*fd.FD) func(k *kernel.Kernel) {
6255
Autosave: true,
6356
Resume: false,
6457
Destination: files[0],
65-
Metadata: map[string]string{VersionKey: version.Version()},
6658
}
6759
if len(files) == 3 {
6860
saveOpts.PagesMetadata = files[1]
6961
saveOpts.PagesFile = files[2]
7062
}
71-
specsStr, err := specutils.ConvertSpecsToString(l.GetContainerSpecs())
72-
if err != nil {
73-
panic(fmt.Sprintf("error to get container specs from metadata, %v", err))
74-
}
75-
saveOpts.Metadata[ContainerSpecsKey] = specsStr
76-
saveOpts.Save(k.SupervisorContext(), k, l.watchdog)
63+
l.saveWithOpts(&saveOpts, &control.SaveRestoreExecOpts{})
7764
})
7865
}
7966
}

runsc/boot/controller.go

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -610,23 +610,16 @@ func (cm *containerManager) Restore(o *RestoreOpts, _ *struct{}) error {
610610
cm.l.k.Pause()
611611
timer.Reached("kernel paused")
612612

613-
var count int
614613
countStr, ok := metadata[ContainerCountKey]
615614
if !ok {
616-
// TODO(gvisor.dev/issue/1956): Add container count with syscall save
617-
// trigger. For now, assume that only a single container exists if metadata
618-
// isn't present.
619-
//
620-
// -return errors.New("container count not present in state file")
621-
count = 1
622-
} else {
623-
count, err = strconv.Atoi(countStr)
624-
if err != nil {
625-
return fmt.Errorf("invalid container count: %w", err)
626-
}
627-
if count < 1 {
628-
return fmt.Errorf("invalid container count value: %v", count)
629-
}
615+
return errors.New("container count not present in state file")
616+
}
617+
count, err := strconv.Atoi(countStr)
618+
if err != nil {
619+
return fmt.Errorf("invalid container count: %w", err)
620+
}
621+
if count < 1 {
622+
return fmt.Errorf("invalid container count value: %v", count)
630623
}
631624
cm.restorer.totalContainers = count
632625
log.Infof("Restoring a total of %d containers", cm.restorer.totalContainers)

runsc/boot/restore.go

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
"gvisor.dev/gvisor/pkg/sentry/pgalloc"
3737
"gvisor.dev/gvisor/pkg/sentry/socket/hostinet"
3838
"gvisor.dev/gvisor/pkg/sentry/socket/netstack"
39+
"gvisor.dev/gvisor/pkg/sentry/state"
3940
"gvisor.dev/gvisor/pkg/sentry/time"
4041
"gvisor.dev/gvisor/pkg/sentry/vfs"
4142
"gvisor.dev/gvisor/pkg/sentry/watchdog"
@@ -409,6 +410,17 @@ func (r *restorer) restore(l *Loader) error {
409410
}
410411

411412
func (l *Loader) save(o *control.SaveOpts) (err error) {
413+
saveOpts, cleanup, err := control.ConvertToStateSaveOpts(o)
414+
if err != nil {
415+
return err
416+
}
417+
defer cleanup()
418+
419+
return l.saveWithOpts(saveOpts, &o.ExecOpts)
420+
}
421+
422+
// saveWithOpts saves the kernel with the given options.
423+
func (l *Loader) saveWithOpts(saveOpts *state.SaveOpts, execOpts *control.SaveRestoreExecOpts) (err error) {
412424
defer func() {
413425
// This closure is required to capture the final value of err.
414426
l.k.OnCheckpointAttempt(err)
@@ -419,27 +431,24 @@ func (l *Loader) save(o *control.SaveOpts) (err error) {
419431
return errors.New("checkpoint not supported when using hostinet")
420432
}
421433

422-
if o.Metadata == nil {
423-
o.Metadata = make(map[string]string)
434+
if saveOpts.Metadata == nil {
435+
saveOpts.Metadata = make(map[string]string)
424436
}
425-
o.Metadata[ContainerCountKey] = strconv.Itoa(l.containerCount())
437+
saveOpts.Metadata[ContainerCountKey] = strconv.Itoa(l.containerCount())
426438

427439
// Save runsc version.
428-
o.Metadata[VersionKey] = version.Version()
440+
saveOpts.Metadata[VersionKey] = version.Version()
429441

430442
// Save container specs.
431443
specsStr, err := specutils.ConvertSpecsToString(l.GetContainerSpecs())
432444
if err != nil {
433445
return err
434446
}
435-
o.Metadata[ContainerSpecsKey] = specsStr
447+
saveOpts.Metadata[ContainerSpecsKey] = specsStr
436448

437449
state := control.State{
438450
Kernel: l.k,
439451
Watchdog: l.watchdog,
440452
}
441-
if err := state.Save(o, nil); err != nil {
442-
return err
443-
}
444-
return nil
453+
return state.SaveWithOpts(saveOpts, execOpts)
445454
}

runsc/sandbox/sandbox.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,11 +1516,13 @@ func (s *Sandbox) Checkpoint(cid string, imagePath string, opts CheckpointOpts)
15161516
FilePayload: urpc.FilePayload{
15171517
Files: files,
15181518
},
1519-
HavePagesFile: len(files) > 1,
1520-
Resume: opts.Resume,
1521-
SaveRestoreExecArgv: opts.SaveRestoreExecArgv,
1522-
SaveRestoreExecTimeout: opts.SaveRestoreExecTimeout,
1523-
SaveRestoreExecContainerID: opts.SaveRestoreExecContainerID,
1519+
HavePagesFile: len(files) > 1,
1520+
Resume: opts.Resume,
1521+
ExecOpts: control.SaveRestoreExecOpts{
1522+
Argv: opts.SaveRestoreExecArgv,
1523+
Timeout: opts.SaveRestoreExecTimeout,
1524+
ContainerID: opts.SaveRestoreExecContainerID,
1525+
},
15241526
}
15251527

15261528
if err := s.call(boot.ContMgrCheckpoint, &opt, nil); err != nil {

0 commit comments

Comments
 (0)