Skip to content

c8d/progress: Fix progress not ending #73

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Aug 24, 2022
Merged

c8d/progress: Fix progress not ending #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 2 additions & 6 deletions daemon/containerd/progress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ func showProgress(ctx context.Context, ongoing *jobs, w io.Writer, updateFunc up
out = streamformatter.NewJSONProgressOutput(w, false)
ticker = time.NewTicker(100 * time.Millisecond)
start = time.Now()
done bool
)

for _, j := range ongoing.Jobs() {
Expand All @@ -54,12 +53,9 @@ func showProgress(ctx context.Context, ongoing *jobs, w io.Writer, updateFunc up
logrus.WithError(err).Error("Updating progress failed")
return
}

if done {
return
}
case <-ctx.Done():
done = true
updateFunc(ctx, ongoing, out, start)
Copy link
Copy Markdown
Collaborator

@thaJeztah thaJeztah Aug 24, 2022

Choose a reason for hiding this comment

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

One last question (before I blame myself for not asking later); do we still need to check ongoing.IsResolved() here? Because we don't know "when" we reach this code. Or would calling updateFunc() be fine if ongoing is not in that state?

Suggested change
updateFunc(ctx, ongoing, out, start)
if ongoing.IsResolved() {
// update progress once more on cancellation or timeout
updateFunc(ctx, ongoing, out, start)
}

Copy link
Copy Markdown
Collaborator Author

@vvoland vvoland Aug 24, 2022

Choose a reason for hiding this comment

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

Generally calling the updateFunc is fine even if IsResolved is false. With the current push/pull progress handlers this is useless anyway, as they just loop over all jobs.
The IsResolved was extracted from the initial pull progress code, so I don't really know if I understand the intent behind it correctly, but to me it's just a safeguard before updating the progress when the jobs hasn't been added yet.
If the progress ends completely, then calling updateFunc without any jobs is still useful IMO, because it may add some kind of "Finished" status update or information that there weren't anything to be done. But this isn't something that is really done by the push/pull progress code we have.

Copy link
Copy Markdown
Collaborator

@thaJeztah thaJeztah Aug 24, 2022

Choose a reason for hiding this comment

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

Thanks for checking! Just wanted to be sure we didn't miss something (and, e.g. get a panic because some data wasn't set 😅)

return
}
}
}()
Expand Down