cilium-cli icon indicating copy to clipboard operation
cilium-cli copied to clipboard

Evict pods at the end of install command

Open lfundaro opened this issue 4 years ago • 7 comments

This PR tries to make the install command more gentle with a production cluster. Instead of deleting all unmanaged pods, we try to evict them, which essentially achieves a pod deletion but it does so by honoring Pod Disruption Budgets. This way deletions won't cause service outages.

Fixes: #239

Signed-off-by: Lorenzo Fundaró [email protected]

lfundaro avatar May 23 '21 21:05 lfundaro

⚠️ No tests yet, waiting for Authors input first.

lfundaro avatar May 23 '21 21:05 lfundaro

@lfundaro thanks! The PR LGTM.

warning No tests yet, waiting for Authors input first.

I think we can start address testing now. I guess it would help if the logic was split into a dedicated function (for readability if not for testing).

kaworu avatar Jun 04 '21 08:06 kaworu

@lfundaro thanks! The PR LGTM.

warning No tests yet, waiting for Authors input first.

I think we can start address testing now. I guess it would help if the logic was split into a dedicated function (for readability if not for testing).

Hi @kaworu ! I refactored the code and split functions. Also, I started testing some of the newly introduced functions. I'm delaying the test of runEvictions as it's a bit more involved and thought I might discuss with you before. For that function I see two ways of testing:

  1. Mocking the eviction API. A bit involved I think.
  2. Not testing the function per se but rather use CI to run a cilium install and verify that restarting pods honors PDBs.

Those are the ones off the top of my head. What do you think ?

btw, I'm gonna be off the radar for a week, but as soon as I'm back, I can pick up any comments left.

Cheers !

lfundaro avatar Jun 06 '21 21:06 lfundaro

@lfundaro yes using CI make sense to me. /cc @cilium/ci-structure

kaworu avatar Jun 07 '21 07:06 kaworu

Quick high-level question before I look at the code, was this an optional behaviour already, or was pod deletion done unconditionally?

errordeveloper avatar Jun 15 '21 08:06 errordeveloper

Quick high-level question before I look at the code, was this an optional behaviour already, or was pod deletion done unconditionally?

@errordeveloper

cilium install has the option --restart-unmanaged-pods which defaults to true.

basically, you can opt-out from this behavior, otherwise, restart happens.

lfundaro avatar Jun 15 '21 12:06 lfundaro

The build for this PR is failing and there seem to be review comments which haven't been addressed yet. I'll thus convert the PR to draft status. Please mark it as ready for review once everything is addressed.

tklauser avatar Jul 08 '21 17:07 tklauser