Skip to content

fix: IPv4 CIDRPool tests expect wrong gatewayIndex for /31 - #201

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/addressing-test-ipv4-cidrpool-tests-expect-wrong
Open

fix: IPv4 CIDRPool tests expect wrong gatewayIndex for /31#201
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/addressing-test-ipv4-cidrpool-tests-expect-wrong

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown

This PR addresses the following issue in pkg/networkoperatorplugin/spectrumx/addressing_test.go: IPv4 CIDRPool tests expect wrong gatewayIndex for /31.

Changes

  • pkg/networkoperatorplugin/spectrumx/addressing_test.go: IPv4 CIDRPool tests expect wrong gatewayIndex for /31.

Details

--- a/pkg/networkoperatorplugin/spectrumx/addressing_test.go
+++ b/pkg/networkoperatorplugin/spectrumx/addressing_test.go
@@ -1,3 +1,3 @@
-require.Equal(t, 0, pools[0].GatewayIndex)
-		GatewayIndex:         0,
-		GatewayIndex:         0,
+require.Equal(t, 1, pools[0].GatewayIndex)
+		GatewayIndex:         1,
+		GatewayIndex:         1,

Tests

  • pkg/networkoperatorplugin/spectrumx/addressing_test.go
--- a/pkg/networkoperatorplugin/spectrumx/addressing_test.go
+++ b/pkg/networkoperatorplugin/spectrumx/addressing_test.go
@@ -205,7 +205,8 @@ func TestBuildCIDRPoolsFromAIR2Tier(t *testing.T) {
 			require.NoError(t, err)
 			require.Len(t, pools, tt.wantPools)
 			require.Equal(t, tt.wantFirstName, pools[0].Name)
+			require.Equal(t, 1, pools[0].GatewayIndex)
 			require.Equal(t, []StaticAllocation{
 				{Gateway: "172.16.0.1", NodeName: "worker-su01-rack01-h01", Prefix: "172.16.0.0/31"},
 				{Gateway: "172.16.0.3", NodeName: "worker-su01-rack01-h02", Prefix: "172.16.0.2/31"},
 			}, pools[0].StaticAllocations)

Contributor guidelines

Per this repo's CONTRIBUTING.md:

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

Update (post-Greptile review): The IPv4 poolSettings implementation has been updated to return GatewayIndex: 1 so it agrees with the corrected test assertions. The Spectrum-X addressing test suite now passes (go test ./pkg/networkoperatorplugin/spectrumx/...). Commit: f9310104ae35ca8067ee669825a87e257bd03c70.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Greptile Summary

The PR completes the IPv4 /31 gateway correction by changing the implementation to use gateway index 1 and updating the corresponding Spectrum-X addressing tests.

  • Aligns poolSettings with the odd-address leaf gateways generated for IPv4 /31 allocations.
  • Updates reference- and AIR-topology test expectations for the corrected gateway index.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the implementation now uses index 1 for the generated odd-address IPv4 /31 gateway, resolving the previously reported test contradiction.

Important Files Changed

Filename Overview
pkg/networkoperatorplugin/spectrumx/addressing.go Changes IPv4 /31 pools to use gateway index 1, matching the generated leaf gateway address.
pkg/networkoperatorplugin/spectrumx/addressing_test.go Updates IPv4 CIDRPool assertions to verify the corrected gateway index across supported topology variants.

Reviews (2): Last reviewed commit: "fix: IPv4 CIDRPool tests expect wrong ga..." | Re-trigger Greptile

Comment thread pkg/networkoperatorplugin/spectrumx/addressing_test.go
Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the bugfix/addressing-test-ipv4-cidrpool-tests-expect-wrong branch from 23b915f to f931010 Compare August 17, 2026 22:04
@andrewwhitecdw

Copy link
Copy Markdown
Author

@greptile-apps Thanks for the review. I updated the IPv4 poolSettings implementation to return GatewayIndex: 1, matching the corrected test expectations (including the AIR 2-tier subtests). go test ./pkg/networkoperatorplugin/spectrumx/... now passes.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Looks good. The fix is consistent — all four assertion sites now expect GatewayIndex: 1, which correctly reflects that in a /31 the gateway is the second address (index 1). The existing PerNodeExclusions: [{StartIndex: 1, EndIndex: 1}] already encoded this, so the test expectations were simply out of sync with the implementation intent. Good to merge.

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