OCPBUGS-63348: Fix capacity calculation #184

kyrtapz · 2025-10-22T08:02:06Z

The previous capacity calculation incorrectly only counted CloudPrivateIPConfigs that had the status set.
This is racy and can lead to artificially reduced capacity.
Fix it by accounting for all CloudPrivateIPConfigs that are or were assigned to the matching node.

openshift-ci-robot · 2025-10-23T11:49:04Z

@kyrtapz: This pull request references Jira Issue OCPBUGS-63348, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

The previous capacity calculation incorrectly only counted CloudPrivateIPConfigs that had the status set.
This is racy and can lead to artificially reduced capacity.
Fix it by accounting for all CloudPrivateIPConfigs that are or were assigned to the matching node.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

kyrtapz · 2025-11-03T07:50:51Z

/jira refresh

openshift-ci-robot · 2025-11-03T07:50:59Z

@kyrtapz: This pull request references Jira Issue OCPBUGS-63348, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @huiran0826

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

huiran0826 · 2025-11-03T10:22:09Z

/verified by 'Pre-merge testing'

openshift-ci-robot · 2025-11-03T10:22:21Z

@huiran0826: This PR has been marked as verified by 'Pre-merge testing'.

In response to this:

/verified by 'Pre-merge testing'

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

pperiyasamy · 2025-11-06T09:57:36Z

pkg/controller/node/node_controller.go

