Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions cmd/containerd-shim-runhcs-v1/events_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,3 @@ func (p *fakePublisher) publishEvent(ctx context.Context, topic string, event in
p.events = append(p.events, event)
return nil
}

func (p *fakePublisher) getEvents() []interface{} {
return p.events
}
50 changes: 17 additions & 33 deletions cmd/containerd-shim-runhcs-v1/exec_hcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"strings"
"sync"
"time"

Expand All @@ -22,18 +21,6 @@ import (
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"go.opencensus.io/trace"
"golang.org/x/sys/windows"
)

const (
// processStopTimeout is the amount of time after signaling the process with
// a signal expected to kill the process that the exec must wait before
// forcibly terminating the process.
//
// For example, sending a SIGKILL is expected to kill a process. If the
// process does not stop within `processStopTimeout` we will forcibly
// terminate the process without a signal.
processStopTimeout = time.Second * 5
Comment thread
katiewasnothere marked this conversation as resolved.
)

// newHcsExec creates an exec to track the lifetime of `spec` in `c` which is
Expand Down Expand Up @@ -199,7 +186,7 @@ func (he *hcsExec) startInternal(ctx context.Context, initializeContainer bool)
}
defer func() {
if err != nil {
he.c.Terminate(ctx)
_ = he.c.Terminate(ctx)
he.c.Close()
}
}()
Expand Down Expand Up @@ -233,22 +220,26 @@ func (he *hcsExec) startInternal(ctx context.Context, initializeContainer bool)
// Publish the task/exec start event. This MUST happen before waitForExit to
// avoid publishing the exit previous to the start.
if he.id != he.tid {
he.events.publishEvent(
if err := he.events.publishEvent(
ctx,
runtime.TaskExecStartedEventTopic,
&eventstypes.TaskExecStarted{
ContainerID: he.tid,
ExecID: he.id,
Pid: uint32(he.pid),
})
}); err != nil {
return err
}
} else {
he.events.publishEvent(
if err := he.events.publishEvent(
ctx,
runtime.TaskStartEventTopic,
&eventstypes.TaskStart{
ContainerID: he.tid,
Pid: uint32(he.pid),
})
}); err != nil {
return err
}
}

