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

investigating where to ignore poststop task in alloc health tracker #9548

Merged
merged 5 commits into from
Jan 12, 2021

Conversation

drewbailey
Copy link
Contributor

@drewbailey drewbailey commented Dec 7, 2020

Ignores poststop jobs pending state when calculating the lastest start time for an allocation

resolves #9361

@drewbailey drewbailey force-pushed the drew/investigate-poststop-health branch from 75eb7c9 to d272803 Compare December 7, 2020 18:44
@vercel vercel bot temporarily deployed to Preview – nomad December 7, 2020 18:44 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 7, 2020 18:44 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 7, 2020 19:40 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 7, 2020 19:40 Inactive
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

This looks good, but this code is hard to review. I think poststop hooks just shouldn't affect deployments? If so we should document that and it might simplify things.

client/allochealth/tracker.go Outdated Show resolved Hide resolved
client/allochealth/tracker.go Outdated Show resolved Hide resolved
if state.State == structs.TaskStatePending {
// Ignore poststop since it will be pending until the main job exists
if (state.State == structs.TaskStatePending) &&
t.lifecycleTasks[taskName] != structs.TaskLifecycleHookPoststop {
Copy link
Member

Choose a reason for hiding this comment

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

Do we ever want to consider poststop hooks as part of a deployment? I don't think so in which case we should probably if poststop { continue } at the top of this loop? Otherwise I think if this code gets hit after poststop has started its started time will get taken into consideration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question! @jazzyfresh @notnoop, I'm very unfamiliar with lifecycle hooks :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @schmichael. In both service & batch cases, deployments should watch for everything but the poststop tasks

@drewbailey drewbailey force-pushed the drew/investigate-poststop-health branch from 0342656 to a9436ba Compare December 7, 2020 21:20
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 7, 2020 21:20 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 7, 2020 21:20 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 7, 2020 21:33 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 7, 2020 21:33 Inactive
client/allochealth/tracker.go Show resolved Hide resolved
client/allochealth/tracker.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nomad December 15, 2020 20:32 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 15, 2020 20:32 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 15, 2020 21:10 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 15, 2020 21:10 Inactive
@jazzyfresh jazzyfresh marked this pull request as ready for review December 15, 2020 21:10
@jazzyfresh jazzyfresh merged commit 492d62d into master Jan 12, 2021
@jazzyfresh jazzyfresh deleted the drew/investigate-poststop-health branch January 12, 2021 18:03
backspace pushed a commit that referenced this pull request Jan 22, 2021
* investigating where to ignore poststop task in alloc health tracker

* ignore poststop when setting latest start time for allocation

* clean up logic

* lifecycle: isolate mocks for poststop deployment test

* lifecycle: update comments in tracker

Co-authored-by: Jasmine Dahilig <jasmine@dahilig.com>
@tgross tgross added this to the 1.0.2 milestone Feb 22, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poststop lifecycle hook prevents deloyment status going healthy
4 participants