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

api: add StartedAt in Node.DrainStrategy #6698

Merged
merged 2 commits into from
Nov 15, 2019
Merged

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Nov 13, 2019

The new StartedAt field records the time the current drain process started in a node.

Sample response:

  "DrainStrategy": {
    "Deadline": 3600000000000,
    "ForceDeadline": "2019-11-13T23:49:32.696485Z",
    "IgnoreSystemJobs": false,
    "StartedAt": "2019-11-13T22:49:08.162419Z"
  }

Fixes #6694

@lgfa29 lgfa29 force-pushed the f-add-drain-start-time branch from 3b7d756 to 1024103 Compare November 13, 2019 23:00
@lgfa29 lgfa29 requested review from notnoop and drewbailey November 13, 2019 23:12
Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

Would it be preferable to setup a var of time.Time outside the two if statements and then assign this within the first if statement as needed to also be used in the second? This would mean the started time and deadline are consistent with the user passed DrainStrategy.Deadline.

// Mark the deadline time
if args.DrainStrategy != nil && args.DrainStrategy.Deadline.Nanoseconds() > 0 {
args.DrainStrategy.ForceDeadline = time.Now().Add(args.DrainStrategy.Deadline)
args.DrainStrategy.ForceDeadline = time.Now().Add(args.DrainStrategy.Deadline).UTC()
Copy link
Contributor

Choose a reason for hiding this comment

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

I like @jrasell's idea of having a var startedAt time that we could then use here as well to add on to. There seems to be at least one more location calling time.Now that could also benefit from using a shared variable

@lgfa29
Copy link
Contributor Author

lgfa29 commented Nov 14, 2019

Ah! That makes perfect sense, thanks for the feedback @jrasell @drewbailey

@lgfa29
Copy link
Contributor Author

lgfa29 commented Nov 14, 2019

@jrasell @drewbailey I refactored the logic to use a single value for current time. Let me know if it's better.

I also remove the check for node.DrainStrategy.StartedAt.IsZero() because it's not relevant. I initially had it as args.DrainStrategy.StartedAt.IsZero() for backwards compatibility, but I don't think it makes sense to check it against node.

@lgfa29 lgfa29 requested a review from drewbailey November 14, 2019 21:11
Copy link
Contributor

@drewbailey drewbailey left a comment

Choose a reason for hiding this comment

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

LGTM!

@lgfa29 lgfa29 merged commit b0615a7 into master Nov 15, 2019
@lgfa29 lgfa29 deleted the f-add-drain-start-time branch November 15, 2019 20:38
@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 Jan 26, 2023
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.

Add a Started timestamp to DrainStrategy
3 participants