compute-domain-daemon: skip SIGUSR1 when DNS mapping update only removes nodes

[AkshatDudeja77]

Fixes #913

The IMEXDaemonUpdateLoopWithDNSNames currently sends SIGUSR1 to the IMEX
daemon whenever the DNS/IP mapping changes.

However, when the update only removes nodes (and does not add new ones),
forcing the daemon to re-resolve and reconnect is unnecessary.

This change tracks the previous mapping size and skips sending SIGUSR1 when
the updated mapping strictly removes nodes.

Before:
SIGUSR1 triggered whenever the mapping changed.

After:
SIGUSR1 triggered only when nodes are added or mappings otherwise change,
but not when nodes are only removed.

This implements the TODO in the code suggesting that reconnects can be
skipped when the new IP set only removes addresses compared to the old set.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[jgehrcke]

when the update only removes nodes (and does not add new ones)

That's a great goal. But how do we detect this situation with sets being our central data structures?

Write two sets down, on a piece of paper. This could look like this:

Set A:  item1, item2, item3
Set B:  item1, item2, item4

What do you need to do in order to find out if set A has an item removed compared to set B?

Do you need to look at more than just set size?

I genuinely wonder what your goals and incentives are. This is I think your sixth pull request of this style against this repository, and I believe we need to think about providing candid feedback soon :). These are wild times!

[jgehrcke]

logically incorrect, as expressed in regular comment

[jgehrcke]

For context, I still find it quite likely that this is AI-generated noise. Last time I looked into this, I noted:
#863 (comment)

[AkshatDudeja77]

@jgehrcke thank you for your point about comparing sets rather than just sizes, I finally do understand what you're saying.

My intention with the current change was only to avoid sending SIGUSR1 when the update strictly removes nodes (i.e. when the mapping shrinks). In cases where
the size stays the same but the elements differ (like old set: A,B,C and new set: A,B,D), we would still send SIGUSR1 because a new peer appeared.

For transparency: I used AI to draft the initial idea here, but I’ve reviewed and adjusted the code and tests myself.

[rajatchopra]

@AkshatDudeja77 Can you write some unit testcases around it

[AkshatDudeja77]

@AkshatDudeja77 Can you write some unit testcases around it

@rajatchopra Refactored the SIGUSR1 decision into a helper and added unit tests covering no-change, pure-removal, additions, replacements, remove+add, and fresh-process cases.

This now explicitly checks for newly added peers rather than relying on size or subset assumptions.

Please let me know if there are additional edge cases you'd like covered

[jgehrcke]

Do we still need updated as an argument into this function? Is it directly implied by oldIPs and newIPs?

[AkshatDudeja77]

Good point, updated is handled at the call site now, so i removed it from the helper and aligned the tests accordingly

[AkshatDudeja77]

Updated the docstring to reflect the current function behavior after removing updated.

Also fixed the IPSet usage at the call site by explicitly constructing the new IP set from the DNS mapping to ensure correct types

[k8s-triage-robot]

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla