do not invoke next.Close when connection not established

[pperiyasamy]

This fixes too many stale interface and ovs port entries upon service request failure (due to IP Pool Empty error).

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

Description

Issue link

How Has This Been Tested?

• Added unit testing to cover
• Tested manually
• Tested by integration testing
• Have not tested

Types of changes

• Bug fix
• New functionallity
• Documentation
• Refactoring
• CI

[edwarnicke]

I'd recommend not using the Path to determine 'established'... instead fall back on fundamentals.

Since the normal pattern is:

1. Decisions are made on the call direction of 'Request'
2. Action is taken on the return direction of 'Request'

If a particular chain element experiences an error trying to do its work, it should call Close from itself down, to signal to all downstream chain elements to clean up after themselves, and then return an error.

But that 'Close' should only occur if the error originates in the chain element itself, if the error is returned from the downstream chain, the chain element should simply return the error up the chain. Since the chain element does its work on the return chain, simply returning the error means the chain element doesn't create the state it would otherwise need to cleanup.

[pperiyasamy]

I'd recommend not using the Path to determine 'established'... instead fall back on fundamentals.

yes, I totally forgot about this, now changed it back to use local metadata cache for connection established check.

Since the normal pattern is:

1. Decisions are made on the call direction of 'Request'
2. Action is taken on the return direction of 'Request'

If a particular chain element experiences an error trying to do its work, it should call Close from itself down, to signal to all downstream chain elements to clean up after themselves, and then return an error.

But that 'Close' should only occur if the error originates in the chain element itself, if the error is returned from the downstream chain, the chain element should simply return the error up the chain. Since the chain element does its work on the return chain, simply returning the error means the chain element doesn't create the state it would otherwise need to cleanup.

Yes Ed, this pattern is followed in client chain elements. The server chain elements slight vary from this i.e. do_its_work -> return_if_work_fails -> next.Request -> cleanup_its_work_done_previously_when_next_request_fails -> return. Hope this is fine with you.

[edwarnicke]

Yes Ed, this pattern is followed in client chain elements. The server chain elements slight vary from this i.e. do_its_work -> return_if_work_fails -> next.Request -> cleanup_its_work_done_previously_when_next_request_fails -> return. Hope this is fine with you.

While there is always an exception to every rule... as a rule I find it is actually good to follow the 'add information/make decisions' on the call side of the Request and 'do work' on the return side works best for most server chain elements (especially for forwarders that are passthroughs).

[pperiyasamy]

Yes Ed, this pattern is followed in client chain elements. The server chain elements slight vary from this i.e. do_its_work -> return_if_work_fails -> next.Request -> cleanup_its_work_done_previously_when_next_request_fails -> return. Hope this is fine with you.

While there is always an exception to every rule... as a rule I find it is actually good to follow the 'add information/make decisions' on the call side of the Request and 'do work' on the return side works best for most server chain elements (especially for forwarders that are passthroughs).

@edwarnicke created an issue #25 to track this work separately as it needs changes across other sdk repos too and realign of xconnect endpoint chain.