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

Support more values and status duration for condition #43

Merged
merged 1 commit into from May 20, 2019

Conversation

@openstacker
Copy link
Contributor

commented May 5, 2019

The attribute LastTransitionTime of NodeCondition indicates that last
time the condition transit from one status to another, so it's very
useful to be leveraged to track how long a node is in a particular
state.

Meanwhile, a new backward-compatible condition format is introduced
so support more values and the status duration for condition as below:

OutOfDisk=True,10m

That means when condition OutOfDisk equals True and the state needs
to be on that status at least 10 mins. And the old format is still
supported. When a simple condition, e.g. KernelDeadlock, is passed in,
it will be parsed as KernelDeadlock=True,0 to keep the backward
compatibility.

Note that the value after comma can be any string which can be parsed
by time.ParseDuration, e.g. 30s, 10m, etc.

fixes #33

@openstacker

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@negz @StevenACoffman could you please help review this PR? Any comments will be appreciated. Thanks.

@codecov

This comment has been minimized.

Copy link

commented May 5, 2019

Codecov Report

Merging #43 into master will increase coverage by 0.63%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   84.78%   85.41%   +0.63%     
==========================================
  Files           6        6              
  Lines         276      288      +12     
==========================================
+ Hits          234      246      +12     
  Misses         40       40              
  Partials        2        2
Impacted Files Coverage Δ
internal/kubernetes/drainer.go 93.57% <ø> (ø) ⬆️
internal/kubernetes/nodefilters.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ffc2df...a0ca2e7. Read the comment docs.

@StevenACoffman

This comment has been minimized.

Copy link

commented May 5, 2019

Thanks! I'm looking on a phone so can't test it from my vacation but looks good to me! What do you think @jacobstr

@jacobstr
Copy link
Contributor

left a comment

I like this. Could we add a few test cases to TestNodeConditionFilter to exercise LastTransitionTime and Unknown. I realize this will require providing a way to stub time.Now. There are some approaches to do this but the simplest is to make the now function a package variable e.g.

package main

import (
	"fmt"
	"time"
)

var now = time.Now

func testTime() time.Time {
	t, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
	if err != nil {
		panic(err)
	}
	return t
}

func main() {
	fmt.Printf("%v\n", now().Format(time.RFC3339))
	now = testTime
	fmt.Printf("%v\n", now().Format(time.RFC3339))
}
internal/kubernetes/nodefilters.go Outdated Show resolved Hide resolved
internal/kubernetes/nodefilters.go Outdated Show resolved Hide resolved
internal/kubernetes/nodefilters.go Outdated Show resolved Hide resolved
internal/kubernetes/drainer.go Outdated Show resolved Hide resolved
@openstacker

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@jacobstr All good suggestions and I do like them. I will address them in next commit. Thanks again for the review.

@openstacker openstacker force-pushed the openstacker:issue-33 branch from 449a834 to c7826fa May 9, 2019

Support more values and status duration for condition
The attribute LastTransitionTime of NodeCondition indicates that last
time the condition transit from one status to another, so it's very
useful to be leveraged to track how long a node is in a particular
state.

Meanwhile, a new backward compatible condition format is introduced
so support more values and the status duration for condition as below:

OutOfDisk=True,10m

That means when condition OutOfDisk equals True and the state needs
to be on that status at least 10 mins. And the old format is still
supported. When a simple condition, e.g. KernelDeadlock, is passed in
, will be parsed as KernelDeadlock=True,0 to keep the backward
compatibility.

Note that the value after comma can be any string which can be parsed
by time.ParseDuration, e.g. 30s, 10m, etc.

@openstacker openstacker force-pushed the openstacker:issue-33 branch from c7826fa to a0ca2e7 May 9, 2019

@jacobstr

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@openstacker I'm going to be running for vacation this evening. Looking forward to picking this up with you when I get back.

@openstacker

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@jacobstr No problem, have a good and safe trip. Cheers.

@jacobstr jacobstr merged commit 83b5f70 into planetlabs:master May 20, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 84.78%)
Details
codecov/project 85.41% (+0.63%) compared to 8ffc2df
Details
@jacobstr

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Thanks @openstacker! Merged!

@StevenACoffman

This comment has been minimized.

Copy link

commented May 20, 2019

Thank you both!

Draino can now automatically drain any node that goes unresponsive for X period of time, e.g. Ready=Unknown,10m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.