Skip to content

Commit 8abdbb8

Browse files
author
John Howard
authored
Merge pull request #533 from Microsoft/wait_vm_or_container
Change HCS TaskExit ownership responsibility
2 parents b849a6e + b672b66 commit 8abdbb8

File tree

6 files changed

+180
-86
lines changed

6 files changed

+180
-86
lines changed

‎cmd/containerd-shim-runhcs-v1/exec_hcs.go‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -613,16 +613,22 @@ func (he *hcsExec) waitForExit() {
613613
he.ioWg.Wait()
614614
he.io.Close()
615615

616-
// We had a valid process so send the exited notification.
617-
he.events(
618-
runtime.TaskExitEventTopic,
619-
&eventstypes.TaskExit{
620-
ContainerID: he.tid,
621-
ID: he.id,
622-
Pid: uint32(he.pid),
623-
ExitStatus: he.exitStatus,
624-
ExitedAt: he.exitedAt,
625-
})
616+
// Only send the `runtime.TaskExitEventTopic` notification if this is a true
617+
// exec. For the `init` exec this is handled in task teardown.
618+
if he.tid != he.id {
619+
// We had a valid process so send the exited notification.
620+
he.events(
621+
runtime.TaskExitEventTopic,
622+
&eventstypes.TaskExit{
623+
ContainerID: he.tid,
624+
ID: he.id,
625+
Pid: uint32(he.pid),
626+
ExitStatus: he.exitStatus,
627+
ExitedAt: he.exitedAt,
628+
})
629+
}
630+
631+
// Free any waiters.
626632
he.exitedOnce.Do(func() {
627633
close(he.exited)
628634
})

‎cmd/containerd-shim-runhcs-v1/exec_wcow_podsandbox.go‎

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -166,15 +166,10 @@ func (wpse *wcowPodSandboxExec) Kill(ctx context.Context, signal uint32) error {
166166
wpse.state = shimExecStateExited
167167
wpse.exitStatus = 0
168168
wpse.exitedAt = time.Now()
169-
wpse.events(
170-
runtime.TaskExitEventTopic,
171-
&eventstypes.TaskExit{
172-
ContainerID: wpse.tid,
173-
ID: wpse.tid, // The init exec ID is always the same as Task ID.
174-
Pid: uint32(wpse.pid),
175-
ExitStatus: wpse.exitStatus,
176-
ExitedAt: wpse.exitedAt,
177-
})
169+
170+
// NOTE: We do not support a non `init` exec for this "fake" init
171+
// process. Skip any exited event which will be sent by the task.
172+
178173
close(wpse.exited)
179174
return nil
180175
case shimExecStateExited:
@@ -233,22 +228,13 @@ func (wpse *wcowPodSandboxExec) ForceExit(status int) {
233228
"status": status,
234229
}).Debug("hcsExec::ForceExit")
235230

236-
wasRunning := wpse.state == shimExecStateRunning
237231
wpse.state = shimExecStateExited
238232
wpse.exitStatus = 1
239233
wpse.exitedAt = time.Now()
240234

241-
if wasRunning {
242-
wpse.events(
243-
runtime.TaskExitEventTopic,
244-
&eventstypes.TaskExit{
245-
ContainerID: wpse.tid,
246-
ID: wpse.tid, // The init exec ID is always the same as Task ID.
247-
Pid: uint32(wpse.pid),
248-
ExitStatus: wpse.exitStatus,
249-
ExitedAt: wpse.exitedAt,
250-
})
251-
}
235+
// NOTE: We do not support a non `init` exec for this "fake" init
236+
// process. Skip any exited event which will be sent by the task.
237+
252238
close(wpse.exited)
253239
}
254240
}

‎cmd/containerd-shim-runhcs-v1/task_hcs.go‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -161,9 +161,13 @@ func newHcsTask(
161161
io)
162162

163163
if parent != nil {
164+
// We have a parent UVM. Listen for its exit and forcibly close this
165+
// task. This is not expected but in the event of a UVM crash we need to
166+
// handle this case.
164167
go ht.waitForHostExit()
165168
}
166-
169+
// In the normal case the `Signal` call from the caller killed this task's
170+
// init process.
167171
go func() {
168172
// Wait for our init process to exit.
169173
ht.init.Wait(context.Background())
@@ -468,14 +472,7 @@ func (ht *hcsTask) waitForHostExit() {
468472
return false
469473
})
470474
ht.init.ForceExit(1)
471-
ht.closeHostOnce.Do(func() {
472-
if err := ht.host.Close(); err != nil {
473-
logrus.WithFields(logrus.Fields{
474-
"tid": ht.id,
475-
logrus.ErrorKey: err,
476-
}).Error("hcsTask::close - failed host vm shutdown")
477-
}
478-
})
475+
ht.closeHost()
479476
}
480477

