Skip to content
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

[release/0.9] Call container.Terminate() on shutdown timeouts #1554

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Oct 29, 2022

Call container.Terminate() on shutdown timeouts

(cherry picked from commit cae120b)
Signed-off-by: Kirtana Ashok Kirtana.Ashok@microsoft.com

@kevpar
Copy link
Member

kevpar commented Oct 29, 2022

  • Generally preferred to cherry-pick the commit with the actual change, rather than the merge commit, in this case that would be cae120b.
  • Please title the PR like [release/0.9] Call container.Terminate() on shutdown timeouts

@kiashok kiashok changed the title Merge pull request #1488 from dcantah/timeout-terminate [release/0.9] Call container.Terminate() on shutdown timeouts Oct 29, 2022
@kevpar
Copy link
Member

kevpar commented Oct 30, 2022

Can you rebase out the merge commit (and the revert) and then force push?

@kevpar
Copy link
Member

kevpar commented Oct 31, 2022

@katiewasnothere since Kirtana is out, can you look at the CI failure?

We were logging if HcsShutdownComputeSystem failed, but we weren't
trying to force kill the container via Terminate after if we timed
out waiting for it to complete. Shutdown is async and we wait for
a notification for success, so most of the time the call itself will
return nil, but it doesn't indicate indicate success until we can
see that the system exited. So now we will fallback to Terminate for:

1. Shutdown returning an error that doesn't indicate the result is
to be waited on.
2. The async result of shutdown was non-nil
3. Waiting for the result passed the timeout we set.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@katiewasnothere
Copy link
Contributor

@kevpar CI build is working now :)

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@katiewasnothere katiewasnothere merged commit e0b7d33 into microsoft:release/0.9 Oct 31, 2022
@kiashok kiashok deleted the terminateOnTimeout branch February 1, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants