Skip to content

Get node ID over network#5

Merged
wenzhang-unity merged 3 commits into
devfrom
wen/node-id-logic
Dec 15, 2022
Merged

Get node ID over network#5
wenzhang-unity merged 3 commits into
devfrom
wen/node-id-logic

Conversation

@wenzhang-unity
Copy link
Copy Markdown
Owner

Implement a way for nodes to self-assign arbitrary but distinct node IDs before cluster initialization. The simple (naive) process involves each node broadcasting a unique identifier over multicast. This is useful in cases like Disguise RenderStream where the node needs to join the cluster but doesn't care what its node ID is.

See DiguiseRenderStreamWithCluster.NegotiateNodeID for details.

Copy link
Copy Markdown
Collaborator

@FrederickUnity FrederickUnity left a comment

Choose a reason for hiding this comment

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

I know there is a lot of comments but they are more ideas for potential improvements, they don't have to be done to merge (especially we want to have it for tomorrow).

using var udpClient = new UdpClient();
udpClient.EnableMulticast(address,
clusterParams.Port,
clusterParams.AdapterName,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Brainstorm: From what I understand we are negotiating on the same clusterParams.AdapterName, which makes perfect sense. However, if not specified it will "pick one" and use it to negotiate. This network might then be different (if code diverge in the future) to the one that cluster display will then choose to propagate the state. Should we put back the adapter name that we found in clusterParams.AdapterName to be sure both are done on the same network?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes, maybe we can combine this idea with your suggestion for picking the best NIC.

continue;
}

firstUpNic ??= nic;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Brainstorm: If I remember correctly this part of code is quite similar to what we are doing in CD to automatically select a network adapter to use for state propagation. However, I wonder if we could do a little bit better for Disguise.

From what I understand render stream nodes will have at least 2 NIC up and running:

  1. For transmission of video signal
  2. General networking

#1 will most likely be a high performance & reliable network, especially if using uncompressed video, it will be a network of at least 10 gbps, most likely 25 and it could event be 100 gbps. Even better, it will be configured for nice reliable udp multicast (they are using SMPTE 2110 under the hood that is using udp multicast).
#2 will most likely be any network and we have no idea how it will be configured, how it is reliable, ...

So based on that I think we should probably try to use #1. And the easiest way to identify it would be to use the one with the highest speed (as opposed to the first one we find as this code is currently doing).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Do you know of the API to get the speed of the NIC reliably? There's NetworkInterface.Speed, but I don't know if that's the preferred way.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AFAIK it returns what the NIC driver reports to windows, so that's probably good enough.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably a good idea to exclude loopback and wireless adapters using NetworkInterface.NetworkInterfaceType


// Allow multiple ClusterDisplay applications to bind on the same address and port. Useful for when running
// multiple nodes locally and unit testing.
// Food for thought: Does it have a performance cost? Do we want to have it configurable or disabled in some
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Opinion: I don't think performance matter that much for this part of the code, this is only a negotiation when starting...

Comment thread DisguiseUnityRenderStream/Runtime/DisguiseRenderStreamWithClustercs.cs Outdated
Comment thread DisguiseUnityRenderStream/Runtime/DisguiseRenderStreamWithClustercs.cs Outdated
public static (string name, IPAddress address) SelectNetworkInterface(string adapterName)
{
var nics = NetworkInterface.GetAllNetworkInterfaces();
NetworkInterface firstUpNic = null;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Minor: maybe call those two variables: implicitNic? Or bestNic? Because it is not necessarily the first one anymore.

@wenzhang-unity wenzhang-unity merged commit 284008a into dev Dec 15, 2022
@wenzhang-unity wenzhang-unity deleted the wen/node-id-logic branch December 15, 2022 18:40
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.

2 participants