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

if loadbalancer section is not defined in cloudconfig, do not init support #65781

Merged
merged 2 commits into from
Jul 10, 2018
Merged

if loadbalancer section is not defined in cloudconfig, do not init support #65781

merged 2 commits into from
Jul 10, 2018

Conversation

zetaab
Copy link
Member

@zetaab zetaab commented Jul 3, 2018 •
edited
Loading

What this PR does / why we need it: if LoadBalancer section is not defined in cloudconfig, we should not initialize loadbalancer support for openstack cloudprovider.

Which issue(s) this PR fixes:
Fixes #65775

Special notes for your reviewer:

Release note:

If Openstack LoadBalancer is not defined in cloud config, the loadbalancer is not initialized any more in openstack. All setups must have some setting under that section

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 3, 2018
@k8s-ci-robot k8s-ci-robot requested review from anguslees and dims July 3, 2018 20:26
@zetaab
if loadbalancer section is not defined in cloudconfig, do not initial…
a78fc59 
…ize lb support

use reflect because structs with []string cannot be compared
@zetaab
Copy link
Member Author

zetaab commented Jul 3, 2018 •
edited
Loading

one thing that I am thinking: is there possibility to use openstack lbaas currently with empty config? I tried to look opts in https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack.go#L93-L105 and for me it looks like all fields are optional? https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#openstack So if we merge this PR, it might break things in some users. Any ideas how we can really check is this section added to config or not? Gfg does not have feature to check "does section exist"?

FengyunPan2
FengyunPan2 approved these changes Jul 4, 2018
View reviewed changes
Copy link
Contributor

@FengyunPan2 FengyunPan2 left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 4, 2018
@neolit123
Copy link
Member

/kind bug
@kubernetes/sig-cloud-provider-pr-reviews

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Jul 5, 2018
@dims
Copy link
Member

dims commented Jul 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, FengyunPan2, zetaab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@andrewsykim
Copy link
Member

andrewsykim commented Jul 10, 2018 •
edited
Loading

@zetaab can you update the release notes to say If OpenStack LoadBalancer... (or [OpenStack] If Loadbalancer... just so it's bit clearer this change is only applicable for OpenStack.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65902, 65781). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 0ff24a2 into kubernetes:master Jul 10, 2018
@k8s-ci-robot k8s-ci-robot added the area/provider/openstack Issues or PRs related to openstack provider label Aug 1, 2018
@m-wcislo m-wcislo mentioned this pull request Dec 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FengyunPan2 FengyunPan2 FengyunPan2 approved these changes

@anguslees anguslees Awaiting requested review from anguslees

@dims dims Awaiting requested review from dims

Assignees

@dims dims

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support enabling BlockStorage but disabling LoadBalancer in cloud provider
7 participants
@zetaab @neolit123 @dims @k8s-ci-robot @andrewsykim @k8s-github-robot @FengyunPan2