Skip to content

fix: IPv4 CIDRPool gatewayIndex is 0 but leaf is second /31 address - #198

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/addressing-ipv4-cidrpool-gatewayindex-is-0-but
Open

fix: IPv4 CIDRPool gatewayIndex is 0 but leaf is second /31 address#198
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/addressing-ipv4-cidrpool-gatewayindex-is-0-but

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown

This PR addresses the following issue in pkg/networkoperatorplugin/spectrumx/addressing.go: IPv4 CIDRPool gatewayIndex is 0 but leaf is second /31 address.

Changes

  • pkg/networkoperatorplugin/spectrumx/addressing.go: IPv4 CIDRPool gatewayIndex is 0 but leaf is second /31 address.

Details

--- a/pkg/networkoperatorplugin/spectrumx/addressing.go
+++ b/pkg/networkoperatorplugin/spectrumx/addressing.go
@@ -1,14 +1,14 @@
-func poolSettings(spcx *config.ProfileSpectrumX) cidrPoolSettings {
-	if isIPv6(spcx) {
-		return cidrPoolSettings{
-			gatewayIndex:         2,
-			perNodeNetworkPrefix: 64,
-			perNodeExclusions:    []PerNodeExclusion{{StartIndex: 2, EndIndex: 2}},
-		}
-	}
-	return cidrPoolSettings{
-		gatewayIndex:         0,
-		perNodeNetworkPrefix: 31,
-		perNodeExclusions:    []PerNodeExclusion{{StartIndex: 1, EndIndex: 1}},
-	}
-}
+func poolSettings(spcx *config.ProfileSpectrumX) cidrPoolSettings {
+	if isIPv6(spcx) {
+		return cidrPoolSettings{
+			gatewayIndex:         2,
+			perNodeNetworkPrefix: 64,
+			perNodeExclusions:    []PerNodeExclusion{{StartIndex: 2, EndIndex: 2}},
+		}
+	}
+	return cidrPoolSettings{
+		gatewayIndex:         1,
+		perNodeNetworkPrefix: 31,
+		perNodeExclusions:    []PerNodeExclusion{{StartIndex: 1, EndIndex: 1}},
+	}
+}

Tests

  • pkg/networkoperatorplugin/spectrumx/pool_settings_test.go
diff --git a/pkg/networkoperatorplugin/spectrumx/pool_settings_test.go b/pkg/networkoperatorplugin/spectrumx/pool_settings_test.go
new file mode 100644
index 0000000..e69ad46
--- /dev/null
+++ b/pkg/networkoperatorplugin/spectrumx/pool_settings_test.go
@@ -0,0 +1,34 @@
+package spectrumx
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/nvidia/k8s-launch-kit/pkg/config"
+)
+
+func TestPoolSettingsIPv4GatewayIndex(t *testing.T) {
+	spcx := &config.ProfileSpectrumX{IPVersion: config.SpectrumXIPVersionIPv4}
+	got := poolSettings(spcx)
+	want := cidrPoolSettings{
+		gatewayIndex:         1,
+		perNodeNetworkPrefix: 31,
+		perNodeExclusions:    []PerNodeExclusion{{StartIndex: 1, EndIndex: 1}},
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("poolSettings(IPv4) = %+v, want %+v", got, want)
+	}
+}
+
+func TestPoolSettingsIPv6Unchanged(t *testing.T) {
+	spcx := &config.ProfileSpectrumX{IPVersion: config.SpectrumXIPVersionIPv6}
+	got := poolSettings(spcx)
+	want := cidrPoolSettings{
+		gatewayIndex:         2,
+		perNodeNetworkPrefix: 64,
+		perNodeExclusions:    []PerNodeExclusion{{StartIndex: 2, EndIndex: 2}},
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("poolSettings(IPv6) = %+v, want %+v", got, want)
+	}
+}

Contributor guidelines

Per this repo's CONTRIBUTING.md:

  • All commits are signed off (Signed-off-by trailer, DCO).

Update (post-Greptile review): The remaining existing IPv4 BuildCIDRPools test expectations in addressing_test.go have been updated to GatewayIndex: 1 so the implementation and tests agree. The Spectrum-X addressing test suite now passes (go test ./pkg/networkoperatorplugin/spectrumx/...). Commit: e714cc7464d66d96522c4d45cd9739d591548066.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Greptile Summary

The PR corrects IPv4 Spectrum-X CIDR pools so the gateway index identifies the leaf at the second address of each /31.

  • Changes the IPv4 gateway index from 0 to 1.
  • Updates all affected BuildCIDRPools expectations.
  • Adds direct coverage for IPv4 and unchanged IPv6 pool settings.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported stale IPv4 pool expectations have been updated consistently.

Important Files Changed

Filename Overview
pkg/networkoperatorplugin/spectrumx/addressing.go Aligns the IPv4 CIDRPool gateway index with the leaf address at index 1 of each /31.
pkg/networkoperatorplugin/spectrumx/addressing_test.go Updates all existing IPv4 pool expectations affected by the gateway-index correction.
pkg/networkoperatorplugin/spectrumx/pool_settings_test.go Adds focused regression coverage for IPv4 and IPv6 pool settings.

Reviews (2): Last reviewed commit: "fix: IPv4 CIDRPool gatewayIndex is 0 but..." | Re-trigger Greptile

}
return cidrPoolSettings{
gatewayIndex: 0,
gatewayIndex: 1,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Stale IPv4 pool expectations

When the Spectrum-X package tests build IPv4 CIDR pools, this change produces GatewayIndex: 1 while two existing expected pools still require GatewayIndex: 0, causing the test suite to fail. Update the existing BuildCIDRPools expectations alongside this contract change.

Knowledge Base Used: Spectrum-X addressing and topology

Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the bugfix/addressing-ipv4-cidrpool-gatewayindex-is-0-but branch from c948842 to e714cc7 Compare August 17, 2026 22:03
@andrewwhitecdw

Copy link
Copy Markdown
Author

@greptile-apps Thanks for the review. I updated the remaining IPv4 BuildCIDRPools test expectations in addressing_test.go to GatewayIndex: 1, so the implementation and existing tests now agree. go test ./pkg/networkoperatorplugin/spectrumx/... passes.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

That's exactly what was needed — implementation, new unit tests, and existing integration-style tests all aligned on gatewayIndex: 1. LGTM.

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.

1 participant