Bugfixes #167

mladjan-gadzic · 2025-09-04T12:01:00Z

UFM Cleanup on Pod Lifecycle Events: Fixed issue where GUIDs weren't properly removed from UFM (Unified Fabric Manager) when pods completed (success/error) or were deleted.
GUID Reallocation Conflicts: Added logic to remove existing GUID allocations from UFM before assigning a new partition (PKey) to prevent conflicts when the same GUID is reused.
Pod State Handling: Improved pod lifecycle management by treating finished pods (succeeded/failed) the same as deleted pods for cleanup purposes.

coveralls · 2025-09-05T09:40:19Z

Pull Request Test Coverage Report for Build 18346261465

Details

103 of 137 (75.18%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+6.4%) to 46.579%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/sm/plugins/noop/noop.go	0	2	0.0%
pkg/daemon/daemon.go	58	90	64.44%

Files with Coverage Reduction	New Missed Lines	%
pkg/daemon/daemon.go	1	15.58%

Totals
Change from base Build 18335558999:	6.4%
Covered Lines:	708
Relevant Lines:	1520

💛 - Coveralls

almaslennikov

Hi @mladjan-gadzic, thank you for your contribution. Left some comments. Could you also cover the changed functionality with unit tests?

pkg/watcher/handler/pod.go

pkg/daemon/daemon.go

almaslennikov · 2025-09-08T07:59:00Z

pkg/daemon/daemon.go

+					log.Info().Msgf("matched guid %s to pod %s, removing", guidAddr, guidPodEntry)
+					guidList = append(guidList, guidAddr)
+				} else {
+					log.Warn().Msgf("guid %s is allocated to another pod %s not %s, not removing",


When can this happen?

Added comment for this.

Could you point me to the comment? I can't find it

almaslennikov · 2025-09-08T08:03:05Z

Hi @mladjan-gadzic there is also a failing DCO check: https://github.com/Mellanox/ib-kubernetes/pull/167/checks?check_run_id=49668982984

Please, sign the commits in your PR and re-push them to the branch

rollandf · 2025-09-08T12:09:13Z

@mladjan-gadzic can you rebase and handle conflict?
Also maybe stash the commits to one?

mladjan-gadzic · 2025-09-08T13:18:04Z

@almaslennikov thanks for the review. i've pushed required changes. once you're okay with those, please resolve comments.

@rollandf thanks for the comment. i've squashed everything under one commit. i am going to resolve conflicts soon.

almaslennikov · 2025-09-18T07:34:40Z

pkg/daemon/daemon.go

+
+	// Remove stale GUIDs that are no longer in use by the subnet manager
+	// This handles cleanup of GUIDs from deleted/finished pods
+	for allocatedGUID, podNetworkID := range d.guidPodNetworkMap {


At the time when syncGuidPool is called, the d.guidPodNetworkMap is empty because it's populated in the d.initPool method. Since we run both only once, let's combine them into a single function and properly handle the order of operations

mladjan-gadzic · 2025-10-06T07:38:48Z

@almaslennikov, please, let me know if any additional changes are needed.

almaslennikov · 2025-10-07T14:18:00Z

@mladjan-gadzic Please, resolve the merge conflicts. After merging #168 I see there are several places where syncGuidPool is called. Could you align your PR with the new logic?

almaslennikov

@mladjan-gadzic could you test that the changes in this PR work? The current version doesn't build

almaslennikov · 2025-10-08T09:15:10Z

pkg/daemon/daemon.go

+	}
+
+	// Initialize guid pool with existing pods and sync with subnet manager
+	err = daemonInstance.initGUIDPool()


I suggest to not run it here anymore since we only should start the actual logic if the instance becomes a leader. In the becomeLeader function we are already running initGUIDPool

Let's refactor this function to return the daemon fully formed at the end

almaslennikov · 2025-10-08T09:16:11Z

pkg/daemon/daemon.go

 	log.Info().Msg("delete periodic update finished")
 }

+<<<<<<< HEAD


There are merge artifacts left here

UFM Cleanup on Pod Lifecycle Events: Fixed issue where GUIDs weren't properly removed from UFM (Unified Fabric Manager) when pods completed (success/error) or were deleted. GUID Reallocation Conflicts: Added logic to remove existing GUID allocations from UFM before assigning a new partition (PKey) to prevent conflicts when the same GUID is reused. Pod State Handling: Improved pod lifecycle management by treating finished pods (succeeded/failed) the same as deleted pods for cleanup purposes. Signed-off-by: Mladjan Gadzic <[email protected]>

mladjan-gadzic · 2025-10-08T13:27:21Z

@mladjan-gadzic could you test that the changes in this PR work? The current version doesn't build

i've run make build/all/test/lint locally and it succeeds.

almaslennikov · 2025-10-08T13:57:08Z

Makefile

 GO      = go

-TIMEOUT = 15
+TIMEOUT = 30


Do we need this change?

i've noticed that tests were flaky without this. it might be something locally to my env.

mladjan-gadzic · 2025-10-14T14:39:12Z

@almaslennikov thanks for merging this!

mladjan-gadzic force-pushed the bugfixes branch from 35562ac to 70eace6 Compare September 4, 2025 12:02

almaslennikov reviewed Sep 8, 2025

View reviewed changes

mladjan-gadzic force-pushed the bugfixes branch from 46ab9d1 to 8d8eafc Compare September 8, 2025 08:32

mladjan-gadzic force-pushed the bugfixes branch from 54de066 to c9dee03 Compare September 8, 2025 12:59

mladjan-gadzic force-pushed the bugfixes branch from c9dee03 to a72cdef Compare September 8, 2025 14:01

almaslennikov requested changes Sep 18, 2025

View reviewed changes

mladjan-gadzic force-pushed the bugfixes branch 2 times, most recently from 0301ca3 to e12ba14 Compare September 23, 2025 11:25

mladjan-gadzic force-pushed the bugfixes branch from e12ba14 to 7b07865 Compare October 7, 2025 19:28

almaslennikov requested changes Oct 8, 2025

View reviewed changes

mladjan-gadzic force-pushed the bugfixes branch from 7b07865 to 9e8319e Compare October 8, 2025 13:26

almaslennikov approved these changes Oct 14, 2025

View reviewed changes

almaslennikov merged commit 20e4e6b into Mellanox:master Oct 14, 2025
7 of 8 checks passed

Bugfixes #167

Bugfixes #167

Uh oh!

Conversation

mladjan-gadzic commented Sep 4, 2025

Uh oh!

coveralls commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18346261465

Details

💛 - Coveralls

Uh oh!

almaslennikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

almaslennikov commented Sep 8, 2025

Uh oh!

rollandf commented Sep 8, 2025

Uh oh!

mladjan-gadzic commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mladjan-gadzic commented Oct 6, 2025

Uh oh!

almaslennikov commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

almaslennikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mladjan-gadzic commented Oct 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mladjan-gadzic commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 5, 2025 •

edited

Loading

mladjan-gadzic commented Sep 8, 2025 •

edited

Loading

almaslennikov commented Oct 7, 2025 •

edited

Loading