Use Octavia #426

hidekazuna · 2019-08-07T00:27:46Z

What this PR does / why we need it:

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

hidekazuna · 2019-08-07T00:37:05Z

Currently this PR is WIP. Bacause this PR created loadbalancer, pool, listener and healthmonitor successfully, but failed to assign floating ip to vip_port_id of loadbalancer.

sbueringer · 2019-08-07T17:29:52Z

pkg/cloud/openstack/cluster/actuator.go

 	if err != nil {
 		return err
 	}
+	loadbalancerService, err := loadbalancer.NewService(osProviderClient, clientOpts, clusterProviderSpec.UseOctavia)


Can you create this services at the start of the method (where networkingService was created before)? It was intended to first create all clients, so that we don't start to do anything if one of them cannot be created (also in the other files)

This method needs clusterProviderSpec.useOcatavia so that must after providerv1.ClusterSpecAndStatusFromProviderSpec.

Oh of course you're right

hidekazuna · 2019-08-10T08:25:16Z

Now I created cluster successfully except for following two changes.

I need to change generate-yaml.sh to use Octavia. floating-network-id should be changed for each environment.

--- a/cmd/clusterctl/examples/openstack/generate-yaml.sh
+++ b/cmd/clusterctl/examples/openstack/generate-yaml.sh
@@ -171,6 +171,14 @@ password=\"$PASSWORD\"
 region=\"$REGION\"
 tenant-id=\"$PROJECT_ID\"
 domain-name=\"$DOMAIN_NAME\"
+[LoadBalancer]
+lb-version=v2
+use-octavia=true
+floating-network-id=5f11f552-7254-47ac-bda3-e8c03b1443cd
+create-monitor=true
+monitor-delay=60s
+monitor-timeout=30s
+monitor-max-retries=5
 "
 
 if [ "$CACERT_ORIGINAL" != "null" ]; then

I need to change controlPlaneEndpoint in master-user-data.sh manually. This is bug, but will be fixed in finalize multi-node control plane implementation #421 .

--- a/cmd/clusterctl/examples/openstack/provider-component/user-data/ubuntu/templates/master-user-data.sh
+++ b/cmd/clusterctl/examples/openstack/provider-component/user-data/ubuntu/templates/master-user-data.sh
@@ -196,7 +196,7 @@ apiServer:
   timeoutForControlPlane: 4m0s
 certificatesDir: /etc/kubernetes/pki
 clusterName: kubernetes
-controlPlaneEndpoint: ${MASTER}
+controlPlaneEndpoint: "10.0.0.214"
 controllerManager:
   extraArgs:
     allocate-node-cidrs: "true"

jichenjc · 2019-08-10T08:32:00Z

i am having similar issue to you on 2)
so let's work together to merge #421 first

hidekazuna · 2019-08-10T08:40:57Z

i am having similar issue to you on 2)
so let's work together to merge #421 first

Yeah, I know second issue is addressed here:
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/421/files#diff-1643cc4766c03f37fef0ea30a352f404R18-R19

Since I have only octavia environment, I hope merging this PR first so that I would test #421 manually in my octavia environment.

sbueringer · 2019-08-10T08:44:43Z

If it makes Octavia deployment possible for you and doesnt break anything else, why not

jichenjc · 2019-08-10T08:52:57Z

/approve

works for me

k8s-ci-robot · 2019-08-10T08:53:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, jichenjc

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:

~~OWNERS~~ [jichenjc]

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

sbueringer

Just some nits. Otherwise lgtm

sbueringer · 2019-08-10T14:24:54Z

pkg/cloud/openstack/services/loadbalancer/loadbalancer.go

 		return errors.New("network.APIServerLoadBalancer is not yet available in clusterProviderStatus")
 	}

 	loadBalancerName := fmt.Sprintf("%s-cluster-%s-%s", networkPrefix, clusterName, kubeapiLBSuffix)


Can you add this log message again? Seems like a rebase mistake to me (concurrent change on master & file move)

I added the log message again due to accidentally removed.

Still removed in the diff :)

Now really added 👍

sbueringer · 2019-08-10T14:25:16Z

pkg/cloud/openstack/services/loadbalancer/loadbalancer.go

-	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/lbaas_v2/loadbalancers"
-	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/lbaas_v2/monitors"
-	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/lbaas_v2/pools"
+	// "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/lbaas_v2/listeners"


I would prefer removing the unused imports

sbueringer · 2019-08-10T14:27:29Z

pkg/cloud/openstack/services/loadbalancer/service.go

+)
+
+const (
+	networkPrefix   string = "k8s"


prefix has been changed to k8s-clusterapi here: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/424/files

Please also change it here

I changed the prefix.

sbueringer · 2019-08-10T14:28:29Z

pkg/cloud/openstack/services/loadbalancer/service.go

+	networkingClient, err := openstack.NewNetworkV2(client, gophercloud.EndpointOpts{
+		Region: clientOpts.RegionName,
+	})
+


please also add error handling here:

if err != nil { return nil, fmt.Errorf("failed to create networking service client: %v", err) }

I added the error handling.

sbueringer · 2019-08-11T08:23:00Z

/lgtm

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. labels Aug 7, 2019

k8s-ci-robot requested review from Lion-Wei and roberthbailey August 7, 2019 00:27

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 7, 2019

sbueringer reviewed Aug 7, 2019

View reviewed changes

hidekazuna force-pushed the use_octavia branch from f8b6cf8 to 8186e43 Compare August 10, 2019 07:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 10, 2019

hidekazuna changed the title ~~WIP: Use Octavia~~ Use Octavia Aug 10, 2019

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 Aug 10, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2019

sbueringer reviewed Aug 10, 2019

View reviewed changes

hidekazuna force-pushed the use_octavia branch from 8186e43 to f0423a5 Compare August 10, 2019 22:01

Use Octavia

5b75178

hidekazuna force-pushed the use_octavia branch from f0423a5 to 5b75178 Compare August 11, 2019 07:47

k8s-ci-robot assigned sbueringer Aug 11, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2019

k8s-ci-robot merged commit 5d49331 into kubernetes-sigs:master Aug 11, 2019

hidekazuna deleted the use_octavia branch February 17, 2020 04:35

hidekazuna mentioned this pull request Jul 27, 2020

🏃 Add hidekazuna to reviewer #597

Merged

EmilienM mentioned this pull request Jan 31, 2023

Manual rebase on upstream openshift/cluster-api-provider-openstack#252

Closed

pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024

Use Octavia (kubernetes-sigs#426)

9f2ec4f

Use Octavia #426

Use Octavia #426

Uh oh!

Conversation

hidekazuna commented Aug 7, 2019

Uh oh!

hidekazuna commented Aug 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidekazuna commented Aug 10, 2019

Uh oh!

jichenjc commented Aug 10, 2019

Uh oh!

hidekazuna commented Aug 10, 2019

Uh oh!

sbueringer commented Aug 10, 2019

Uh oh!

jichenjc commented Aug 10, 2019

Uh oh!

k8s-ci-robot commented Aug 10, 2019

Uh oh!

sbueringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Aug 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbueringer left a comment •

edited

Loading