Support for NSX-T based Load Balancers by mandelsoft · Pull Request #292 · kubernetes/cloud-provider-vsphere

mandelsoft · 2020-01-27T16:21:26Z

What this PR does / why we need it:
This PR adds support for the NSX-T loadbalancers to the vsphere cloud-controller-manager.
It must explicitly be enabled by additional configuration options in the configuration file.

If enabled, it manages the load balancer service and virtual servers for TCP/UDP kubernetes services, including health checks.
It also includes a cleanup feature for deleting/releasing obsolete NSX-T elements of formerly available services in the case of a crash recovery or services deleted without a running controller manager.
By configuring dedicated load balancer classes, it is possible to refer by annotations to various IP Pools for the allocated IP of the virtual server.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

k8s-ci-robot · 2020-01-27T16:21:35Z

Welcome @mandelsoft!

It looks like this is your first PR to kubernetes/cloud-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-01-27T16:21:35Z

Hi @mandelsoft. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

andrewsykim · 2020-01-27T16:24:20Z

/assign

andrewsykim

Did a first pass, just left some minor comments will go through it again soon

cmd/vsphere-cloud-controller-manager/main.go

pkg/cloudprovider/vsphere/cloud.go

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go

go.mod

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go

pkg/cloudprovider/vsphere/loadbalancer/config/config.go

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go

pkg/cloudprovider/vsphere/loadbalancer/interface.go

pkg/cloudprovider/vsphere/loadbalancer/nsxt_broker.go

andrewsykim · 2020-01-30T18:13:50Z

/assign @yastij

pkg/cloudprovider/vsphere/loadbalancer/config/config.go

pkg/cloudprovider/vsphere/loadbalancer/config/config_test.go

pkg/cloudprovider/vsphere/loadbalancer/config/config.go

pkg/cloudprovider/vsphere/loadbalancer/helpers.go

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go

pkg/cloudprovider/vsphere/loadbalancer/interface.go

pkg/cloudprovider/vsphere/loadbalancer/reorg.go

mandelsoft · 2020-02-06T14:51:50Z

@andrewsykim the dedicated lock per service might be useful to avoid the global reorg go routine to block all service reconcilations

andrewsykim · 2020-02-20T16:06:38Z

/unassign @yastij

andrewsykim · 2020-02-20T16:07:01Z

/assign

andrewsykim

Some more comments, thanks again for driving this

go.mod

pkg/cloudprovider/vsphere/cloud.go

pkg/cloudprovider/vsphere/loadbalancer/README.md

pkg/cloudprovider/vsphere/loadbalancer/access.go

pkg/cloudprovider/vsphere/loadbalancer/config/config_test.go

pkg/cloudprovider/vsphere/loadbalancer/helpers.go

pkg/cloudprovider/vsphere/vapilogger.go

pkg/cloudprovider/vsphere/loadbalancer/README.md

andrewsykim

Overall I think this is looking good, I still don't understand the use-case around ReorgServices, can you expand on that please?

go.mod

go.sum

pkg/cloudprovider/vsphere/loadbalancer/README.md

pkg/cloudprovider/vsphere/loadbalancer/interface.go

andrewsykim

/ok-to-test

A few more comments, the most important is getting the feature gate there. Otherwise I'm okay to merge and iterate on the other comments as a follow-up

andrewsykim · 2020-02-25T14:08:03Z

