node-manager: fix race-condition

[marseel]

There was potential race-conditions in handling nodes in manager. In background-sync where we were releasing manager mutex before holding entry mutex which would result in updating node information with stale informantion.

This race-condition could cause stale node information up until next background sync or node update, so it was evenually resolved though, but it could take up to 15+ minutes for large clusters, depending on number of nodes.

Follow up from #32577

Edit: seems like this was introduced on main and it's not present on v1.15 branch, so removing release-note.
Regression was introduced by #29415
I've requested review from Fernand, just to make sure I haven't missed anything.

[marseel]

/test

[derailed]

@marseel Thank you for reviewing these Marcel!
I think it make sense but also could entail a dead lock. If someone else has a lock on the entry, the node map could no longer be read. This introduce a dependency and also would prevent the sync from exiting should the context be canceled. Given the reconciliation loop at stake, I think it's safer to read potentially stale state.
Am I wrong about that?

[marseel]

I think it make sense but also could entail a dead lock. If someone else has a lock on the entry, the node map could no longer be read. This introduce a dependency and also would prevent the sync from exiting should the context be canceled. Given the reconciliation loop at stake, I think it's safer to read potentially stale state.
Am I wrong about that?

This is the official way we should be always ordering mutexes of nodeManager:

cilium/pkg/node/manager/manager.go

Lines 88 to 104 in 598209e

	// mutex is the lock protecting access to the nodes map. The mutex must
	// be held for any access of the nodes map.
	//
	// The manager mutex works together with the entry mutex in the
	// following way to minimize the duration the manager mutex is held:
	//
	// 1. Acquire manager mutex to safely access nodes map and to retrieve
	// node entry.
	// 2. Acquire mutex of the entry while the manager mutex is still held.
	// This guarantees that no change to the entry has happened.
	// 3. Release of the manager mutex to unblock changes or reads to other
	// node entries.
	// 4. Change of entry data or performing of datapath interactions
	// 5. Release of the entry mutex
	//
	// If both the nodeEntry.mutex and Manager.mutex must be held, then the
	// Manager.mutex must *always* be acquired first.

Also, we had it in this order forever. So my question is if that change was intended in #29415 (if yes, what was the reason?) or accidental.

[derailed]

@marseel Ah I see. Tx for the correction Marcel. This is indeed my mistake ;(

[ldelossa]

Change looks good to me, and now aligns with the comments in the manager 👍