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

Move servicelb into cloudprovider #6181

Merged
merged 6 commits into from
Sep 30, 2022
Merged

Conversation

brandond
Copy link
Member

@brandond brandond commented Sep 27, 2022

Proposed Changes

  • Implement the newer, more efficient InstancesV2 interface which doesn't require us to get the node on our own
  • Drop the InformerUser interface, since we are using InstancesV2 or Wrangler controllers and no longer make use of core Kubernetes shared informers.
  • Move servicelb into cloudprovider and implement the LoadBalancer interface with the help of some Wrangler controllers to paper over holes in the upstream integrations. This appears to have been planned for upstream, as cloudprovider.ImplementedElsewhere can be returned most places to indicate a no-op status.
  • Using the core LoadBalancer interface gives us all the normal Service events that other cloud providers expose, handles finalizers, and many other niceties that we were previously hand-rolling.
  • Moving it into the cloud controller means we can easily enable it on RKE2 if we so desire.

Types of Changes

enhancement

Verification

Normal servicelb tests

Testing

Linked Issues

User-Facing Change

The ServiceLB (klipper-lb) service controller is now integrated into the K3s stub cloud controller manager.

Further Comments

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 9.72% // Head: 9.76% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (1ff3682) compared to base (0d75d74).
Patch coverage: 1.86% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #6181      +/-   ##
=========================================
+ Coverage    9.72%   9.76%   +0.04%     
=========================================
  Files         138     139       +1     
  Lines       10061   10100      +39     
=========================================
+ Hits          978     986       +8     
- Misses       8884    8912      +28     
- Partials      199     202       +3     
Flag Coverage Δ
unittests 9.76% <1.86%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cli/server/server.go 0.00% <0.00%> (ø)
pkg/cloudprovider/cloudprovider.go 0.00% <0.00%> (ø)
pkg/cloudprovider/instances.go 0.00% <0.00%> (ø)
pkg/cloudprovider/loadbalancer.go 0.00% <0.00%> (ø)
pkg/cloudprovider/servicelb.go 0.00% <0.00%> (ø)
pkg/daemons/agent/agent_linux.go 0.00% <0.00%> (ø)
pkg/daemons/config/types.go 64.91% <ø> (ø)
pkg/daemons/control/server.go 0.00% <0.00%> (ø)
pkg/daemons/executor/embed.go 2.08% <0.00%> (+0.12%) ⬆️
pkg/deploy/zz_generated_bindata.go 0.00% <ø> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brandond brandond force-pushed the servicelb_ccm branch 4 times, most recently from 87430d5 to 433a276 Compare September 27, 2022 20:36
@brandond brandond marked this pull request as ready for review September 27, 2022 20:39
@brandond brandond requested a review from a team as a code owner September 27, 2022 20:39
Copy link
Contributor

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

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

This is very nice! How could we use this in rke2? I guess by setting cloud=provider-external in kubelet but then we should somehow register this cloud-provider, right?

pkg/cloudprovider/cloudprovider.go Show resolved Hide resolved
pkg/cloudprovider/cloudprovider.go Show resolved Hide resolved
pkg/daemons/control/deps/deps.go Show resolved Hide resolved
@brandond
Copy link
Member Author

brandond commented Sep 28, 2022

How could we use this in rke2

  1. Update the k3s version in https://github.com/rancher/image-build-rke2-cloud-provider and cut a new tag with the changes
  2. Update k3s in rke2
  3. Update the rke2 CLI wrapper so that servicelb isn't always disabled
  4. Update the rke2 RBAC helpers to sync the CCM RBAC changes from k3s
  5. Update the rke2-cloud-provider image tag in rke2

... and drop legacy ClusterID support.

Signed-off-by: Brad Davidson <[email protected]>
@brandond brandond force-pushed the servicelb_ccm branch 2 times, most recently from 95f7755 to 78c38f5 Compare September 29, 2022 21:27
@brandond brandond force-pushed the servicelb_ccm branch 4 times, most recently from aa42659 to 98f75e9 Compare September 30, 2022 03:01
If CCM and ServiceLB are both disabled, don't run the cloud-controller-manager at all;
this should provide the same CLI flag behavior as previous releases, and not create
problems when users disable the CCM but still want ServiceLB.

Signed-off-by: Brad Davidson <[email protected]>
@brandond brandond force-pushed the servicelb_ccm branch 2 times, most recently from 8d30793 to 3bcd985 Compare September 30, 2022 09:14
@brandond brandond merged commit 0270395 into k3s-io:master Sep 30, 2022
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