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

MGMT-20043: Modify MTU Validatin CMN to support multiple machine networks #7357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+88 −30
Open

MGMT-20043: Modify MTU Validatin CMN to support multiple machine networks #7357

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 44 additions & 19 deletions internal/host/validations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2699,7 +2699,25 @@ var _ = Describe("Validations test", func() {
cluster *common.Cluster
host *models.Host
)
DescribeTable("MTU validations across CMN and UMN", func(mtuReports []*models.MtuReport, validationStatus ValidationStatus, isUMN, isDay2 bool) {

mNet := []*models.MachineNetwork{
{
Cidr: "1.2.3.1/24",
ClusterID: clusterID,
},
}

multipleMNets := []*models.MachineNetwork{
{
Cidr: "1.2.3.1/24",
ClusterID: clusterID,
},
{
Cidr: "1.2.4.1/24",
ClusterID: clusterID,
},
}
DescribeTable("MTU validations across CMN and UMN", func(mtuReports []*models.MtuReport, mNets []*models.MachineNetwork, validationStatus ValidationStatus, isUMN, isDay2 bool) {
mockProviderRegistry.EXPECT().IsHostSupported(commontesting.EqPlatformType(models.PlatformTypeVsphere), gomock.Any()).Return(false, nil).AnyTimes()

if isDay2 {
Expand All @@ -2714,13 +2732,7 @@ var _ = Describe("Validations test", func() {
}

cluster.UserManagedNetworking = &isUMN
mNet := []*models.MachineNetwork{
{
Cidr: "1.2.3.1/24",
ClusterID: clusterID,
},
}
cluster.MachineNetworks = mNet
cluster.MachineNetworks = mNets

connectivityReport := &models.ConnectivityReport{}
connectivityReport.RemoteHosts = append(connectivityReport.RemoteHosts, &models.ConnectivityRemoteHost{
Expand All @@ -2737,27 +2749,40 @@ var _ = Describe("Validations test", func() {
mockAndRefreshStatus(host)

refreshedHost := &hostutil.GetHostFromDB(*host.ID, host.InfraEnvID, db).Host
fmt.Println(host.ValidationsInfo)
status, _, ok := getValidationResult(refreshedHost.ValidationsInfo, IsMtuValid)
Expect(ok).To(BeTrue(), fmt.Sprintf("debuuging info: status %s, isOK: %t host: %#v cluster: %#v", status, ok, host, cluster))
Expect(status).To(Equal(validationStatus))
},
Entry("isMtuValidInMachineNetwork (CMN) - Happy flow - MTU reports in the machine network are successful", []*models.MtuReport{
{MtuSuccessful: true, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"},
{MtuSuccessful: false, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"},
}, ValidationSuccess, false, false),
Entry("isMtuValidInMachineNetwork (CMN) - Bad flow - MTU reports in the machine network are unsuccessful", []*models.MtuReport{
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"},
}, ValidationFailure, false, false),

Entry("isMtuValidInMachineNetwork (CMN) - Happy flow - all reports where both sourceIP and remoteIP are within the machine network are successful", []*models.MtuReport{
{MtuSuccessful: true, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
{MtuSuccessful: false, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"}, // sourceIP in machine net, remoteIP not
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth1"}, // remoteIP in machine net, sourceIP not
}, mNet, ValidationSuccess, false, false),
Entry("isMtuValidInMachineNetwork (CMN) - Bad flow - at least one report where both sourceIP and remoteIP are within the machine network is unsuccessful", []*models.MtuReport{
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
{MtuSuccessful: true, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"}, // sourceIP in machine net, remoteIP not
{MtuSuccessful: true, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth1"}, // remoteIP in machine net, sourceIP not
}, mNet, ValidationFailure, false, false),
Entry("isMtuValidAllInterfaces (UMN) - Happy flow - all Mtu reports successful", []*models.MtuReport{
{MtuSuccessful: true, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"},
{MtuSuccessful: true, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"},
}, ValidationSuccess, true, false),
}, mNet, ValidationSuccess, true, false),
Entry("isMtuValidAllInterfaces (UMN) - Bad flow - all MTU reports are unsuccessful", []*models.MtuReport{
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"},
{MtuSuccessful: false, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"},
}, ValidationFailure, true, false),
Entry("Day 2 cluster - the validation should be skipped", []*models.MtuReport{}, ValidationSuccess, false, true),
}, mNet, ValidationFailure, true, false),
Entry("isMtuValidInMachineNetwork (CMN) - Happy flow - all reports where both sourceIP and remoteIP are within any of the machine networks are successful", []*models.MtuReport{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also test dual stack happy and failing flow

{MtuSuccessful: true, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
{MtuSuccessful: true, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
{MtuSuccessful: false, RemoteIPAddress: "1.2.5.5", OutgoingNic: "eth0"}, // sourceIP in machine net, remoteIP not
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth1"}, // remoteIP in machine net, sourceIP not
}, multipleMNets, ValidationSuccess, false, false),
Entry("isMtuValidInMachineNetwork (CMN) - Bad flow - at least one report where both sourceIP and remoteIP are within any of the machine networks is unsuccessful", []*models.MtuReport{
{MtuSuccessful: false, RemoteIPAddress: "1.2.3.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
{MtuSuccessful: true, RemoteIPAddress: "1.2.4.5", OutgoingNic: "eth0"}, // sourceIP and remoteIP are in machine net
}, multipleMNets, ValidationFailure, false, false),
Entry("Day 2 cluster - the validation should be skipped", []*models.MtuReport{}, mNet, ValidationSuccess, false, true),
)
})

Expand Down
27 changes: 16 additions & 11 deletions internal/host/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,19 +346,24 @@ func (v *validator) isMtuValidAllInterfaces(c *validationContext, connectivityRe

// isMtuValidInMachineNetwork - intended for CMN, focusing exclusively on the MTU reports for the machine network.
func (v *validator) isMtuValidInMachineNetwork(c *validationContext, connectivityReport *models.ConnectivityReport) (ValidationStatus, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

General note about this validation, I think we can improve its performance - I M worried about the time it will take to perform it on large clusters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I'm not sure if there's a better way to optimize it.

The best I can think of, though I'm not sure how much it would improve performance, is to loop over the machine networks beforehand, parse them once, and store them in a separate list to avoid repeated parsing.

Other than that, I'm not sure how else we can improve it.

Do you have any other suggestions?

for _, machineNet := range c.cluster.MachineNetworks {
_, mNetwork, err := net.ParseCIDR(string(machineNet.Cidr))
if err != nil {
return ValidationError, "Internal error - failed to parse machine network CIDR"
}
for _, r := range connectivityReport.RemoteHosts {
for _, mtuReport := range r.MtuReport {
sourceIP, err := network.FindSourceIPInMachineNetwork(mtuReport.OutgoingNic, mNetwork, c.inventory.Interfaces)
for _, r := range connectivityReport.RemoteHosts {
for _, mtuReport := range r.MtuReport {
// We are only interested in the source IP within machine networks. If it is not part of any machine network, we don't need to consider it.
isSourceIPOk, err := network.IsSourceIPBelongsToAnyMachineNetwork(mtuReport.OutgoingNic, c.cluster.MachineNetworks, c.inventory.Interfaces)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe isSourceIPOk -> isSourceIPBelongToAnyMachineNetwork ? or any name describing the result value of this variable rather than OK ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understand this check. Why do we care if the interface belongs to any machine network ? is it because we want to ignore ISCSI interfaces ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The MTU check is layer 3 check right ? why do we care about machine networks at all ? if in UMN platform we check each interface against all other interfaces, I don't see why shouldn't we do it here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understand this check. Why do we care if the interface belongs to any machine network ?

Because in CMN, we only care about communication between interfaces within the machine networks. If the ping is sent from an interface outside the machine network, we don't want to consider it.

why do we care about machine networks at all ?

In CMN, the machine networks are known in advance, so we only check interfaces within those networks. In UMN, since the machine networks are not known in advance, we check all interfaces.

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth checking for error before the other logic no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what I do. checking the error before if !isSourceIPOk

return ValidationError, err.Error()
}
if !isSourceIPOk {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move this few lines above for early skip and avoid the opposite if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how. I need the MTU report data, so I placed it right after the for loop begins.

continue
}
remoteIP := net.ParseIP(mtuReport.RemoteIPAddress)
for _, machineNet := range c.cluster.MachineNetworks {
_, mNetwork, err := net.ParseCIDR(string(machineNet.Cidr))
if err != nil {
return ValidationError, err.Error()
return ValidationError, "Internal error - failed to parse machine network CIDR"
}
remoteIP := net.ParseIP(mtuReport.RemoteIPAddress)
if mNetwork.Contains(remoteIP) && sourceIP != "" {
// We are only interested in the remote IP within machine networks. If it is not part of any machine network, we don't need to consider it.
if mNetwork.Contains(remoteIP) {
if !mtuReport.MtuSuccessful {
return ValidationFailure, fmt.Sprintf("MTU is broken. Interface: %s, remote IP address: %s", mtuReport.OutgoingNic, mtuReport.RemoteIPAddress)
}
Expand Down
17 changes: 17 additions & 0 deletions internal/network/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,3 +512,20 @@ func FindSourceIPInMachineNetwork(outgoingNicName string, mNetwork *net.IPNet, i
}
return sourceIP, nil
}

func IsSourceIPBelongsToAnyMachineNetwork(outgoingNicName string, mNetworks []*models.MachineNetwork, interfaces []*models.Interface) (bool, error) {
for _, mNet := range mNetworks {
_, mNetwork, err := net.ParseCIDR(string(mNet.Cidr))
if err != nil {
return false, errors.New("Internal error - failed to parse machine network CIDR")
}
sourceIP, err := FindSourceIPInMachineNetwork(outgoingNicName, mNetwork, interfaces)
if err != nil {
return false, err
}
if sourceIP != "" {
return true, nil
}
}
return false, nil
}
11 changes: 11 additions & 0 deletions internal/network/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,3 +1066,14 @@ var _ = Describe("FindSourceIPInMachineNetwork", func() {
Entry("source IP doesn't in machine network", "eth0", &models.MachineNetwork{Cidr: "192.168.10.1/24"}, []*models.Interface{{IPV4Addresses: []string{"192.168.11.2/24"}, Name: "eth0"}}, ""),
)
})

var _ = Describe("IsSourceIPBelongsToAnyMachineNetwork", func() {
DescribeTable("", func(outgoingNicName string, mNetworks []*models.MachineNetwork, interfaces []*models.Interface, expectedResult bool) {
isSourceIPOk, err := IsSourceIPBelongsToAnyMachineNetwork(outgoingNicName, mNetworks, interfaces)
Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("Debugging info: %v", err))
Expect(isSourceIPOk).To(Equal(expectedResult))
},
Entry("source IP belongs to one of the machine networks", "eth0", []*models.MachineNetwork{{Cidr: "192.168.10.0/24"}, {Cidr: "192.168.11.0/24"}}, []*models.Interface{{IPV4Addresses: []string{"192.168.10.2/24"}, Name: "eth0"}}, true),
Entry("source IP doesn't belongs to any of the machine networks", "eth0", []*models.MachineNetwork{{Cidr: "192.168.10.0/24"}, {Cidr: "192.168.11.0/24"}}, []*models.Interface{{IPV4Addresses: []string{"192.168.12.2/24"}, Name: "eth0"}}, false),
)
})