Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/v1alpha3/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ func TestFuzzyConversion(t *testing.T) {
v1alpha6Cluster.Spec.AllowAllInClusterTraffic = false
v1alpha6Cluster.Spec.DisableAPIServerFloatingIP = false
v1alpha6Cluster.Spec.APIServerLoadBalancer.AllowedCIDRs = nil
v1alpha6Cluster.Spec.APIServerLoadBalancer.Provider = ""
if v1alpha6Cluster.Spec.Bastion != nil {
v1alpha6Cluster.Spec.Bastion.Instance.ImageUUID = ""
v1alpha6Cluster.Spec.Bastion.Instance.Ports = nil
Expand Down
4 changes: 4 additions & 0 deletions api/v1alpha4/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@ func TestFuzzyConversion(t *testing.T) {

v1alpha6Cluster.Spec.APIServerLoadBalancer.AllowedCIDRs = nil

v1alpha6Cluster.Spec.APIServerLoadBalancer.Provider = ""

v1alpha6Cluster.Spec.ControlPlaneOmitAvailabilityZone = false

if v1alpha6Cluster.Spec.Bastion != nil {
Expand Down Expand Up @@ -407,6 +409,8 @@ func TestFuzzyConversion(t *testing.T) {

v1alpha6ClusterTemplate.Spec.Template.Spec.APIServerLoadBalancer.AllowedCIDRs = nil

v1alpha6ClusterTemplate.Spec.Template.Spec.APIServerLoadBalancer.Provider = ""

v1alpha6ClusterTemplate.Spec.Template.Spec.ControlPlaneOmitAvailabilityZone = false

if v1alpha6ClusterTemplate.Spec.Template.Spec.Bastion != nil {
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha5/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,8 @@ func Convert_v1alpha6_OpenStackClusterSpec_To_v1alpha5_OpenStackClusterSpec(in *
// Our new flag has no equivalent in v1alpha5
return autoConvert_v1alpha6_OpenStackClusterSpec_To_v1alpha5_OpenStackClusterSpec(in, out, s)
}

func Convert_v1alpha6_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in *infrav1.APIServerLoadBalancer, out *APIServerLoadBalancer, s conversion.Scope) error {
// Provider has been added in v1alpha6 but has no equivalent in v1alpha5
return autoConvert_v1alpha6_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in, out, s)
}
16 changes: 6 additions & 10 deletions api/v1alpha5/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions api/v1alpha6/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,4 +313,6 @@ type APIServerLoadBalancer struct {
AdditionalPorts []int `json:"additionalPorts,omitempty"`
// AllowedCIDRs restrict access to all API-Server listeners to the given address CIDRs.
AllowedCIDRs []string `json:"allowedCidrs,omitempty"`
// Octavia Provider Used to create load balancer
Provider string `json:"provider,omitempty"`
}
Original file line number Diff line number Diff line change
Expand Up @@ -4283,6 +4283,9 @@ spec:
description: Enabled defines whether a load balancer should be
created.
type: boolean
provider:
description: Octavia Provider Used to create load balancer
type: string
type: object
apiServerPort:
description: APIServerPort is the port on which the listener on the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,9 @@ spec:
description: Enabled defines whether a load balancer should
be created.
type: boolean
provider:
description: Octavia Provider Used to create load balancer
type: string
type: object
apiServerPort:
description: APIServerPort is the port on which the listener
Expand Down
44 changes: 31 additions & 13 deletions pkg/cloud/services/loadbalancer/loadbalancer.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@ import (
)

const (
networkPrefix string = "k8s-clusterapi"
kubeapiLBSuffix string = "kubeapi"
defaultLoadBalancerProvider string = "amphora"
networkPrefix string = "k8s-clusterapi"
kubeapiLBSuffix string = "kubeapi"
)

const loadBalancerProvisioningStatusActive = "ACTIVE"
Expand All @@ -64,13 +63,18 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
return err
}

// As mostly all LoadBalancer features are only supported on "amphora" we explicitly set the provider
// in the LoadBalancer create call to make sure to get the desired features - even if multiple providers exist.
var lbProvider string
for _, v := range providers {
if v.Name == defaultLoadBalancerProvider {
lbProvider = v.Name
break
// Choose the selected provider if it is set in cluster spec, if not, omit the field and Octavia will use the default provider.
lbProvider := ""
if openStackCluster.Spec.APIServerLoadBalancer.Provider != "" {
for _, v := range providers {
if v.Name == openStackCluster.Spec.APIServerLoadBalancer.Provider {
lbProvider = v.Name
break
}
}
if lbProvider == "" {
record.Warnf(openStackCluster, "OctaviaProviderNotFound", "Provider specified for Octavia not found.")
record.Eventf(openStackCluster, "OctaviaProviderNotFound", "Provider %s specified for Octavia not found, using the default provider.", openStackCluster.Spec.APIServerLoadBalancer.Provider)
}
}

Expand Down Expand Up @@ -124,7 +128,7 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
return err
}

pool, err := s.getOrCreatePool(openStackCluster, lbPortObjectsName, listener.ID, lb.ID)
pool, err := s.getOrCreatePool(openStackCluster, lbPortObjectsName, listener.ID, lb.ID, lb.Provider)
if err != nil {
return err
}
Expand Down Expand Up @@ -296,7 +300,7 @@ func validateIPs(openStackCluster *infrav1.OpenStackCluster, definedCIDRs []stri
return marshaledCIDRs
}

func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, poolName, listenerID, lbID string) (*pools.Pool, error) {
func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, poolName, listenerID, lbID, lbProvider string) (*pools.Pool, error) {
pool, err := s.checkIfPoolExists(poolName)
if err != nil {
return nil, err
Expand All @@ -308,12 +312,19 @@ func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, po

s.scope.Logger.Info(fmt.Sprintf("Creating load balancer pool for listener %q", listenerID), "name", poolName, "lb-id", lbID)

method := pools.LBMethodRoundRobin

if lbProvider == "ovn" {
method = pools.LBMethodSourceIpPort
}

poolCreateOpts := pools.CreateOpts{
Name: poolName,
Protocol: "TCP",
LBMethod: pools.LBMethodRoundRobin,
LBMethod: method,
ListenerID: listenerID,
}

pool, err = s.loadbalancerClient.CreatePool(poolCreateOpts)
if err != nil {
record.Warnf(openStackCluster, "FailedCreatePool", "Failed to create pool %s: %v", poolName, err)
Expand Down Expand Up @@ -351,6 +362,13 @@ func (s *Service) getOrCreateMonitor(openStackCluster *infrav1.OpenStackCluster,
Timeout: 5,
}
monitor, err = s.loadbalancerClient.CreateMonitor(monitorCreateOpts)
// Skip creating monitor if it is not supported by Octavia provider
if capoerrors.IsNotImplementedError(err) {
record.Warnf(openStackCluster, "SkippedCreateMonitor", "Health monitors are not supported with the current Octavia provider.")
record.Eventf(openStackCluster, "SkippedCreateMonitor", "Health Monitor is not created as it's not implemented with the current Octavia provider.")
Comment on lines +367 to +368
Copy link
Contributor

Choose a reason for hiding this comment

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

Only now I realized both Warnf() and Eventf() produce an event. We shouldn't need both, just Warnf() should be enough. Obviously that isn't something to fix here, but rather on master branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this is a carbon copy from what was inside of main from the existing v1alpha7 was trying to keep it as close as possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, it should be kept exactly like this. We can fix this on master and then backport the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dulek Do you want to submit that while it's fresh in your mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

return nil
}

if err != nil {
record.Warnf(openStackCluster, "FailedCreateMonitor", "Failed to create monitor %s: %v", monitorName, err)
return err
Expand Down
11 changes: 11 additions & 0 deletions pkg/utils/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,14 @@ func IsConflict(err error) bool {

return false
}

func IsNotImplementedError(err error) bool {
var errUnexpectedResponseCode gophercloud.ErrUnexpectedResponseCode
if errors.As(err, &errUnexpectedResponseCode) {
if errUnexpectedResponseCode.Actual == http.StatusNotImplemented {
return true
}
}

return false
}