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

filter out pod in eviction using pod ownerreference kind #99

Conversation

dbenque
Copy link
Contributor

@dbenque dbenque commented Oct 1, 2020

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:

        --do-not-evict-pod-controlled-by=StatefulSet
        --do-not-evict-pod-controlled-by=DaemonSet
        --do-not-evict-pod-controlled-by=ExtendedDaemonSet
        --do-not-evict-pod-controlled-by=

This set of flags replace previous flag:

       --evict-daemonset-pods=false
       --evict-statefulset-pods=false
       --evict-unreplicated-pods=false

and adds an exclusion on kind=ExtendedDaemonSet

The default values have been set to match historical behavior.

@dbenque dbenque force-pushed the david.benque/issue98-do-not-evict-pod-controlled-by branch from c82de02 to 592b491 Compare October 1, 2020 11:41
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

❗ No coverage uploaded for pull request head (david.benque/issue98-do-not-evict-pod-controlled-by@df8c0d9). Click here to learn what that means.
The diff coverage is n/a.

@dbenque
Copy link
Contributor Author

dbenque commented Oct 1, 2020

@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 util.go in the meantime.

@dbenque dbenque force-pushed the david.benque/issue98-do-not-evict-pod-controlled-by branch from 592b491 to 365a458 Compare October 1, 2020 13:39
@jacobstr
Copy link
Contributor

jacobstr commented Oct 1, 2020

@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.

@dbenque
Copy link
Contributor Author

dbenque commented Oct 6, 2020

@jacobstr I don't know if you did something, but now I have access to the report.
It is going to be difficult to add more relevant test.
What is the policy here? are you going to block progress/merge under a given threshold ?

Copy link
Contributor

@jacobstr jacobstr left a 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.

cmd/draino/draino.go Outdated Show resolved Hide resolved
cmd/draino/draino.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internal/kubernetes/drainer_test.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -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()
Copy link
Contributor

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.

Copy link
Contributor Author

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.

internal/kubernetes/podfilters.go Outdated Show resolved Hide resolved
@dbenque dbenque force-pushed the david.benque/issue98-do-not-evict-pod-controlled-by branch from 9d75b1a to df8c0d9 Compare October 21, 2020 10:27
@dbenque dbenque requested a review from jacobstr October 21, 2020 10:28
@dbenque dbenque closed this Dec 29, 2022
@dbenque dbenque deleted the david.benque/issue98-do-not-evict-pod-controlled-by branch December 29, 2022 08:29
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.

2 participants