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

Make evacuate aware of replica set and daemon set #11284

Merged
merged 2 commits into from
Oct 14, 2016

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Oct 10, 2016

@Kargakis this is asking for refactoring desperately and I need to find the appropriate helper in upstream to use for matching labels with unversioned.LabelSelector, but PTAL.

Fixes: #11193

return err
}

dss, err := e.Options.Kclient.DaemonSets(kapi.NamespaceAll).List(kapi.ListOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also include pet set pods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis yeah, which is why i think this need some refactoring love...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pet sets added. @smarterclayton do you know if we have any helper/utility that knows how to match the pods using MatchExpressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis I think we should also check the Job Pods as well... @soltysh mentioned "managed-ref" annotation that all managed pod should set (but there is an concern about backward compatibility where old servers might not set that...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Add Jobs as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soltysh done.

if numPodsWithNoRC > 0 {
err := fmt.Errorf(`Unable to evacuate some pods because they are not backed by replication controller.
if numUnmanagedPods > 0 {
err := fmt.Errorf(`Unable to evacuate some pods because they are not managed by replication controller or replica set or deaemon set.
Copy link
Contributor

Choose a reason for hiding this comment

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

"because they are not managed by any type of controller (replication controller, replica set, daemon set, pet set)" or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@mfojtik
Copy link
Contributor Author

mfojtik commented Oct 10, 2016

[test]

@soltysh
Copy link
Contributor

soltysh commented Oct 11, 2016

LGTM, ship it, once it pass tests. I've created #11310 to make the evacuate code more generic and #11309 for aligning evacuate with kubectl drain.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 010bf86

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9869/)

@soltysh
Copy link
Contributor

soltysh commented Oct 11, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 010bf86

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 14, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10038/) (Base Commit: 6d047d3) (Image: devenv-rhel7_5175)

@openshift-bot openshift-bot merged commit fc9374d into openshift:master Oct 14, 2016
@mfojtik mfojtik deleted the evacuate branch September 5, 2018 21:07
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.

4 participants