keikoproj / lifecycle-manager

Graceful AWS scaling event on Kubernetes using lifecycle hooks

Apache License 2.0

93 stars 28 forks source link

Exclude nodes from external load balancers in Kubernetes 1.19 #72

Closed sarahhodne closed 3 years ago

sarahhodne commented 3 years ago

Starting in Kubernetes 1.19, unschedulable nodes were included in load balancers again (see kubernetes/kubernetes#90823), which causes a conflict between the cloud controller and lifecycle-manager, where lifecycle-manager attempts to remove the instance from the load balancer, but then the cloud controller attempts to re-add it.

Kubernetes 1.19 also moved the ServiceNodeExclusion feature gate to beta support, and defaults it to enabled, so you can set the node.kubernetes.io/exclude-from-external-load-balancers label on the node to have the node be excluded. An older version of this feature used the alpha.service-controller.kubernetes.io/exclude-balancer, but this label is removed in Kubernetes 1.19.

This commit sets the new node.kubernetes.io/exclude-from-external-load-balancers label in addition to the older alpha label so lifecycle-manager continues to work on both old and new clusters.

sarahhodne commented 3 years ago

CLA assistant was confusing, "Sign in" apparently means you sign the CLA immediately. I did not mean to sign it yet, since I believe I need to have it signed by my workplace, so please hold off on this for now.

eytan-avisror commented 3 years ago

Thanks @sarahhodne We dont use CLA anymore actually, we need a DCO - so you just need to sign your commits accordingly See here: https://github.com/keikoproj/keiko#news

I will remove the CLA assistant from this repo (I guess we left it)

eytan-avisror commented 3 years ago

Hey @sarahhodne looks like you may need to recreate this PR since it's out-of-date with base (not sure why it's not allowing a rebase on the PR).

We'd ideally like to merge this soon, let me know if you can use the DCO instead of CLA

sarahhodne commented 3 years ago

I'm just checking with legal at $work regarding the DCO, I think that should be fine but I just want their sign-off first. Once I have that, I'll rebase or recreate this PR with the sign-off in the commit.

eytan-avisror commented 3 years ago

I'm just checking with legal at $work regarding the DCO, I think that should be fine but I just want their sign-off first. Once I have that, I'll rebase or recreate this PR with the sign-off in the commit.

OK, sounds good, thanks!

sarahhodne commented 3 years ago

@eytan-avisror Added the DCO signoff to my commit and rebased against latest master so I think this should be ready for a review now.

codecov[bot] commented 3 years ago

Codecov Report

Merging #72 (a994368) into master (d2e9027) will decrease coverage by 0.11%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   69.81%   69.69%   -0.12%     
==========================================
  Files          12       12              
  Lines         911      914       +3     
==========================================
+ Hits          636      637       +1     
- Misses        215      216       +1     
- Partials       60       61       +1

Impacted Files	Coverage Δ
pkg/service/server.go	`59.01% <33.33%> (-0.22%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d2e9027...a994368. Read the comment docs.