-
Notifications
You must be signed in to change notification settings - Fork 86
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
filter out pod in eviction using pod ownerreference kind #99
filter out pod in eviction using pod ownerreference kind #99
Conversation
c82de02
to
592b491
Compare
Codecov Report
|
@jacobstr , I cannot get access to the code coverage report. Codecov is complaining that the project is not public (looks like it is... ). Is there any settings you can change so that we can read the report? Also I am going to add some test to the |
592b491
to
365a458
Compare
@dbenque there seem to be no permissions related to view access outside of making in a collaborator - which I can't do unilaterally. Perhaps there's some workaround e.g. setting it up for your fork? I realize it's not a great story for contributors if you have to guess and and check to pass the CI flow in an MR. |
@jacobstr I don't know if you did something, but now I have access to the report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great, mostly wording nits. The one implementation suggestion would be to deprecate the old style instead of removing it for the time being.
@@ -67,11 +68,9 @@ func main() { | |||
leaderElectionRetryPeriod = app.Flag("leader-election-retry-period", "Leader election retry period.").Default(DefaultLeaderElectionRetryPeriod.String()).Duration() | |||
leaderElectionTokenName = app.Flag("leader-election-token-name", "Leader election token name.").Default(kubernetes.Component).String() | |||
|
|||
skipDrain = app.Flag("skip-drain", "Whether to skip draining nodes after cordoning.").Default("false").Bool() | |||
evictDaemonSetPods = app.Flag("evict-daemonset-pods", "Evict pods that were created by an extant DaemonSet.").Bool() | |||
evictStatefulSetPods = app.Flag("evict-statefulset-pods", "Evict pods that were created by an extant StatefulSet.").Bool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've asked folks to preserve these flags in the past. It looks like it could be done here - we've got one in the case of --node-label
already.
Given we no longer publish draino:latest, I'm less slightly less concerned about breaking compatibility at the moment.
This does largely make me feel like a changelog, semantic versioning scheme, and deprecation policy for the project would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can remove the flags that would help to keep a simple code base.
If you prefer I can reintroduce them, they can exist next to the new flags, but that may confuse the users. If we keep them we may add a deprecation notice.
If ok for you I prefer to remove the old flags.
9d75b1a
to
df8c0d9
Compare
Fix issue #98
It is now possible to define eviction exclusion based on the pod ownerReference type. This open the door for more control on pods that are controlled by CRD.
example:
This set of flags replace previous flag:
and adds an exclusion on
kind=ExtendedDaemonSet
The default values have been set to match historical behavior.