Skip to content

Conversation

@mlguerrero12
Copy link
Collaborator

This commit addresses a potential issue where IP allocations could be accidentally deleted when new pods with the same name and namespace are created in statefulset scenarios. The logic now correctly checks if a pod with the matching name and namespace exists and is not marked for deletion. If such a pod exists, the deallocation is skipped, preventing conflicts. The commit also includes clarified logging messages for improved debugging.

@mlguerrero12 mlguerrero12 requested a review from maiqueb as a code owner July 14, 2025 10:14
@mlguerrero12
Copy link
Collaborator Author

@bpickard22, @thomasferrandiz, PTAL

@coveralls
Copy link

coveralls commented Jul 14, 2025

Pull Request Test Coverage Report for Build 16272872047

Details

  • 6 of 12 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 37.676%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controlloop/pod.go 6 12 50.0%
Totals Coverage Status
Change from base Build 16272857961: 0.04%
Covered Lines: 1443
Relevant Lines: 3830

💛 - Coveralls

This commit addresses a potential issue where IP allocations
could be accidentally deleted when new pods with the same name
and namespace are created in statefulset scenarios. The logic
now correctly checks if a pod with the matching name and namespace
exists and is not marked for deletion. If such a pod exists,
the deallocation is skipped, preventing conflicts. The commit
also includes clarified logging messages for improved debugging.

Signed-off-by: Marcelo <[email protected]>

// The allocation could belong to a new pod with the same name and namespace. Stateful set scenarios.
// The previous pod should be gone by the time a pod deletion event is received.
if newPod, err := pc.k8sClient.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{}); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we check if the pod is owned by a statefulset so we dont break the behavior for non stateful deployments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can check the ownerref of the pod to see if it is part of a statefulset, if it is we release the ip, if not we proceed to use the existing logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't apply to non stateful deployments. Pod name is different.

Copy link
Collaborator

@bpickard22 bpickard22 Jul 15, 2025

Choose a reason for hiding this comment

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

@maiqueb we can assume the user is following naming conventions i guess

@bpickard22 bpickard22 merged commit 9dd10f0 into k8snetworkplumbingwg:master Jul 15, 2025
11 checks passed
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