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

Nodereaper skip label #32

Merged
merged 4 commits into from
May 9, 2020
Merged

Conversation

pyieh
Copy link
Contributor

@pyieh pyieh commented May 8, 2020

Added 5 node label flag checks that can be used to skip reaping for a specific node with the given label:

"governor.keikoproj.io/node-reaper-disabled" 
"governor.keikoproj.io/reap-unready-disabled"
"governor.keikoproj.io/reap-unknown-disabled"
"governor.keikoproj.io/reap-flappy-disabled"
"governor.keikoproj.io/reap-old-disabled"

Each label correspond to a specific criteria for reaping, except for governor.keikoproj.io/node-reaper-disabled which ignore reaping regardless of the criteria.

Fixes #30

@pyieh pyieh requested a review from a team as a code owner May 8, 2020 22:20
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #32 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   59.10%   59.19%   +0.08%     
==========================================
  Files           5        5              
  Lines         939      941       +2     
==========================================
+ Hits          555      557       +2     
  Misses        319      319              
  Partials       65       65              
Impacted Files Coverage Δ
pkg/reaper/nodereaper/nodereaper.go 47.03% <100.00%> (+0.23%) ⬆️

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 b7f60a8...29b6005. Read the comment docs.

}
}

func (u *ReaperUnitTest) RunWithSkipLabel(t *testing.T, timeTest bool, skipLabels []SkipLabel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mm, do we need a separate method vs. Run()? seems like extra duplicate code here.
Can we just use Run() and add Labels as part of type ReaperUnitTest? then you can create nodes with the extra argument if len(u.Labels) != 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to minimize touching the existing tests and went with this implementation as there isn't method overloading in Go. But editing the ReaperUnitTest does seem cleaner. I'll update that.

… moved node label creation to ReaperUnitTest field to remove duplicate code
pyieh added 2 commits May 8, 2020 17:44
…he node type label (master/slave) to the list of nodes specified in FakeNode.nodeLabels
Copy link
Collaborator

@eytan-avisror eytan-avisror left a comment

Choose a reason for hiding this comment

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

🥇 Looks good! Thanks

@eytan-avisror eytan-avisror merged commit 4a20228 into keikoproj:master May 9, 2020
viveksyngh pushed a commit to viveksyngh/governor that referenced this pull request Oct 27, 2020
* added node label checks that can be used to skip node reaping

* simplify SkipLabel type usage to use existing string label constants, moved node label creation to ReaperUnitTest field to remove duplicate code

* moved node labels to FakeNode struct, createNodeLabels will combine the node type label (master/slave) to the list of nodes specified in FakeNode.nodeLabels

* updated skipLabelTest's to have skip labels with false to ensure those still get reaped
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.

Node Reaper - Add Support To Skip Nodes
2 participants