// wait in the background for the exit.
Expand Down Expand Up @@ -350,7 +341,7 @@ func (he *hcsExec) ForceExit(ctx context.Context, status int) {
he.exitFromCreatedL(ctx, status)
case shimExecStateRunning:
// Kill the process to unblock `he.waitForExit`
he.p.Process.Kill(ctx)
_, _ = he.p.Process.Kill(ctx)
}
}
}
Expand Down Expand Up @@ -451,14 +442,14 @@ func (he *hcsExec) waitForExit() {
he.sl.Unlock()

// Wait for all IO copies to complete and free the resources.
he.p.Wait()
_ = he.p.Wait()
he.io.Close(ctx)

// Only send the `runtime.TaskExitEventTopic` notification if this is a true
// exec. For the `init` exec this is handled in task teardown.
if he.tid != he.id {
// We had a valid process so send the exited notification.
he.events.publishEvent(
if err := he.events.publishEvent(
ctx,
runtime.TaskExitEventTopic,
&eventstypes.TaskExit{
Expand All @@ -467,7 +458,9 @@ func (he *hcsExec) waitForExit() {
Pid: uint32(he.pid),
ExitStatus: he.exitStatus,
ExitedAt: he.exitedAt,
})
}); err != nil {
log.G(ctx).WithError(err).Error("failed to publish TaskExitEvent")
}
}

// Free any waiters.
Expand All @@ -490,7 +483,7 @@ func (he *hcsExec) waitForContainerExit() {

cexit := make(chan struct{})
go func() {
he.c.Wait()
_ = he.c.Wait()
close(cexit)
}()
select {
Expand All @@ -503,20 +496,11 @@ func (he *hcsExec) waitForContainerExit() {
he.exitFromCreatedL(ctx, 1)
case shimExecStateRunning:
// Kill the process to unblock `he.waitForExit`.
he.p.Process.Kill(ctx)
_, _ = he.p.Process.Kill(ctx)
}
he.sl.Unlock()
case <-he.processDone:
// Process exited first. This is the normal case do nothing because
// `he.waitForExit` will release any waiters.
}
}

// escapeArgs makes a Windows-style escaped command line from a set of arguments
func escapeArgs(args []string) string {
escapedArgs := make([]string, len(args))
for i, a := range args {
escapedArgs[i] = windows.EscapeArg(a)
}
return strings.Join(escapedArgs, " ")
}
6 changes: 2 additions & 4 deletions cmd/containerd-shim-runhcs-v1/exec_wcow_podsandbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,15 @@ func (wpse *wcowPodSandboxExec) Start(ctx context.Context) error {
wpse.state = shimExecStateRunning
wpse.pid = 1 // Fake but init pid is always 1

// Publish the task start event. We mever have an exec for the WCOW
// Publish the task start event. We never have an exec for the WCOW
// PodSandbox.
wpse.events.publishEvent(
return wpse.events.publishEvent(
ctx,
runtime.TaskStartEventTopic,
&eventstypes.TaskStart{
ContainerID: wpse.tid,
Pid: uint32(wpse.pid),
})

return nil
}

func (wpse *wcowPodSandboxExec) Kill(ctx context.Context, signal uint32) error {
Expand Down
14 changes: 1 addition & 13 deletions cmd/containerd-shim-runhcs-v1/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"errors"
"fmt"
"os"
"runtime"
"strings"

"github.com/Microsoft/go-winio/pkg/etw"
Expand Down Expand Up @@ -44,17 +43,6 @@ var (
idFlag string
)

func stack() []byte {
buf := make([]byte, 1024)
for {
n := runtime.Stack(buf, true) // true means all goroutines
if n < len(buf) {
return buf[:n]
}
buf = make([]byte, 2*len(buf))
}
}

Comment thread
dcantah marked this conversation as resolved.
func etwCallback(sourceID guid.GUID, state etw.ProviderState, level etw.Level, matchAnyKeyword uint64, matchAllKeyword uint64, filterData uintptr) {
if state == etw.ProviderStateCaptureState {
resp, err := svc.DiagStacks(context.Background(), &shimdiag.StacksRequest{})
Expand Down Expand Up @@ -83,7 +71,7 @@ func main() {
}
}

provider.WriteEvent(
_ = provider.WriteEvent(
"ShimLaunched",
nil,
etw.WithFields(
Expand Down
6 changes: 4 additions & 2 deletions cmd/containerd-shim-runhcs-v1/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques
p.sandboxTask = newWcowPodSandboxTask(ctx, events, req.ID, req.Bundle, parent, nsid)
// Publish the created event. We only do this for a fake WCOW task. A
// HCS Task will event itself based on actual process lifetime.
events.publishEvent(
if err := events.publishEvent(
ctx,
runtime.TaskCreateEventTopic,
&eventstypes.TaskCreate{
Expand All @@ -191,7 +191,9 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques
},
Checkpoint: "",
Pid: 0,
})
}); err != nil {
return nil, err
}
} else {
if isWCOW {
// The pause container activation will immediately exit on Windows
Expand Down
3 changes: 1 addition & 2 deletions cmd/containerd-shim-runhcs-v1/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,5 @@ func setupDebuggerEvent() {
return
}
logrus.WithField("event", event).Info("Halting until signalled")
windows.WaitForSingleObject(handle, windows.INFINITE)
return
_, _ = windows.WaitForSingleObject(handle, windows.INFINITE)
}
7 changes: 1 addition & 6 deletions cmd/containerd-shim-runhcs-v1/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ import (
"go.opencensus.io/trace"
)

type cdevent struct {
topic string
event interface{}
}

var _ = (task.TaskService)(&service{})

type service struct {
Expand Down Expand Up @@ -460,7 +455,7 @@ func (s *service) DiagPid(ctx context.Context, req *shimdiag.PidRequest) (*shimd
if s == nil {
return nil, nil
}
ctx, span := trace.StartSpan(ctx, "DiagPid")
ctx, span := trace.StartSpan(ctx, "DiagPid") //nolint:ineffassign,staticcheck
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious; what was the linter complaining about here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It complained because the call to StartSpan returns a new ctx, which isn't used again in the function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah!

would something like this work for those as well?

Suggested change
ctx, span := trace.StartSpan(ctx, "DiagPid") //nolint:ineffassign,staticcheck
_, span := trace.StartSpan(ctx, "DiagPid")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do _, span := ... and remove the nolint?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, @thaJeztah commented right before me :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't add new functions on the context struct type since it's defined in a different imported package that we don't own.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the nolint directives in, and then later change the code such that they are no longer needed (e.g. start using ctx), will the linter tell us "hey, this directive isn't doing anything"?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from what I've seen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference here. I would probably go with _, span := ..., and rely on re-creating the ctx variable later if we end up needing it, but leaving the nolint directives in seems okay to me as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to keep the nolint directives in for now and if it becomes annoying we can take them out.

defer span.End()

span.AddAttributes(trace.StringAttribute("tid", s.tid))
Expand Down
2 changes: 1 addition & 1 deletion cmd/containerd-shim-runhcs-v1/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ The start command can either start a new shim or return an address to an existin
w.Close()
defer func() {
if err != nil {
cmd.Process.Kill()
_ = cmd.Process.Kill()
}
}()

Expand Down
9 changes: 0 additions & 9 deletions cmd/containerd-shim-runhcs-v1/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,6 @@ import (

var errTaskNotIsolated = errors.New("task is not isolated")

// shimTaskPidPair groups a process pid to its execID if it was user generated.
type shimTaskPidPair struct {
// Pid is the pid of the container process.
Pid int
// ExecID is the id of the exec if this container process was user
// generated.
ExecID string
}

type shimTask interface {
// ID returns the original id used at `Create`.
ID() string
Expand Down
40 changes: 23 additions & 17 deletions cmd/containerd-shim-runhcs-v1/task_hcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,11 @@ func newHcsTask(
}

opts := hcsoci.CreateOptions{
ID: req.ID,
Owner: owner,
Spec: s,
HostingSystem: parent,
NetworkNamespace: netNS,
ScaleCPULimitsToSandbox: shimOpts.ScaleCpuLimitsToSandbox,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove this assignment?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter was complaining because below we check if shimOpts is nil, so it didn't like this direct access to it. But also, we set it additionally right below this struct creation, so we only need one :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was very confused, because I thought we already fixed this in #913... Turns out the issue was re-introduced, probably due to a bad merge, here. :(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cry

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it was noticed just before / after merge 🙈 #915 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I force pushed to fix before checking in 😭 😭 😭. Well I've gotta give it to myself, I both fixed it and then broke it again. This deserves a medal

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💩 happens; you did that to test if the linters work, correct? 😂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100%, don't let anyone tell you otherwise 😉

ID: req.ID,
Owner: owner,
Spec: s,
HostingSystem: parent,
NetworkNamespace: netNS,
}

if shimOpts != nil {
Expand Down Expand Up @@ -204,7 +203,7 @@ func newHcsTask(
go ht.waitInitExit(!isTemplate)

// Publish the created event
ht.events.publishEvent(
if err := ht.events.publishEvent(
ctx,
runtime.TaskCreateEventTopic,
&eventstypes.TaskCreate{
Expand All @@ -219,7 +218,9 @@ func newHcsTask(
},
Checkpoint: "",
Pid: uint32(ht.init.Pid()),
})
}); err != nil {
return nil, err
}
return ht, nil
}

Expand Down Expand Up @@ -312,7 +313,7 @@ func newClonedHcsTask(
go ht.waitInitExit(true)

// Publish the created event
ht.events.publishEvent(
if err := ht.events.publishEvent(
ctx,
runtime.TaskCreateEventTopic,
&eventstypes.TaskCreate{
Expand All @@ -327,7 +328,9 @@ func newClonedHcsTask(
},
Checkpoint: "",
Pid: uint32(ht.init.Pid()),
})
}); err != nil {
return nil, err
}
return ht, nil
}

Expand Down Expand Up @@ -443,15 +446,13 @@ func (ht *hcsTask) CreateExec(ctx context.Context, req *task.ExecProcessRequest,
ht.execs.Store(req.ExecID, he)

// Publish the created event
ht.events.publishEvent(
return ht.events.publishEvent(
ctx,
runtime.TaskExecAddedEventTopic,
&eventstypes.TaskExecAdded{
ContainerID: ht.id,
ExecID: req.ExecID,
})

return nil
}

func (ht *hcsTask) GetExec(eid string) (shimExec, error) {
Expand Down Expand Up @@ -542,7 +543,7 @@ func (ht *hcsTask) DeleteExec(ctx context.Context, eid string) (int, uint32, tim
}

// Publish the deleted event
ht.events.publishEvent(
if err := ht.events.publishEvent(
ctx,
runtime.TaskDeleteEventTopic,
&eventstypes.TaskDelete{
Expand All @@ -551,7 +552,9 @@ func (ht *hcsTask) DeleteExec(ctx context.Context, eid string) (int, uint32, tim
Pid: status.Pid,
ExitStatus: status.ExitStatus,
ExitedAt: status.ExitedAt,
})
}); err != nil {
return 0, 0, time.Time{}, err
}

return int(status.Pid), status.ExitStatus, status.ExitedAt, nil
}
Expand Down Expand Up @@ -741,7 +744,8 @@ func (ht *hcsTask) closeHost(ctx context.Context) {
}
// Send the `init` exec exit notification always.
exit := ht.init.Status()
ht.events.publishEvent(

if err := ht.events.publishEvent(
ctx,
runtime.TaskExitEventTopic,
&eventstypes.TaskExit{
Expand All @@ -750,7 +754,9 @@ func (ht *hcsTask) closeHost(ctx context.Context) {
Pid: uint32(exit.Pid),
ExitStatus: exit.ExitStatus,
ExitedAt: exit.ExitedAt,
})
}); err != nil {
log.G(ctx).WithError(err).Error("failed to publish TaskExitEventTopic")
}
close(ht.closed)
})
}
Expand Down
Loading