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

Harvester cloud provider enhancement #32

Merged
merged 5 commits into from
Jun 7, 2023
Merged

Harvester cloud provider enhancement #32

merged 5 commits into from
Jun 7, 2023

Conversation

yaocw2020
Copy link
Contributor

  • Upgrade to load balancer v1beta1
  • Support share load balancer between services

Related issue: harvester/harvester#1762 harvester/harvester#2772

@yaocw2020
Copy link
Contributor Author

Merge after harvester/load-balancer-harvester#13

@yaocw2020 yaocw2020 mentioned this pull request Apr 26, 2023
Closed
@yaocw2020 yaocw2020 force-pushed the v1beta1 branch 2 times, most recently from daef3a3 to 86256bb Compare April 26, 2023 09:32
@yaocw2020 yaocw2020 changed the title Harvester cloud provider enhancement [WIP]Harvester cloud provider enhancement Apr 26, 2023
@yaocw2020 yaocw2020 force-pushed the v1beta1 branch 2 times, most recently from 85566f9 to 2d23106 Compare April 26, 2023 12:15
@yaocw2020 yaocw2020 changed the title [WIP]Harvester cloud provider enhancement Harvester cloud provider enhancement Apr 28, 2023
w13915984028
w13915984028 reviewed Apr 28, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

One general question is about the Primay and Secondary services, which scenario will require it ?

@yaocw2020
Copy link
Contributor Author

One general question is about the Primay and Secondary services, which scenario will require it ?

harvester/harvester#2772

w13915984028
w13915984028 reviewed Apr 28, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

Still needs a better clarification of the scenario of the primary and secondary service

how will the user create/configure and use this feature.

pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
@yaocw2020
Copy link
Contributor Author

yaocw2020 commented Apr 28, 2023 •
edited
Loading

Still needs a better clarification of the scenario of the primary and secondary service

how will the user create/configure and use this feature.

The UI is working in progress and I'll add the document later. Users add annotation cloudprovider.harvesterhci.io/primary-service to specify the primary service for the secondary service. Then, secondary service can share the load balancer of the primary service. It's allowed to access both the primary service and secondary service via the same load balancer IP.

@w13915984028
Copy link
Member

Still needs a better clarification of the scenario of the primary and secondary service
how will the user create/configure and use this feature.

The UI is working in progress and I'll add the document later. Users add annotation cloudprovider.harvesterhci.io/primary-service to specify the primary service for the secondary service. Then, secondary service can share the load balancer of the primary service. It's allowed to access both the primary service and secondary service via the same load balancer IP.

Primay + Secondary may not be enough for more complicated scenarios, could be Primary + Secondaries

I guess, the users may want to use one VIP for all, for each PORT/each group of PORTs, it can specify a kind of service. But it should be in a further enhancement.

Please Martin-Weiss confirm if the current design can meet your requirement in harvester/harvester#2772, thanks.

w13915984028
w13915984028 approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

w13915984028
w13915984028 approved these changes May 8, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@n313893254 n313893254 mentioned this pull request May 10, 2023
Merged
@guangbochen guangbochen requested review from futuretea and removed request for bk201 May 11, 2023 03:26
guangbochen
guangbochen reviewed May 11, 2023
View reviewed changes
deploy/generate_addon.sh Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Show resolved Hide resolved
starbops
starbops reviewed May 12, 2023
View reviewed changes
deploy/generate_addon.sh Outdated Show resolved Hide resolved
futuretea
futuretea reviewed May 12, 2023
View reviewed changes
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
pkg/cloud-controller-manager/loadbalancer.go Outdated Show resolved Hide resolved
@guangbochen guangbochen mentioned this pull request May 29, 2023
Closed
chrisho
chrisho approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@chrisho chrisho left a comment

Choose a reason for hiding this comment

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

LGTM and it works fine. I've tested including DHCP/Range of IP pool, primary and secondary services.
image
image

starbops
starbops approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@starbops starbops left a comment

Choose a reason for hiding this comment

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

Tested both DHCP & Pool IPAMs, LGTM! Thanks.

futuretea

This comment was marked as duplicate.

Sign in to view

futuretea
futuretea approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@futuretea futuretea left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

guangbochen
guangbochen reviewed Jun 6, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@yaocw2020
Support sharing load balancer with other services
97d5df9 
Define two service types
- Primary service is the load balancer service which will be used to create the load balancer
- Secondary service is the load balancer service which will share the load balancer created by the primary service.
The ccm watches a secondary service, it will not create a load balancer, but get the specify primary service from annotation `cloudprovider.harvesterhci.io/primary-service`. The primary service will share its load balancer with the secondary service. Since the secondary service will have the same load balancer IP with the primary service and the service address format is IP:port, secondary service ports must not be used in the primary service to avoid service address conflict.
@yaocw2020
Update go.mod
8148a2c
@yaocw2020
Update vendor
8184f00
w13915984028
w13915984028 approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@guangbochen guangbochen merged commit 3da7184 into harvester:master Jun 7, 2023
@guangbochen guangbochen mentioned this pull request Jul 10, 2023
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@starbops starbops starbops approved these changes

@chrisho chrisho chrisho approved these changes

@guangbochen guangbochen guangbochen approved these changes

@futuretea futuretea futuretea approved these changes

@w13915984028 w13915984028 w13915984028 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yaocw2020 @w13915984028 @starbops @chrisho @guangbochen @futuretea