-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix nil pointer panic for monitor #6830
Conversation
runtime/restart/monitor/monitor.go
Outdated
@@ -220,11 +220,13 @@ func (m *monitor) monitor(ctx context.Context) ([]change, error) { | |||
} | |||
desiredStatus := containerd.ProcessStatus(labels[restart.StatusLabel]) | |||
task, err := c.Task(ctx, nil) | |||
if err != nil && desiredStatus == containerd.Stopped { | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the logic some, is there a case this was trying to account for when Task
may return not found and the switch statement will handle it when desired state is running?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the case when Task
returns not found and the restart desired to stop, maybe we need to continue to stop the container.
I change the logic to be the same as before .
Build succeeded.
|
runtime/restart/monitor/monitor.go
Outdated
if err != nil { | ||
logrus.WithError(err).Error("monitor") | ||
if desiredStatus == containerd.Stopped { | ||
goto desiredSwitch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can avoid goto
and rewrite the logic a bit more clearly.
var status containerd.Status
if task, err := c.Task(ctx, nil); err == nil {
status, err = task.Status(ctx)
if err == nil {
if desiredStatus == status.Stopped {
continue
}
} else {
logrus.WithError(err).Error("monitor")
if desiredStatus == containerd.Stopped {
continue
}
}
} else {
logrus.WithError(err).Error("monitor")
if desiredStatus == containerd.Stopped {
continue
}
}
This allows error cases where desiredStatus
is equal to containerd.Running
to fall through to the switch statement while avoiding the scope clarity issues with goto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
var (
task containerd.Task
status containerd.Status
err error
)
if task, err = c.Task(ctx, nil); err == nil {
if status, err = task.Status(ctx); err == nil {
if desiredStatus == status.Status {
continue
}
}
}
// Task or Status return error, only desired to stop
if err != nil {
logrus.WithError(err).Error("monitor")
if desiredStatus != containerd.Stopped {
continue
}
}
Signed-off-by: Ye Sijun <[email protected]>
Build succeeded.
|
Seems still broken
https://github.com/containerd/nerdctl/runs/6091620240?check_suite_focus=true |
Signed-off-by: Ye Sijun <[email protected]>
Signed-off-by: Ye Sijun [email protected]
build main and run containerd failed.