+	// Include CPICs where spec.node OR status.node matches to handle transitions
+	cpicIPs := sets.New[string]()
 	for _, cloudPrivateIPConfig := range cloudPrivateIPConfigs {
-		if isAssignedCloudPrivateIPConfigOnNode(cloudPrivateIPConfig, key) {


you may remove isAssignedCloudPrivateIPConfigOnNode method which is no longer needed.

pperiyasamy · 2025-11-06T10:01:28Z

pkg/controller/node/node_controller.go

-		if isAssignedCloudPrivateIPConfigOnNode(cloudPrivateIPConfig, key) {
-			cloudPrivateIPConfigs[index] = cloudPrivateIPConfig
-			index++
+		if cloudPrivateIPConfig.Spec.Node == key || cloudPrivateIPConfig.Status.Node == key {


Nit: If capacity is just about excluding only IPs_consumed_by_non_CPIC_sources from the limit, why can't we just pass all cloudPrivateIPConfigs to GetNodeEgressIPConfiguration method for simplicity ?

This would mean the same logic would have to be copied across all of the implementations. Doing it the way we have here avoids it.

i meant to check assignedIP with all cpicIPs, but anyway this also looks fine.

pperiyasamy · 2025-11-06T10:06:01Z

pkg/cloudprovider/gcp.go

+	g.nodeMapLock.Lock()
+	defer g.nodeMapLock.Unlock()
+	if _, ok := g.nodeLockMap[nodeName]; !ok {
+		g.nodeLockMap[nodeName] = &sync.Mutex{}


shouldn't cleanup the lock when node is deleted ?

That's a valid point. Done(added it for azure as well).

The previous capacity calculation incorrectly only counted CloudPrivateIPConfigs that had the status set. This is racy and can lead to artificially reduced capacity. Fix it by accounting for all CloudPrivateIPConfigs that are or were assigned to the matching node. Change the calculation in openstack to only consider IPs that were assigned to the port by using `allowed_address_pairs`. Signed-off-by: Patryk Diak <[email protected]>

To avoid the following error being returned when trying to add/remove multiple IPs at once: err: googleapi: Error 412: Invalid fingerprint. From the docs: The request will fail with error 400 Bad Request if the fingerprint is not provided, or 412 Precondition Failed if the fingerprint is out of date. Signed-off-by: Patryk Diak <[email protected]>

Signed-off-by: Patryk Diak <[email protected]>

coderabbitai · 2025-11-06T11:50:25Z

Walkthrough

This pull request refactors CloudPrivateIPConfig parameter handling across cloud provider implementations by replacing slice-based inputs with set-based IP collections. The changes include parameter signature updates in AWS, Azure, GCP, and OpenStack providers, addition of CleanupNode() lifecycle methods, GCP per-node lock synchronization, and controller-level updates to collect and pass CPIC IPs as sets.

Changes

Cohort / File(s)	Summary
Interface & Mock Implementation `pkg/cloudprovider/cloudprovider.go`, `pkg/cloudprovider/cloudprovider_fake.go`	Updated `GetNodeEgressIPConfiguration` signature to accept `sets.Set[string]` instead of `[]*v1.CloudPrivateIPConfig`; added `CleanupNode` method to interface; updated imports to add sets package and remove v1 cloudnetwork dependency.
AWS Provider `pkg/cloudprovider/aws.go`	Changed `GetNodeEgressIPConfiguration` and `getCapacity` to accept `cpicIPs sets.Set[string]`; removed error return from `getCapacity`; added `CleanupNode` method; updated capacity calculation logic to use set membership checks instead of slice iteration.
Azure Provider `pkg/cloudprovider/azure.go`	Updated `GetNodeEgressIPConfiguration` and `getCapacity` signatures to accept `cpicIPs sets.Set[string]`; added `CleanupNode` method; refactored capacity calculation to count current IP usage from the provided set; updated imports for sets package.
GCP Provider `pkg/cloudprovider/gcp.go`	Added per-node concurrency control with `nodeMapLock` and `nodeLockMap` fields; implemented `getNodeLock()` helper for thread-safe per-node operations; updated `GetNodeEgressIPConfiguration` to accept `cpicIPs sets.Set[string]`; added locking to `AssignPrivateIP` and `ReleasePrivateIP`; added `CleanupNode` method; refactored capacity calculation with unified IP usage counter.
OpenStack Provider `pkg/cloudprovider/openstack.go`	Updated `GetNodeEgressIPConfiguration` and `getNeutronPortNodeEgressIPConfiguration` to accept `cpicIPs sets.Set[string]` instead of CloudPrivateIPConfig slice; refactored capacity counting to use set-based lookup; added `CleanupNode` method; removed v1 import.
OpenStack Tests `pkg/cloudprovider/openstack_test.go`	Changed test data types from `[]*v1.CloudPrivateIPConfig` to `[]string`; updated test cases to use `sets.New[string]()` for CPIC IPs; extended port test data with additional IPv4/IPv6 addresses; adjusted capacity calculations and test expectations to reflect new data structures.
Node Controller `pkg/controller/node/node_controller.go`	Replaced `isAssignedCloudPrivateIPConfigOnNode` logic with set-based collection of CPIC IPs using `NameToIP`; updated `GetNodeEgressIPConfiguration` call to pass new `cpicIPs` set; added `CleanupNode` invocation on node deletion; added `DeleteFunc` to event handler; updated imports for sets and cloudprivateipconfig packages.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

GCP concurrency implementation: Review per-node lock acquisition/release patterns in AssignPrivateIP, ReleasePrivateIP, and getNodeLock() helper for potential race conditions or deadlock scenarios
Capacity calculation consistency: Verify capacity calculation logic is correctly implemented identically across AWS, Azure, and OpenStack providers (all switched from counting-based to set-based exclusion)
Set-based parameter propagation: Trace cpicIPs set construction and usage throughout controller and all provider implementations to ensure consistency
Test coverage: Validate that test case adjustments in openstack_test.go properly reflect the new set-based data structures and capacity expectations
Cleanup lifecycle: Verify CleanupNode invocation flow in controller deletion path and proper lock cleanup in GCP implementation

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 8384756 and f6b12fd.

📒 Files selected for processing (8)

pkg/cloudprovider/aws.go (4 hunks)
pkg/cloudprovider/azure.go (5 hunks)
pkg/cloudprovider/cloudprovider.go (3 hunks)
pkg/cloudprovider/cloudprovider_fake.go (2 hunks)
pkg/cloudprovider/gcp.go (8 hunks)
pkg/cloudprovider/openstack.go (6 hunks)
pkg/cloudprovider/openstack_test.go (7 hunks)
pkg/controller/node/node_controller.go (5 hunks)

🔇 Additional comments (1)

pkg/controller/node/node_controller.go (1)

124-143: Set-based CPIC filtering handles moves cleanly

Collecting CPIC IPs by spec/status match and feeding the set into the provider plugs the race where transitional assignments skewed capacity. The cleanup on node disappearance ties the lifecycle together nicely.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

huiran0826 · 2025-11-06T11:55:33Z

/verified

openshift-ci-robot · 2025-11-06T11:55:37Z

@huiran0826: The /verified command must be used with one of the following actions: by, later, remove, or bypass. See https://docs.ci.openshift.org/docs/architecture/jira/#premerge-verification for more information.

In response to this:

/verified

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

huiran0826 · 2025-11-06T12:01:30Z

/verified by @huiran0826

openshift-ci-robot · 2025-11-06T12:01:41Z

@huiran0826: This PR has been marked as verified by @huiran0826.

In response to this:

/verified by @huiran0826

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

kyrtapz · 2025-11-06T14:08:15Z

/test hypershift-e2e-aks
Failure looks unrelated.

pperiyasamy · 2025-11-06T14:10:26Z

/lgtm
//hold waiting for CI to complete.

openshift-ci · 2025-11-06T14:14:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyrtapz, pperiyasamy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kyrtapz,pperiyasamy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kyrtapz · 2025-11-06T16:11:27Z

/override ci/prow/hypershift-e2e-aks
https://redhat-external.slack.com/archives/C01C8502FMM/p1762445069386699

openshift-ci · 2025-11-06T16:13:08Z

@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/hypershift-e2e-aks

In response to this:

/override ci/prow/hypershift-e2e-aks
https://redhat-external.slack.com/archives/C01C8502FMM/p1762445069386699

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2025-11-06T16:13:19Z

@kyrtapz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`f6b12fd`	link	false	`/test security`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2025-11-06T16:17:06Z

@kyrtapz: Jira Issue Verification Checks: Jira Issue OCPBUGS-63348
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-63348 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

In response to this:

The previous capacity calculation incorrectly only counted CloudPrivateIPConfigs that had the status set.
This is racy and can lead to artificially reduced capacity.
Fix it by accounting for all CloudPrivateIPConfigs that are or were assigned to the matching node.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

kyrtapz · 2025-11-06T16:19:05Z

/cherry-pick release-4.20

openshift-cherrypick-robot · 2025-11-06T16:19:52Z

@kyrtapz: new pull request created: #188

In response to this:

/cherry-pick release-4.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2025

openshift-ci bot requested review from arkadeepsen and tssurya October 22, 2025 08:03

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2025

kyrtapz force-pushed the fix_capacity_calc branch from c183655 to 2ce7584 Compare October 22, 2025 08:07

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2025

kyrtapz force-pushed the fix_capacity_calc branch 2 times, most recently from 8379c29 to 06577fe Compare October 23, 2025 07:54

kyrtapz changed the title ~~Fix capacity calculation~~ OCPBUGS-63348: Fix capacity calculation Oct 23, 2025

kyrtapz force-pushed the fix_capacity_calc branch 2 times, most recently from b71c9a0 to 394540b Compare October 24, 2025 12:51

kyrtapz force-pushed the fix_capacity_calc branch from 394540b to 939fac2 Compare November 3, 2025 07:47

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Nov 3, 2025

openshift-ci bot requested a review from huiran0826 November 3, 2025 07:51

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 3, 2025

pperiyasamy reviewed Nov 6, 2025

View reviewed changes

kyrtapz added 3 commits November 6, 2025 12:47

Cleanup in-memory node state on node removal

f6b12fd

Signed-off-by: Patryk Diak <[email protected]>

kyrtapz force-pushed the fix_capacity_calc branch from 939fac2 to f6b12fd Compare November 6, 2025 11:50

openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Nov 6, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 6, 2025

openshift-ci bot assigned pperiyasamy Nov 6, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2025

openshift-merge-bot bot merged commit a179997 into openshift:main Nov 6, 2025
13 of 14 checks passed

openshift-cherrypick-robot mentioned this pull request Nov 6, 2025

[release-4.20] OCPBUGS-64742: Fix capacity calculation #188

Open

OCPBUGS-63348: Fix capacity calculation #184

OCPBUGS-63348: Fix capacity calculation #184

Uh oh!

Conversation

kyrtapz commented Oct 22, 2025

Uh oh!

openshift-ci-robot commented Oct 23, 2025

Uh oh!

kyrtapz commented Nov 3, 2025

Uh oh!

openshift-ci-robot commented Nov 3, 2025

Uh oh!

huiran0826 commented Nov 3, 2025

Uh oh!

openshift-ci-robot commented Nov 3, 2025

Uh oh!

pperiyasamy Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

pperiyasamy Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyrtapz Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

pperiyasamy Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

pperiyasamy Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

kyrtapz Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

huiran0826 commented Nov 6, 2025

Uh oh!

openshift-ci-robot commented Nov 6, 2025

Uh oh!

huiran0826 commented Nov 6, 2025

Uh oh!

openshift-ci-robot commented Nov 6, 2025

Uh oh!

kyrtapz commented Nov 6, 2025

Uh oh!

pperiyasamy commented Nov 6, 2025

Uh oh!

openshift-ci bot commented Nov 6, 2025

Uh oh!

kyrtapz commented Nov 6, 2025

Uh oh!

openshift-ci bot commented Nov 6, 2025

Uh oh!

openshift-ci bot commented Nov 6, 2025

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 6, 2025

Uh oh!

kyrtapz commented Nov 6, 2025

Uh oh!

openshift-cherrypick-robot commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pperiyasamy Nov 6, 2025 •

edited

Loading

kyrtapz Nov 6, 2025 •

edited

Loading

coderabbitai bot commented Nov 6, 2025 •

edited

Loading