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

Openstack loadbalancer disablement support #6608

Closed
wants to merge 4 commits into from
Closed

Openstack loadbalancer disablement support #6608

wants to merge 4 commits into from

Conversation

drekle
Copy link
Contributor

@drekle drekle commented Mar 12, 2019

resolves:
#6593
#6592

The loadbalancer can be disabled by not specifiying an api loadbalancer type.
Non loadbalancer deployments will generate a certificate with the 3 master floating IP addresses.
Kubeconfig will choose arbitrarily between each master.

/sig openstack

@k8s-ci-robot k8s-ci-robot added the area/provider/openstack Issues or PRs related to openstack provider label Mar 12, 2019
@k8s-ci-robot k8s-ci-robot requested review from rdrgmnzs and zetaab March 12, 2019 15:41
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2019
This reverts commit 99485e5.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2019
@@ -936,15 +930,24 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e
MaxRetries: fi.Int(3),
},
}
if c.APILoadBalancerType != "" {
// Set defaults for an openstack LB
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe you could move this down to where we make the choice between DNS vs Load Balancers later in the file (around line 1190). It would mean checking again to see if cloud == "openstack", but that's fine...

@@ -221,7 +221,7 @@ func (b *FirewallModelBuilder) addHTTPSRules(c *fi.ModelBuilderContext, sgMap ma
addDirectionalGroupRule(c, masterSG, nodeSG, httpsIngress)
addDirectionalGroupRule(c, masterSG, masterSG, httpsIngress)

if b.UseLoadBalancerForAPI() {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to continue to honor UseLoadBalancerForAPI? This might be more consistent across clouds...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I might be able to do this by addressing your previous comment

Copy link
Contributor Author

@drekle drekle Mar 18, 2019

Choose a reason for hiding this comment

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

It looks like my issues revolve around using public topology without a loadbalancer, as this will initialize the loadbalancer struct.

My understanding of public topology for openstack might mean that either a loadbalancer is created and exposed outside of the openstack environment or floating IP's of the masters, without loadbalancers, are exposed.

My understanding of private topology might mean for me that if designate is supported it will create records for a loadbalancer or the api-servers depending on loadbalancer support, and if designate is not supported to use gossip, which might also be able to leverage loadbalancers.

@zetaab do you have any opinions on this. Unless I am misunderstanding how this ought to work I cannot use this method as I am under the impression that we could do a public and private topology without the use of loadbalancers, however undesirable it may be.

Copy link
Member

Choose a reason for hiding this comment

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

This default openstack setup is kind of "private" if we think how kops works in aws, or aws private subnets works itself. In my opinion in case of openstack the public and private from network and subnet are similar, the only difference is that is api exposed to outside or not? Should we even have terminology private and public in openstack kops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinsb any thoughts here?

I could potentially allow private topology to create a loadbalancer and expose it outside, which seems weird to me. However I cannot specify public topology and use this method to determine if a loadbalancer should be created or not, as this method will always return true under public topology.

Copy link
Contributor Author

@drekle drekle Mar 18, 2019

Choose a reason for hiding this comment

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

I am under the opinion that we not support Kops without loadbalancers. This isn't an elegant way to deploy Kubernetes. This would lack the healthchecks against the API server done by the API loadbalancer. Removing support for this might mean dropping users which don't have these capabilities, forcing them to use kubeadm or a similar tool, however would mean that Kops aim is to truely support production grade deployments.

This PR could simply close.

Copy link
Member

@zetaab zetaab left a comment

Choose a reason for hiding this comment

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

Fast tests trying to create cluster without lb:

  1. I cannot see floatingips in any master?
  2. cluster deletion will throw panic to row https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/openstack/cloud.go#L401

@drekle
Copy link
Contributor Author

drekle commented Mar 15, 2019

/hold

Our openstack environment which I would use to test is under maintenance. Sorry for the delay in updating this.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2019
@drekle
Copy link
Contributor Author

drekle commented Mar 15, 2019

I cannot see floatingips in any master?
cluster deletion will throw panic to row

@zetaab can you confirm that your build was of this branch. These have been addressed for some time and I cannot reproduce on the binary created by this branch. My masters each get floating IP's. My kubeconfig chooses arbitrarily one of these IP's. My cluster deletes successfully.

@yanndegat
Copy link

yanndegat commented Mar 16, 2019

i think there's an issue when deleting clusters
it fails when it runs ListListeneters/ListLBListeners
i had to remove the following lines as a dirty hack

diff --git a/pkg/resources/openstack/openstack.go b/pkg/resources/openstack/openstack.go
index 969de53..5c3fe05 100644
--- a/pkg/resources/openstack/openstack.go
+++ b/pkg/resources/openstack/openstack.go
@@ -47,9 +47,6 @@ func ListResources(cloud openstack.OpenstackCloud, clusterName string) (map[stri
                os.ListInstances,
                os.ListServerGroups,
                os.ListVolumes,
-               os.ListLBListener,
-               os.ListLBPools,
-               os.ListLB,
                os.ListPorts,
                os.ListSecurityGroups,
                os.ListNetwork,

@drekle
Copy link
Contributor Author

drekle commented Mar 18, 2019

i think there's an issue when deleting clusters

This bit is wrong. I've updated this in my branch however will wait to push. Not sure how we want to proceed here.

@drekle drekle closed this Mar 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: drekle
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: gambol99

If they are not already assigned, you can assign the PR to them by writing /assign @gambol99 in a comment when ready.

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

@drekle
Copy link
Contributor Author

drekle commented Mar 19, 2019

Closing. We should put a requirement on loadbalancers in openstack.

@k8s-ci-robot
Copy link
Contributor

@drekle: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kops-verify-gofmt 37b50cf link /test pull-kops-verify-gofmt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants