Skip to content

Conversation

@pau-hedgehog
Copy link
Contributor

No description provided.

@pau-hedgehog pau-hedgehog requested a review from Copilot August 11, 2025 15:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for testing multi-VPC attachments in the testing framework. It enables servers to be attached to multiple VPCs simultaneously using a primary+next pattern for VPC selection.

Key changes:

  • Added multi-VPC server selection logic that chooses servers by connection type (unbundled, bundled, mclag, eslag)
  • Modified VPC attachment creation to support multiple attachments per server with proper naming
  • Updated connectivity testing to test all attachment combinations between servers

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
pkg/hhfab/testing.go Main implementation of multi-VPC attachment logic, modified VPC setup and connectivity testing
pkg/hhfab/hhnet.sh Added addvlan command to support adding VLANs to existing bonds
cmd/hhfab/main.go Added CLI flag --servers-with-multi-vpc to control the feature

Comment on lines +606 to +613
// If wraparound results in same VPC (edge case with single VPC), skip to avoid duplicate
if currentVPCID == vpcID && totalVPCs > 1 {
currentVPCID = (vpcID + 2) % totalVPCs
Copy link

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

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

The edge case handling for wraparound when currentVPCID equals vpcID could be simplified. Consider using a while loop to ensure we always get a different VPC ID rather than hardcoding the +2 offset.

Suggested change
// If wraparound results in same VPC (edge case with single VPC), skip to avoid duplicate
if currentVPCID == vpcID && totalVPCs > 1 {
currentVPCID = (vpcID + 2) % totalVPCs
// If wraparound results in same VPC, increment until different (avoids duplicate)
for currentVPCID == vpcID && totalVPCs > 1 {
currentVPCID = (currentVPCID + 1) % totalVPCs

Copilot uses AI. Check for mistakes.
}
slog.Debug("Checking connectivity", logArgs...)

sourceInterface := fmt.Sprintf("bond0.%d", attachA.VLAN)
Copy link

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

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

The interface name format "bond0.%d" is hardcoded in multiple places. Consider defining it as a constant to improve maintainability.

Suggested change
sourceInterface := fmt.Sprintf("bond0.%d", attachA.VLAN)
sourceInterface := fmt.Sprintf(BondInterfaceFormat, attachA.VLAN)

Copilot uses AI. Check for mistakes.
cmd := fmt.Sprintf("toolbox -q timeout -v %d iperf3 -P 4 -J -c %s -t %d", opts.IPerfsSeconds+25, toIP.String(), opts.IPerfsSeconds)

if sourceInterface != "" {
cmd += fmt.Sprintf(" -B %s", sourceInterface)
Copy link

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

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

The iperf3 -B flag expects an IP address or interface name, but sourceInterface contains a VLAN interface name (e.g., "bond0.1000"). This may not work correctly with iperf3. Consider using the IP address of the interface instead.

Suggested change
cmd += fmt.Sprintf(" -B %s", sourceInterface)
bindAddr := sourceInterface
// If sourceInterface looks like an interface name (not an IP), resolve to IP
if netip.ParseAddr(sourceInterface).IsValid() == false {
ip, err := getInterfaceIP(sourceInterface)
if err != nil {
return fmt.Errorf("could not resolve IP for interface %q: %w", sourceInterface, err)
}
bindAddr = ip
}
cmd += fmt.Sprintf(" -B %s", bindAddr)

Copilot uses AI. Check for mistakes.

slog.Debug("Checking external connectivity", "from", fmt.Sprintf("%s(%s/%s)", serverA, attach.VPC, attach.Subnet), "reachable", reachable)

sourceInterface := fmt.Sprintf("bond0.%d", attach.VLAN)
Copy link

Copilot AI Aug 11, 2025

Choose a reason for hiding this comment

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

The interface name format "bond0.%d" is duplicated again. This reinforces the need for a constant to avoid inconsistencies.

Suggested change
sourceInterface := fmt.Sprintf("bond0.%d", attach.VLAN)
sourceInterface := fmt.Sprintf(BondInterfaceFormat, attach.VLAN)

Copilot uses AI. Check for mistakes.
@Frostman
Copy link
Member

I think we shouldn't be doing it now, there are too many things to do:

  • SetupVPCs is already overloaded, and to make it properly configurable, it would be difficult w/o rewriting and introducing some kind of simple lang to define VPCs (similar to the peerings)
  • We cannot rely on bond0 - what if you have multiple multihomed connections at the same time
  • We should calculate source interface for pings/iperfs/curls very carefully and it technically could be a list to test, not just a single one
  • When we're doing multi-attach to servers we would need to test all permutations - check connectivity from all attachments of all servers to all attachments of all servers to make sure that traffic will not go through where we don't expect it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants