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

SPLAT-1410: Modified vSphere config provider to not lose AddressesFromPools when applying Failure Domains #273

Merged
merged 1 commit into from Feb 20, 2024
Merged

SPLAT-1410: Modified vSphere config provider to not lose AddressesFromPools when applying Failure Domains #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ require (
github.com/onsi/gomega v1.30.0
github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e
github.com/openshift/client-go v0.0.0-20240125160436-aa5df63097c4
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20231130130825-ea989e248004
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20240201120052-f5a6a5f74055
github.com/openshift/library-go v0.0.0-20240115112243-470c096a1ca9
github.com/spf13/pflag v1.0.5
go.uber.org/zap v1.26.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Expand Up @@ -460,8 +460,8 @@ github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e h1:cxgCNo/R769CO23AK
github.com/openshift/api v0.0.0-20240124164020-e2ce40831f2e/go.mod h1:CxgbWAlvu2iQB0UmKTtRu1YfepRg1/vJ64n2DlIEVz4=
github.com/openshift/client-go v0.0.0-20240125160436-aa5df63097c4 h1:Ct+/c9d5rjZudN+VBLxRJIQfPy1gJZier1P1KdpvCaM=
github.com/openshift/client-go v0.0.0-20240125160436-aa5df63097c4/go.mod h1:ZS5cpA+0zI/ziDQxAKKvGkRKn9BJppqDYdAph+OUTlo=
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20231130130825-ea989e248004 h1:z5MmF35DnAa+HOX6AHMrGhkm7YXaKZAxrNL3JsYHJ1I=
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20231130130825-ea989e248004/go.mod h1:8U97lU5NyyrNKkVcG3zRXFbI33Z5D+GZYp6k4oHoT9k=
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20240201120052-f5a6a5f74055 h1:tDyJ73kGqi2RbS5bgpDXfKxckR2JbVOX4sMR4yf46mE=
github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20240201120052-f5a6a5f74055/go.mod h1:osVq9/R6qKHBQxDP4cYTvkgXVBKOMs1SOfPLFfn0m7A=
github.com/openshift/library-go v0.0.0-20240115112243-470c096a1ca9 h1:dQmXUWo3Ko+8K/fxKPF01NuTrD92d/ABBQ4NfxkwbAY=
github.com/openshift/library-go v0.0.0-20240115112243-470c096a1ca9/go.mod h1:82B0gt8XawdXWRtKMrm3jSMTeRsiOSYKCi4F0fvPjG0=
github.com/otiai10/copy v1.2.0 h1:HvG945u96iNadPoG2/Ja2+AUJeW5YuFQMixq9yirC+k=
Expand Down
41 changes: 35 additions & 6 deletions pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package providerconfig

import (
"encoding/json"
"fmt"
"strings"

Expand All @@ -33,6 +34,7 @@ import (
type VSphereProviderConfig struct {
providerConfig machinev1beta1.VSphereMachineProviderSpec
infrastructure *configv1.Infrastructure
logger logr.Logger
}

func (v VSphereProviderConfig) getFailureDomainFromInfrastructure(fd machinev1.VSphereFailureDomain) (*configv1.VSpherePlatformFailureDomainSpec, error) {
Expand Down Expand Up @@ -90,16 +92,29 @@ func (v VSphereProviderConfig) InjectFailureDomain(fd machinev1.VSphereFailureDo
newVSphereProviderConfig.providerConfig.Workspace = newVSphereProviderConfig.getWorkspaceFromFailureDomain(failureDomain)
topology := failureDomain.Topology

logNetworkInfo(newVSphereProviderConfig.providerConfig.Network, "control plane machine set network before failure domain: %v", v.logger)

if len(topology.Networks) > 0 {
newVSphereProviderConfig.providerConfig.Network = machinev1beta1.NetworkSpec{
Devices: []machinev1beta1.NetworkDeviceSpec{
{
NetworkName: topology.Networks[0],
},
},
networkSpec := machinev1beta1.NetworkSpec{}
// If original has AddressesFromPools, that means static IP is desired for the CPMS. Keep that and just add the FD info.
if len(newVSphereProviderConfig.providerConfig.Network.Devices[0].AddressesFromPools) > 0 {
networkSpec.Devices = newVSphereProviderConfig.providerConfig.Network.Devices
}

// Set the network name for the device from FD.
for index, network := range topology.Networks {
if len(networkSpec.Devices) <= index {
networkSpec.Devices = append(networkSpec.Devices, machinev1beta1.NetworkDeviceSpec{})
}

networkSpec.Devices[index].NetworkName = network
}

newVSphereProviderConfig.providerConfig.Network = networkSpec
}

logNetworkInfo(newVSphereProviderConfig.providerConfig.Network, "control plane machine set network after failure domain: %v", v.logger)

if len(topology.Template) > 0 {
newVSphereProviderConfig.providerConfig.Template = topology.Template[strings.LastIndex(topology.Template, "/")+1:]
} else if len(v.infrastructure.Spec.PlatformSpec.VSphere.FailureDomains) > 0 {
Expand Down Expand Up @@ -181,6 +196,7 @@ func newVSphereProviderConfig(logger logr.Logger, raw *runtime.RawExtension, inf
VSphereProviderConfig := VSphereProviderConfig{
providerConfig: vsphereMachineProviderSpec,
infrastructure: infrastructure,
logger: logger,
}

// For networking, we only need to compare the network name. For static IPs, we can ignore all ip configuration;
Expand All @@ -204,3 +220,16 @@ func newVSphereProviderConfig(logger logr.Logger, raw *runtime.RawExtension, inf

return config, nil
}

// logNetworkInfo log network info as json for better debugging of data being processed.
func logNetworkInfo(network machinev1beta1.NetworkSpec, msg string, logger logr.Logger) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this is something we really want to log in the long term, or is this primarily for our own diagnostics? Could testing not ensure that this works as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a nice development / debug nice to have. The purpose of this is in case there is a config that we are not expecting and this can show how we modified it incorrectly. I know we log very little and this allows me to see what we are changing the network config to be. It only logs if someone overrides the deployment config to v(4). so this is being skipped by default. If this is something that we frown upon, I'll remove this logging.

// To limit marshalling to only when log level > 4.
if logger.GetV() >= 4 {
jsonOutput, err := json.Marshal(network)
if err != nil {
logger.Error(err, "Got error Marshalling NetworkSpec")
}

logger.V(4).Info(msg, string(jsonOutput))
}
}
28 changes: 28 additions & 0 deletions pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere_test.go
View file
Open in desktop
Expand Up @@ -94,6 +94,34 @@ var _ = Describe("VSphere Provider Config", Label("vSphereProviderConfig"), func
})
})

Context("StaticIP", func() {

BeforeEach(func() {
machineProviderConfig := machinev1beta1resourcebuilder.VSphereProviderSpec().
WithZone(usCentral1a).WithIPPool().
Build()

providerConfig = VSphereProviderConfig{
providerConfig: *machineProviderConfig,
infrastructure: configv1resourcebuilder.Infrastructure().AsVSphereWithFailureDomains("vsphere-test", nil).Build(),
}
})

It("contains an AddressesFromPools block", func() {
Expect(providerConfig.providerConfig.Network.Devices[0].AddressesFromPools).To(Not(BeEmpty()),
"expected AddressesFromPools to not be empty as a static IPPool has been configured")
})

It("returns networking with AddressesFromPools and the configured network name from failure domain", func() {
expected, err := providerConfig.InjectFailureDomain(providerConfig.ExtractFailureDomain())
Expect(err).To(Not(HaveOccurred()))
Expect(expected.providerConfig.Network.Devices[0].AddressesFromPools).To(Not(BeNil()),
"expected AddressesFromPools to still be present after injecting Failure Domain")
Expect(expected.providerConfig.Network.Devices[0].NetworkName).To(Equal(providerConfig.infrastructure.Spec.PlatformSpec.VSphere.FailureDomains[0].Topology.Networks[0]),
"expected NetworkName to still be equal to the the original after injection of the Failure Domain")
})
})

Context("newVSphereProviderConfig", func() {
var providerConfig ProviderConfig
var expectedVSphereConfig machinev1beta1.VSphereMachineProviderSpec
Expand Down
14 changes: 9 additions & 5 deletions ...uster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/vsphere_provider_spec.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions vendor/modules.txt
View file
Open in desktop
Expand Up @@ -639,8 +639,8 @@ github.com/openshift/client-go/config/listers/config/v1alpha1
github.com/openshift/client-go/machine/applyconfigurations/internal
github.com/openshift/client-go/machine/applyconfigurations/machine/v1
github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1
# github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20231130130825-ea989e248004
## explicit; go 1.20
# github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20240201120052-f5a6a5f74055
## explicit; go 1.21
github.com/openshift/cluster-api-actuator-pkg/testutils
github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder
github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/config/v1
Expand Down