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.

Sign up for GitHub

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

Jump to bottom

[occm] Add ability to disable Service controller #1777

Merged
merged 1 commit into from Feb 10, 2022
Merged

[occm] Add ability to disable Service controller #1777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .../openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md
View file
Open in desktop
Expand Up @@ -172,6 +172,10 @@ The options in `Global` section are used for openstack-cloud-controller-manager

Although the openstack-cloud-controller-manager was initially implemented with Neutron-LBaaS support, Octavia is recommended now because Neutron-LBaaS has been deprecated since Queens OpenStack release cycle and no longer accepted new feature enhancements. As a result, lots of advanced features in openstack-cloud-controller-manager rely on Octavia, even the CI is running based on Octavia enabled OpenStack environment. Functionalities are not guaranteed if using Neutron-LBaaS.

* `enabled`
Whether or not to enable the LoadBalancer type of Services integration at all.
Default: true

* `use-octavia`
Whether or not to use Octavia for LoadBalancer type of Service implementation instead of using Neutron-LBaaS. Default: true

Expand Down
7 changes: 7 additions & 0 deletions pkg/openstack/loadbalancer.go
View file
Open in desktop
Expand Up @@ -1910,6 +1910,10 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName

// EnsureLoadBalancer creates a new load balancer or updates the existing one.
func (lbaas *LbaasV2) EnsureLoadBalancer(ctx context.Context, clusterName string, apiService *corev1.Service, nodes []*corev1.Node) (*corev1.LoadBalancerStatus, error) {
if !lbaas.opts.Enabled {
return nil, cloudprovider.ImplementedElsewhere
}

Comment on lines +1913 to +1916
Copy link
Contributor

Choose a reason for hiding this comment

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

Did I understand correctly from the PR description that we can't either:

  • Stop watching Service
  • Entirely disable the Service controller

Because it may need to Reconcile on delete? Is that correct? Is there anything else we still need to reconcile in CPO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any control over Service controller here as it's running in kubernetes/cloud-provider and here we only implement an interface that's called there. There is an explanation on how it's supposed to work here: https://github.com/kubernetes/cloud-provider/blob/master/cloud.go#L117-L132

This seems to makes sense - imagine someone running with enabled: true, creating a few LBs and then switching to enabled: false. It's still expected that when these Services will get deleted, the Octavia LB will get removed, even though the integration is disabled now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdbooth the other resource creation is in other controllers, here we only create LB for LB typed services , so should be ok to return here directly

mc := metrics.NewMetricContext("loadbalancer", "ensure")
status, err := lbaas.ensureLoadBalancer(ctx, clusterName, apiService, nodes)
return status, mc.ObserveReconcile(err)
Expand Down Expand Up @@ -2733,6 +2737,9 @@ func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName

// UpdateLoadBalancer updates hosts under the specified load balancer.
func (lbaas *LbaasV2) UpdateLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service, nodes []*corev1.Node) error {
if !lbaas.opts.Enabled {
return cloudprovider.ImplementedElsewhere
}
mc := metrics.NewMetricContext("loadbalancer", "update")
err := lbaas.updateLoadBalancer(ctx, clusterName, service, nodes)
return mc.ObserveReconcile(err)
Expand Down
2 changes: 2 additions & 0 deletions pkg/openstack/openstack.go
View file
Open in desktop
Expand Up @@ -73,6 +73,7 @@ type LoadBalancer struct {

// LoadBalancerOpts have the options to talk to Neutron LBaaSV2 or Octavia
type LoadBalancerOpts struct {
Enabled bool `gcfg:"enabled"` // if false, disables the controller
LBVersion string `gcfg:"lb-version"` // overrides autodetection. Only support v2.
UseOctavia bool `gcfg:"use-octavia"` // uses Octavia V2 service catalog endpoint
SubnetID string `gcfg:"subnet-id"` // overrides autodetection.
Expand Down Expand Up @@ -190,6 +191,7 @@ func ReadConfig(config io.Reader) (Config, error) {
var cfg Config

// Set default values explicitly
cfg.LoadBalancer.Enabled = true
cfg.LoadBalancer.UseOctavia = true
cfg.LoadBalancer.InternalLB = false
cfg.LoadBalancer.LBProvider = "amphora"
Expand Down