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

pkg/daemon: use upstream kubectl/pkg/drain #1571

Merged
merged 1 commit into from Mar 19, 2020

Conversation

runcom
Copy link
Member

@runcom runcom commented Mar 18, 2020

Get rid of openshift/kubernetes-drain and use the upstream kubectl/pkg/drain pkg.
MAO has switched to that as well and it's the right way forward since the old
drain pkg in openshift and cluster-apis obsolete.

Closes #71 also

This is a pre-req to fix https://bugzilla.redhat.com/show_bug.cgi?id=1814241

Signed-off-by: Antonio Murdaca runcom@linux.com

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2020
@runcom
Copy link
Member Author

runcom commented Mar 18, 2020

/retest

Get rid of openshift/kubernetes-drain and use the upstream kubectl/pkg/drain pkg.
MAO has switched to that as well and it's the right way forward since the old
drain pkg in openshift and cluster-apis obsolete.

Signed-off-by: Antonio Murdaca <runcom@linux.com>
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

Can confirm that existing drain behaviour is the same.

@runcom
Copy link
Member Author

runcom commented Mar 18, 2020

/retest

4 similar comments
@runcom
Copy link
Member Author

runcom commented Mar 18, 2020

/retest

@yuqi-zhang
Copy link
Contributor

/retest

@runcom
Copy link
Member Author

runcom commented Mar 19, 2020

/retest

@runcom
Copy link
Member Author

runcom commented Mar 19, 2020

/retest

@sinnykumari
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2020
@sinnykumari
Copy link
Contributor

/test e2e-gcp-op

@sinnykumari
Copy link
Contributor

/test e2e-gcp-upgrade

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, runcom, sinnykumari, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ashcrow,runcom,sinnykumari,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 14b5472 into openshift:master Mar 19, 2020
@runcom runcom deleted the remove-drain-fork branch March 19, 2020 14:54
IgnoreAllDaemonSets: true,
DeleteLocalData: true,
GracePeriodSeconds: -1,
Timeout: 20 * time.Second,
Copy link
Member Author

@runcom runcom Jun 24, 2020

Choose a reason for hiding this comment

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

It seems this isn't enough for certain pods like the router (we bumped that to 90s in master, and MAO still has that at 20s) - this is related to https://bugzilla.redhat.com/show_bug.cgi?id=1843998 and it seems that the router can't be evicted because this drain isn't respecting the 60m graceful termination period and also, the error we get is about not respecting the PDB.

@michaelgugino @enxebre do you know more about this behavior or what is best? This patch regresses the behavior as we were waiting "forever" before this patch, and then only 20s/90s do you have any inputs here?

cc @danehans

Copy link
Contributor

Choose a reason for hiding this comment

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

Timeout here is just how long the drain library will run before giving up (and then drain will be run again on a subsequent requeue).

GracePeriodSeconds is the amount of time that the eviction API will set as grace period if PDBs allow deleting the pod. -1 (IIRC) means use the pod's specified grace period.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update k8s drain lib so we can use stable daemonset API group
8 participants