milo icon indicating copy to clipboard operation
milo copied to clipboard

Add support for client redundancy via subscription transfer (#523)

Open marcus-orchard opened this issue 5 years ago • 23 comments

Signed-off-by: Marcus Orchard [email protected]

marcus-orchard avatar Aug 19 '19 15:08 marcus-orchard

Can one of the admins verify this patch?

eclipse-milo-bot avatar Aug 19 '19 15:08 eclipse-milo-bot

Hey Marcus,

It looks like you have an ECA on file for [email protected] but you've authored this commit with your Emerson email address.

kevinherron avatar Aug 19 '19 15:08 kevinherron

Oh crud. Ok, let me try to reauthor that.

marcus-orchard avatar Aug 19 '19 15:08 marcus-orchard

Alright, looks like everything is happy now.

marcus-orchard avatar Aug 19 '19 16:08 marcus-orchard

rebuild

kevinherron avatar Aug 19 '19 16:08 kevinherron

Thanks Marcus, this looks really good on the surface. I'm going to check it out and take a closer look sometime today. I might try to build a transfer example using it to get a feel for the API.

kevinherron avatar Aug 19 '19 16:08 kevinherron

Ok, just so you have my approach for reference (leaving out obvious null and empty checks):

  1. Verify diagnostics are enabled by querying the EnabledFlag on the ServerDiagnostics node.
  2. Call getSessionDiagnosticsArray on SessionsDiagnosticsSummaryNode.
  3. Filter the results for items containing my expected session name, ordered by that name and select the head (name is appended with tick stamp).
  4. Then I call getObjectNode on the client address space and provide the sessionId of the session I just retrieved and classOf[SessionDiagnosticsObjectNode] as the arguments.
  5. Finally, I call getSubscriptionDiagnosticsArray on the object node returned.

Now I have subscription diagnostic information for each subscription I want to transfer.

  1. I call transferSubscription on the UaClient.
  2. From here I can call GetMonitoredItems for each subscription.
  3. I take the results of that and get the first client handle, then lookup the subscription configuration associated with that client handle. Now I know which server subscription id matches which of my subscription configurations.
  4. Then I construct the TransferMonitoredItemsRequest for each grouping of monitoredItem/notification handlers that I need to reconstruct.
  5. This also requires me to create the MonitoredItemTransferRequests for each item, using the earlier GetMonitoredItems results for server handles and and my configuration.
  6. Finally I call transferSubscription on the UaSubscriptionManager passing in the information from my subscription diagnostics, subscription config, and the constructed TransferMonitoredItemRequests.

I think I wrote this in a way to allow other approaches as needed by other implementations, but having mine as reference I thought my be helpful.

marcus-orchard avatar Aug 19 '19 17:08 marcus-orchard

I want to see if I can piggy back on this to add a "local" transfer API as well. That way there's an easy to use transfer for when you're transferring on the same/local machine and have UaSubscription and UaMonitoredItems on hand, and this more sophisticated "remote" transfer option for transferring to a remote machine.

If there's not an easy to use "local" transfer API I'm going to have to spend a lot of time trying to support and explain how to use this "remote" transfer API.

kevinherron avatar Aug 20 '19 15:08 kevinherron

A separate "local" transfer makes sense to me, no need for some of the overhead that the "remote" transfer requires.

marcus-orchard avatar Aug 20 '19 15:08 marcus-orchard

@marcus-orchard do you mind if I start pushing some minor changes to this PR as I review?

kevinherron avatar Aug 20 '19 20:08 kevinherron

@marcus-orchard do you mind if I start pushing some minor changes to this PR as I review?

Not at all. Do you want to address the items you mentioned in your review as well, or would you like me to tackle those?

I think TransferMonitorItemResponse also needs to be refactored to TransferMonitorItemsResponse.

marcus-orchard avatar Aug 20 '19 20:08 marcus-orchard

I'll make the changes, I've got it open right now.

I've intentionally not opted into Java's broken serialization system (and gone towards immutable objects with final fields), so no-arg constructors aren't necessary or desirable.

kevinherron avatar Aug 20 '19 20:08 kevinherron

I like the "trick" where you call modify when "transferring" monitored items to make sure you have current values for all the revised fields. I'm wondering if that same trick might be worth doing for subscriptions, even though you can technically get that information from the diagnostics. If an application configuration had all the other original/requested subscription values it might eliminate the need to read from the diagnostics?

kevinherron avatar Aug 20 '19 20:08 kevinherron

So, I did consider this at one point.

I think where I abandoned the idea is when I realized I have to query the subscription diagnostics anyways to get the subscription Ids to transfer, so I just grab the diagnostics when I'm querying that, and calling modifySubscription would be more overhead.

What initially interested me in this approach is that it reduces the information you need to provide when transferring a subscription, leading me to entertain the idea that it could potentially satisfy a larger number of unknown use cases.

But, another part of me thinks, "if they have a means to know the subscriptionId that the server assigned, I don't know why they wouldn't have the means to also know publishingInterval, lifetimeCount, etc." ; and if that is true then, again, calling modifySubscription would be just more overhead.

Does that make sense? I did consider this as an option, but I'm open to the idea that I missed something that moves the needle more toward using the modifySubscription approach.

marcus-orchard avatar Aug 20 '19 20:08 marcus-orchard

Maybe to be safe we add an overload that uses this approach on the off chance there is a customer that somehow knows which subscriptionId's they need to transfer without using diagnostics and that only has access to the subscriptionId and their config?

marcus-orchard avatar Aug 20 '19 21:08 marcus-orchard

I get what you're saying. I forgot that you don't even have subscription ids as the client taking over.

An overload might not be a bad idea, I'll think about it a bit.

If a client did somehow have subscription ids, or if there were some method that could be called to obtain subscription ids (not sure how that would work... call a method to get existing session ids belonging to the same user, then call a method against the most recent session id to get existing subscriptions?), it might be possible to do a transfer without relying on diagnostics.

What I'm getting at is, while right now it appears diagnostics are required, I'm wondering if we can learn something from implementing this and I can use it to make some suggestions for OPC UA 1.05 on how to make client redundancy and subscription transfer suck less.

kevinherron avatar Aug 20 '19 21:08 kevinherron

@marcus-orchard what do you think about taking the transferMonitoredItems methods off of the UaSubscription interface and just giving them local visibility in OpcUaSubscription?

Is there some scenario where a user might be calling these themselves rather than letting UaSubscriptionManager handle the transfer of the subscription and monitored items? Making these visible to the user seems like it might increase the odds of misuse.

kevinherron avatar Aug 21 '19 22:08 kevinherron

TODO: my local transfer implementation is still slightly broken because it doesn't offer the caller a chance to hook up monitored item event and value callbacks before the notification semaphore is released.

kevinherron avatar Aug 21 '19 23:08 kevinherron

@marcus-orchard what do you think about taking the transferMonitoredItems methods off of the UaSubscription interface and just giving them local visibility in OpcUaSubscription?

Is there some scenario where a user might be calling these themselves rather than letting UaSubscriptionManager handle the transfer of the subscription and monitored items? Making these visible to the user seems like it might increase the odds of misuse.

Agreed.

marcus-orchard avatar Aug 22 '19 15:08 marcus-orchard

Hey, just wanted to let you know I haven't forgotten this, I've just been wildly busy the last couple weeks at both work and home. Things should calm down after next week.

kevinherron avatar Sep 10 '19 15:09 kevinherron

No worries at all and I was worried you thought the same about me. I'll keep a look out for activity and keep pressing on with other things.

marcus-orchard avatar Sep 10 '19 20:09 marcus-orchard

Hello there, any chance of this PR being merged in the foreseeable future? Or is there some blocking problem with it? I would have a use for inter process subscriptions transfer.

FSangouard avatar Aug 31 '22 16:08 FSangouard

@FSangouard I don't know... it will take some work to make it mergeable and then try to understand it and make sure it's (still) sane.

I probably won't be able to take a good look at this until after I attend the OPC interoperability event in October. In the mean time I am working hard towards OPC UA 1.05 support that I plan to test at the event.

kevinherron avatar Aug 31 '22 16:08 kevinherron