cmd/vsphere-cloud-controller-manager/main.go

 	pflag.CommandLine.VisitAll(func(flag *pflag.Flag) {
-		if flag.Name == "version" {
+		switch flag.Name {
+		case "cluster-name":


nit: I actually prefer this to be cluster-id to match with the cluster ID flag in the CSI driver

this is a predefined flag from Kubernetes and cannot be changed.
see https://github.com/kubernetes/kubernetes/blob/2652e176e0a24ed9a5d90cc9b3d670c0d4ed8720/cmd/controller-manager/app/options/kubecloudshared.go#L60

I prefer we don't use this flag, it has some implications that aren't so obvious (the flag description is already telling).

I think what would be best is adding a cluster-id field to the global config https://github.com/kubernetes/cloud-provider-vsphere/blob/master/pkg/common/config/types.go#L21 and then using that value for "cluster name" for LBs.

introduced new flag cluster-id

The reference docu for the cloud-controller maneger (https://kubernetes.io/docs/reference/command-line-tools-reference/cloud-controller-manager/) explains the option cluster-namewith

--cluster-name string Default: "kubernetes"
The instance prefix for the cluster.

which is exactly the use case we want to use. Therefore it really seems to be the right choice. It is also already passed by the load balancer interface of the cloud controller manager. That's another clear indicator that we are using it as intended.

pkg/cloudprovider/vsphere/loadbalancer/README.md

go.mod

pkg/cloudprovider/vsphere/loadbalancer/README.md

pkg/cloudprovider/vsphere/cloud.go

pkg/cloudprovider/vsphere/loadbalancer/access.go

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go

pkg/cloudprovider/vsphere/loadbalancer/reorg.go

pkg/cloudprovider/vsphere/loadbalancer/state.go

andrewsykim

Just a few more comments, almost there :) Appreciate your patience.

go.mod

pkg/cloudprovider/vsphere/cloud.go

andrewsykim

/approve

One more minor comment, lgtm otherwise!

andrewsykim · 2020-02-29T01:59:49Z

pkg/cloudprovider/vsphere/cloud.go

+	if lb == nil {
+		klog.Infof("NSX-T load balancer support disabled")
+	} else {
+		klog.Infof("NSX-T load balancer support enabled")


Can we add a warning log here saying something like "this feature is alpha, use in production at your own risk"

I second this as well. Also since this is an alpha feature, I would also recommend adding something to the effect of "since this is an alpha feature, this implementation is a work in progress and the underlying implementation details can change."

Changed logging to klog.Infof("NSX-T load balancer support enabled. This feature is alpha, use in production at your own risk."). Also added the sentence "since this is an alpha feature... can change." to the README.md.

k8s-ci-robot · 2020-02-29T02:03:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewsykim]

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

andrewsykim · 2020-02-29T02:05:48Z

also if you can squash commits that'd be great!

andrewsykim · 2020-03-02T15:18:56Z

Can we update this PR to be a normal PR and not a draft? I don't think it'll merge otherwise

davidvonthenen · 2020-03-02T16:39:13Z

I left a comment as well, but looks good to me as an initial commit! Nice job!

davidvonthenen · 2020-03-02T20:17:03Z

/lgtm

andrewsykim · 2020-03-03T21:45:08Z

This will merge automatically once it's no longer a draft

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 27, 2020

k8s-ci-robot requested review from BaluDontu and davidvonthenen January 27, 2020 16:22

k8s-ci-robot assigned andrewsykim Jan 27, 2020

andrewsykim reviewed Jan 27, 2020

View reviewed changes

maplain reviewed Jan 27, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/loadbalancer/lbprovider.go Show resolved Hide resolved

maplain reviewed Jan 27, 2020

View reviewed changes

pkg/cloudprovider/vsphere/loadbalancer/interface.go Outdated Show resolved Hide resolved

maplain reviewed Jan 27, 2020

View reviewed changes

pkg/cloudprovider/vsphere/loadbalancer/nsxt_broker.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned yastij Jan 30, 2020

MartinWeindel mentioned this pull request Feb 3, 2020

Install docs? gardener-attic/nsxt-lb-provider#9

Open

andrewsykim reviewed Feb 3, 2020

View reviewed changes

k8s-ci-robot unassigned yastij Feb 20, 2020

andrewsykim reviewed Feb 20, 2020

View reviewed changes

maplain reviewed Feb 20, 2020

View reviewed changes

pkg/cloudprovider/vsphere/loadbalancer/README.md Show resolved Hide resolved

MartinWeindel force-pushed the master branch from dee4264 to 7678bbf Compare February 21, 2020 08:59

andrewsykim reviewed Feb 21, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

go.sum Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/loadbalancer/README.md Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/loadbalancer/interface.go Outdated Show resolved Hide resolved

mandelsoft mentioned this pull request Feb 25, 2020

What benefit do we get from this instead of using sync.Mutex and locking against all keys? Is it for performance reasons so we can operate against multiple keys concurrently or something else? #300

Closed

mandelsoft force-pushed the master branch from ff741bf to 91aee1e Compare February 25, 2020 10:56

andrewsykim reviewed Feb 25, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020

MartinWeindel force-pushed the master branch from 1aa736a to f5e0493 Compare February 27, 2020 12:32

andrewsykim reviewed Feb 27, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/cloud.go Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/cloud.go Show resolved Hide resolved

MartinWeindel force-pushed the master branch 2 times, most recently from 898630f to 4b557e4 Compare February 28, 2020 12:48

mandelsoft force-pushed the master branch from 390e371 to 9eaf341 Compare February 28, 2020 14:03

andrewsykim reviewed Feb 29, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 29, 2020

MartinWeindel force-pushed the master branch 2 times, most recently from 31e2720 to dca6a2f Compare March 2, 2020 07:49

NSX-T loadbalancer support

fc81964

MartinWeindel force-pushed the master branch from dca6a2f to fc81964 Compare March 2, 2020 17:13

k8s-ci-robot assigned davidvonthenen Mar 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2020

mandelsoft marked this pull request as ready for review March 4, 2020 12:53

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020

k8s-ci-robot merged commit 7d77c09 into kubernetes:master Mar 4, 2020

onesolpark mentioned this pull request Mar 18, 2020

Version compatibility for NSX-T LoadBalancer #306

Closed

andrewsykim mentioned this pull request Apr 14, 2020

[WIP] add experimental support for Service Type=LoadBalancer w/ NSX-T #272

Closed

YanzhaoLi mentioned this pull request Oct 3, 2023

Create pluggable mechanism for service type LoadBalancer in CPI #279

Open

Conversation

mandelsoft commented Jan 27, 2020

Uh oh!

k8s-ci-robot commented Jan 27, 2020

Uh oh!

k8s-ci-robot commented Jan 27, 2020

Uh oh!

andrewsykim commented Jan 27, 2020

Uh oh!

andrewsykim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsykim commented Jan 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mandelsoft commented Feb 6, 2020

Uh oh!

andrewsykim commented Feb 20, 2020

Uh oh!

andrewsykim commented Feb 20, 2020

Uh oh!

andrewsykim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsykim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewsykim left a comment

Choose a reason for hiding this comment

Uh oh!

andrewsykim Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

MartinWeindel Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

andrewsykim Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

MartinWeindel Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

mandelsoft Feb 28, 2020 •

edited

Loading

davidvonthenen Mar 2, 2020 •

edited

Loading

andrewsykim commented Mar 2, 2020 •

edited

Loading

andrewsykim commented Mar 3, 2020 •

edited

Loading