481478
// close shuts down the container that is owned by this task and if
@@ -544,18 +541,37 @@ func (ht *hcsTask) close() {
544541
}).Error("hcsTask::close - failed to close container")
545542
}
546543
}
544+
ht.closeHost()
545+
})
546+
}
547547

548+
// closeHost safely closes the hosting UVM if this task is the owner. Once
549+
// closed and all resources released it events the `runtime.TaskExitEventTopic`
550+
// for all upstream listeners.
551+
//
552+
// Note: If this is a process isolated task the hosting UVM is simply a `noop`.
553+
//
554+
// This call is idempotent and safe to call multiple times.
555+
func (ht *hcsTask) closeHost() {
556+
ht.closeHostOnce.Do(func() {
548557
if ht.ownsHost && ht.host != nil {
549-
// This task is also the host owner. Shutdown the host as part of
550-
// the init process going down.
551-
ht.closeHostOnce.Do(func() {
552-
if err := ht.host.Close(); err != nil {
553-
logrus.WithFields(logrus.Fields{
554-
"tid": ht.id,
555-
logrus.ErrorKey: err,
556-
}).Error("hcsTask::close - failed host vm shutdown")
557-
}
558-
})
558+
if err := ht.host.Close(); err != nil {
559+
logrus.WithFields(logrus.Fields{
560+
"tid": ht.id,
561+
logrus.ErrorKey: err,
562+
}).Error("hcsTask::closeHost - failed host vm shutdown")
563+
}
559564
}
565+
// Send the `init` exec exit notification always.
566+
exit := ht.init.Status()
567+
ht.events(
568+
runtime.TaskExitEventTopic,
569+
&eventstypes.TaskExit{
570+
ContainerID: ht.id,
571+
ID: exit.ID,
572+
Pid: uint32(exit.Pid),
573+
ExitStatus: exit.ExitStatus,
574+
ExitedAt: exit.ExitedAt,
575+
})
560576
})
561577
}

‎cmd/containerd-shim-runhcs-v1/task_wcow_podsandbox.go‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@ import (
1717
"github.com/sirupsen/logrus"
1818
)
1919

20+
// newWcowPodSandboxTask creates a fake WCOW task with a fake WCOW `init`
21+
// process as a performance optimization rather than creating an actual
22+
// container and process since it is not needed to hold open any namespaces like
23+
// the equivalent on Linux.
24+
//
25+
// It is assumed that this is the only fake WCOW task and that this task owns
26+
// `parent`. When the fake WCOW `init` process exits via `Signal` `parent` will
27+
// be forcibly closed by this task.
2028
func newWcowPodSandboxTask(ctx context.Context, events publisher, id, bundle string, parent *uvm.UtilityVM) shimTask {
2129
logrus.WithFields(logrus.Fields{
2230
"tid": id,
@@ -28,6 +36,9 @@ func newWcowPodSandboxTask(ctx context.Context, events publisher, id, bundle str
2836
init: newWcowPodSandboxExec(ctx, events, id, bundle),
2937
}
3038
if parent != nil {
39+
// We have (and own) a parent UVM. Listen for its exit and forcibly
40+
// close this task. This is not expected but in the event of a UVM crash
41+
// we need to handle this case.
3142
go func() {
3243
werr := parent.Wait()
3344
if werr != nil && !hcs.IsAlreadyClosed(werr) {
@@ -40,9 +51,20 @@ func newWcowPodSandboxTask(ctx context.Context, events publisher, id, bundle str
4051
// already) to unblock any waiters since the platform wont send any
4152
// events for this fake process.
4253
wpst.init.ForceExit(1)
43-
parent.Close()
54+
55+
// Close the host and event the exit.
56+
wpst.close()
4457
}()
4558
}
59+
// In the normal case the `Signal` call from the caller killed this fake
60+
// init process.
61+
go func() {
62+
// Wait for it to exit on its own
63+
wpst.init.Wait(context.Background())
64+
65+
// Close the host and event the exit
66+
wpst.close()
67+
}()
4668
return wpst
4769
}
4870

@@ -118,14 +140,6 @@ func (wpst *wcowPodSandboxTask) KillExec(ctx context.Context, eid string, signal
118140
if err != nil {
119141
return err
120142
}
121-
if eid == "" {
122-
// We killed the fake init task. Bring down the uvm.
123-
wpst.closeOnce.Do(func() {
124-
if wpst.host != nil {
125-
wpst.host.Close()
126-
}
127-
})
128-
}
129143
return nil
130144
}
131145

@@ -171,3 +185,31 @@ func (wpst *wcowPodSandboxTask) Pids(ctx context.Context) ([]options.ProcessDeta
171185
},
172186
}, nil
173187
}
188+
189+
// close safely closes the hosting UVM. Because of the specialty of this task it
190+
// is assumed that this is always the owner of `wpst.host`. Once closed and all
191+
// resources released it events the `runtime.TaskExitEventTopic` for all
192+
// upstream listeners.
193+
//
194+
// This call is idempotent and safe to call multiple times.
195+
func (wpst *wcowPodSandboxTask) close() {
196+
wpst.closeOnce.Do(func() {
197+
if err := wpst.host.Close(); !hcs.IsAlreadyClosed(err) {
198+
logrus.WithFields(logrus.Fields{
199+
"tid": wpst.id,
200+
logrus.ErrorKey: err,
201+
}).Error("wcowPodSandboxTask::close - failed host vm shutdown")
202+
}
203+
// Send the `init` exec exit notification always.
204+
exit := wpst.init.Status()
205+
wpst.events(
206+
runtime.TaskExitEventTopic,
207+
&eventstypes.TaskExit{
208+
ContainerID: wpst.id,
209+
ID: exit.ID,
210+
Pid: uint32(exit.Pid),
211+
ExitStatus: exit.ExitStatus,
212+
ExitedAt: exit.ExitedAt,
213+
})
214+
})
215+
}

‎internal/hcs/process.go‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ type Process struct {
2323
callbackNumber uintptr
2424

2525
logctx logrus.Fields
26+
27+
waitBlock chan struct{}
28+
waitError error
2629
}
2730

2831
func newProcess(process hcsProcess, processID int, computeSystem *System) *Process {
@@ -34,6 +37,7 @@ func newProcess(process hcsProcess, processID int, computeSystem *System) *Proce
3437
logfields.ContainerID: computeSystem.ID(),
3538
logfields.ProcessID: processID,
3639
},
40+
waitBlock: make(chan struct{}),
3741
}
3842
}
3943

@@ -163,33 +167,47 @@ func (process *Process) Kill() (err error) {
163167
return nil
164168
}
165169

166-
// Wait waits for the process to exit.
170+
// waitBackground waits for the process exit notification. Once received sets
171+
// `process.waitError` (if any) and unblocks all `Wait` and `WaitTimeout` calls.
172+
//
173+
// This MUST be called exactly once per `process.handle` but `Wait` and
174+
// `WaitTimeout` are safe to call multiple times.
175+
func (process *Process) waitBackground() {
176+
process.waitError = waitForNotification(process.callbackNumber, hcsNotificationProcessExited, nil)
177+
close(process.waitBlock)
178+
}
179+
180+
// Wait waits for the process to exit. If the process has already exited returns
181+
// the pervious error (if any).
167182
func (process *Process) Wait() (err error) {
168183
operation := "hcsshim::Process::Wait"
169184
process.logOperationBegin(operation)
170185
defer func() { process.logOperationEnd(operation, err) }()
171186

172-
err = waitForNotification(process.callbackNumber, hcsNotificationProcessExited, nil)
173-
if err != nil {
187+
<-process.waitBlock
188+
if process.waitError != nil {
174189
return makeProcessError(process, operation, err, nil)
175190
}
176-
177191
return nil
178192
}
179193

180-
// WaitTimeout waits for the process to exit or the duration to elapse. It returns
181-
// false if timeout occurs.
194+
// WaitTimeout waits for the process to exit or the duration to elapse. If the
195+
// process has already exited returns the pervious error (if any). If a timeout
196+
// occurs returns `ErrTimeout`.
182197
func (process *Process) WaitTimeout(timeout time.Duration) (err error) {
183198
operation := "hcssshim::Process::WaitTimeout"
184199
process.logOperationBegin(operation)
185200
defer func() { process.logOperationEnd(operation, err) }()
186201

187-
err = waitForNotification(process.callbackNumber, hcsNotificationProcessExited, &timeout)
188-
if err != nil {
189-
return makeProcessError(process, operation, err, nil)
202+
select {
203+
case <-process.waitBlock:
204+
if process.waitError != nil {
205+
return makeProcessError(process, operation, process.waitError, nil)
206+
}
207+
return nil
208+
case <-time.After(timeout):
209+
return makeProcessError(process, operation, ErrTimeout, nil)
190210
}
191-
192-
return nil
193211
}
194212

195213
// ResizeConsole resizes the console of the process.

0 commit comments

Comments
 (0)