Skip to content

Allow load balancer functionality to be disabled #199

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

Merged
merged 1 commit into from
May 22, 2018

Conversation

harveylowndes
Copy link
Contributor

Resolves: #198

When the load balancer configuration is completely omitted, the load balancer functionality is disabled. @jhorwit2 suggested we look at using resource quotas:

apiVersion: v1 kind: ResourceQuota metadata: name: lb-count spec: hard: services.loadbalancers: "0"

When applied and a load balancer of type service is applied the service is rejected.
Error from server (Forbidden): error when creating "echo-svc.yaml": services "echoheaders" is forbidden: exceeded quota: lb-count, requested: services.loadbalancers=1, used: services.loadbalancers=0, limited: services.loadbalancers=0

Shall we simply leave this alone or include this PR as an additional enhancement?

@harveylowndes harveylowndes requested review from prydie and jhorwit2 May 15, 2018 12:01
@owainlewis
Copy link
Member

owainlewis commented May 18, 2018

The code looks good. Due to the way we're implementing this (testing for nil config rather than an explicit flag) if you want to turn load balancing "on" you're now forced to set "something" in the config. We currently mandate that subnets need to be set in config so it's not a problem but when we remove that requirement then this logic becomes a little bit hard to reason about IMO vs being explicit. For instance I might prefer to not set any LB config and would assume that it would be enabled with defaults rather than being disabled entirely.

By merging this we're saying that the way to "enable" load balancing on is by explicitly setting config for values that already have defaults.

@owainlewis owainlewis self-requested a review May 18, 2018 12:47
@@ -121,3 +121,9 @@ func ReadConfig(r io.Reader) (*Config, error) {

return cfg, nil
}

// LoadBalancerIsDisabled returns a bool based on whether a LoadBalancer
Copy link
Member

Choose a reason for hiding this comment

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

Should be LoadBalancerIsEnabled

@harveylowndes harveylowndes force-pushed the disable-load-balancer branch from 4bbef71 to ed67907 Compare May 18, 2018 14:20
@owainlewis
Copy link
Member

My preference would be for an explicit enable/disable flag in the CCM load balancer config rather than relying on the "magic" absent config trick. This would be easier to reason about and less likely to result in user confusion.

@harveylowndes harveylowndes force-pushed the disable-load-balancer branch from 616e27b to b8515b4 Compare May 22, 2018 09:37
@owainlewis owainlewis self-assigned this May 22, 2018
@owainlewis owainlewis removed request for prydie and jhorwit2 May 22, 2018 10:01
@owainlewis owainlewis merged commit dc782cc into master May 22, 2018
@owainlewis owainlewis deleted the disable-load-balancer branch May 22, 2018 10:02
l-technicore pushed a commit to l-technicore/oci-cloud-controller-manager that referenced this pull request Jun 14, 2022
…m task/OKE-18125 to internal

* commit 'e1cb4074cbf30e2d895207abba29e4c95db799a0':
  Added missing break statement
  Reverted pod start timeout change
  Changed test to make deployment use a single node to spawn pods
  Changed test to exec into restarted pod to check data
  JIRA:task/OKE-18125 added test to check data integrity on pod restart
ayushverma14 pushed a commit to ayushverma14/oci-cloud-controller-manager that referenced this pull request Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants