[occm] Allow ip address sharing for services of type loadbalancers

chrischdi commented 4 years ago

Uncomment only one, leave it on its own line:

/kind bug /kind feature

The binaries affected:

[x] openstack-cloud-controller-manager
[ ] cinder-csi-plugin
[ ] k8s-keystone-auth
[ ] client-keystone-auth
[ ] octavia-ingress-controller
[ ] manila-csi-plugin
[ ] manila-provisioner
[ ] magnum-auto-healer
[ ] barbican-kms-plugin

What happened:

Created two services of type: loadBalancer having set the same svc.spec.loadBalancerIP
No listener, pool, healthmonitor or members are created for the second service:
- kubectl describe svc shows: Warning SyncLoadBalancerFailed 2m32s (x6 over 5m9s) service-controller Error syncing load balancer: failed to ensure load balancer: floating IP X.X.X.X is not available

What you expected to happen:

Controller will add a new listener, pool, healthmonitor and members for the additional port/nodeport to the existing loadbalancer having floating IP X.X.X.X

How to reproduce it:

$ cat service.yaml 
apiVersion: v1
kind: Service
metadata:
  name: kuard-1
spec:
  type: LoadBalancer
  loadBalancerIP: 192.168.200.5
  ports:
  - port: 80
    protocol: TCP
    targetPort: 8080
  selector:
    app: kuard
$ cat service2.yaml 
apiVersion: v1
kind: Service
metadata:
  name: kuard-2
spec:
  type: LoadBalancer
  loadBalancerIP: 192.168.200.5
  ports:
  - port: 8080
    protocol: TCP
    targetPort: 8080
  selector:
    app: kuard
$ kubectl apply -f service.yaml -f service2.yaml

Anything else we need to know?:

Metall LB implements this feature: docs
We could implement it similar to Metall LB and toggle it via an annotation on services analog to metallb.universe.tf/allow-shared-ip

What get's touched by implementing it:

The creation and reconcile of a loadbalancer by the service name may not work.
- The above exampe creates the following loadbalancers:
```
+--------------------------------------+-----------------------------------------+-------------+---------------------+------------+
| id                                   | name                                    | vip_address | provisioning_status | provider   |
+--------------------------------------+-----------------------------------------+-------------+---------------------+------------+
| cd117dcf-518e-4a34-8613-d2ba29b3f9e2 | kube_service_kubernetes_default_kuard-1 | 10.6.0.32   | ACTIVE              | vmwareedge |
| df2281b6-6c0b-46a2-bfbd-6566b5697372 | kube_service_kubernetes_default_kuard-2 | 10.6.0.10   | ACTIVE              | vmwareedge |
```
  We would need to adjust the following code to have a common name for mutliple services for the loadBalancer, or detect the loadBalancer by other loadBalancer metadata: https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go#L520
We could enforce the need to set a loadBalancerIP when the ip-address-sharing annotation is set and resolve the name for the loadBalancer by using the loadBalancerIP by using for example something like kube_service_x-x-x-x for the loadBalancer name
We would need some additional validation for e.g.:
- do not allow multiple use of the external port
- potential other invalid options when merging multiple services of type loadBalancer (e.g. annotations)

There may be other things I didn't consider here.

Environment:

openstack-cloud-controller-manager version: master
OpenStack version: Queens
Others: Kubernetes v1.16

kayrus commented 4 years ago

Sorry for advertising this, but it may help you: https://github.com/kayrus/ingress-terraform

lingxiankong commented 4 years ago

@chrischdi, thanks for proposing this feature request.

sbueringer commented 4 years ago

Just some input. I think we shouldn't force the user to define the loadBalancerIP because he might not know which IPs are available in OpenStack. An alternative solution for the name would be in my opinion to don't set a name and add all associated services to the tags of the LB resources. This would be the easiest from a user perspective.

chrischdi commented 4 years ago

Just some input. I think we shouldn't force the user to define the loadBalancerIP because he might not know which IPs are available in OpenStack. An alternative solution for the name would be in my opinion to don't set a name and add all associated services to the tags of the LB resources. This would be the easiest from a user perspective.

Or use the metadata/tags for creation of the lb and after knowing the ip address we could rename the loadbalancer

Other alternative: use a second annotation which needs to define a unique name, which then will be shared on all services which should share the loadbalancer.

hamzazafar commented 4 years ago

Just some input. I think we shouldn't force the user to define the loadBalancerIP because he might not know which IPs are available in OpenStack. An alternative solution for the name would be in my opinion to don't set a name and add all associated services to the tags of the LB resources. This would be the easiest from a user perspective.

Or use the metadata/tags for creation of the lb and after knowing the ip address we could rename the loadbalancer

Other alternative: use a second annotation which needs to define a unique name, which then will be shared on all services which should share the loadbalancer.

Or why not simply pass the ID of external loadbalancer in an annotation. Workflow: Create 1st service -> External LB gets created -> Get ID of LB using OpenStack CLI -> Create 2nd service and pass ID of existing LB in an annotation

This requires adding a new function to get loadbalancer by ID.

Some additional things to take care of:

Deletion case: When you delete service 1 OCCM will delete all resources of the loadbalancer, OCCM doesn't know that any other services are also using the same loadbalancer.
Update case: What happens when you (accidentally or intentionally) set InternalLB annotation for service 2? should OCCM remove the floating ip attached to LB?

hamzazafar commented 4 years ago

Currently OCCM deletes all obsolete resources (e.g listeners attached to a loadbalancer that don't correspond to any ports defined in a service): https://github.com/kubernetes/cloud-provider-openstack/blob/27b70e3ded626783dbb0d83e1a58dd5cacbed37d/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go#L1181

In view of a service, all resources (listeners, pools, members etc.) created by other services will be considered as obsolete and eventually deleted.

A lot of refactoring is required to support this feature.

chrigl commented 4 years ago

Tags are unfortunately not an option as long as we support Neutron LBaaS v2 and Octavia < 2.5 :/

sbueringer commented 4 years ago

@lingxiankong @hamzazafar @chrigl

If I/we would commit to implementing this, is there interest in merging this if we come up with a feasible solution?

I think the main problems are:

there's a lot of code to refactor for this
we have to come up with a name or something for the LB. I think it's a little bit awkward to name the LB after one of the two services and then when the wrong one is deleted it has the name of a non-existing service. So might be the best solution to just give the LB no name or a UUID and find some other way, e.g. via an annotation or the loadBalancerIP to map the Kubernetes Service to the loadbalancer. And if the "lb sharing" is not activated we can always fall back to the current implementation

So what I want to clarify is more or less. Is there a chance that this will go upstream, because if not we can just design it for our internal use-cases and implement it on a fork. Of course I would rather like to implement it upstream

lingxiankong commented 4 years ago

@sbueringer I still have concerns that this feature may break the stability of openstack cloud controller manager, as you said, lots of refactoring is needed. But thank you so much for bringing this discussion.

hamzazafar commented 4 years ago

@sbueringer Just like other people, I am also very much interested in this feature. I think we should refactor code base in small chunks, in this way we can detect issues earlier.

@lingxiankong what do you think ?

lingxiankong commented 4 years ago

I think we should refactor code base in small chunks

I think this could work

rochaporto commented 4 years ago

@sbueringer do you already have some code for this? We're very interested in this feature and have some downstream prototype code.

sbueringer commented 4 years ago

@rochaporto We don't have any code yet, but we wanted to start developing it at some point in the next few weeks when we get to it. We would definitely be interested to take a look at your code if possible :)

/cc @chrischdi

lingxiankong commented 4 years ago

FYI, please see a draft design for this feature at https://github.com/kubernetes/cloud-provider-openstack/pull/1118#issuecomment-665938534

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot commented 3 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

chrischdi commented 3 years ago

/remove-lifecycle rotte

fejta-bot commented 3 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /close

k8s-ci-robot commented 3 years ago

@fejta-bot: Closing this issue.

In response to [this](https://github.com/kubernetes/cloud-provider-openstack/issues/933#issuecomment-751438969): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta). >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

chrischdi commented 3 years ago

/reopen /remove-lifecycle rotten

k8s-ci-robot commented 3 years ago

@chrischdi: Reopened this issue.

In response to [this](https://github.com/kubernetes/cloud-provider-openstack/issues/933#issuecomment-784981631): >/reopen >/remove-lifecycle rotten Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

fejta-bot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

fejta-bot commented 3 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten

k8s-triage-robot commented 3 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community. /close

k8s-ci-robot commented 3 years ago

@k8s-triage-robot: Closing this issue.

In response to [this](https://github.com/kubernetes/cloud-provider-openstack/issues/933#issuecomment-886996601): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

rissson commented 3 years ago

I think this is still relevant, as many people expressed their interest for it.

lingxiankong commented 3 years ago

I think this is still relevant, as many people expressed their interest for it.

Thanks for your interest, I will continue implementing this feature.

lingxiankong commented 3 years ago

/assign

lingxiankong commented 3 years ago

/reopen

lingxiankong commented 3 years ago

/remove-lifecycle rotten

kubernetes / cloud-provider-openstack

[occm] Allow ip address sharing for services of type loadbalancers #933