From 4229dd6bd47a04b62e5489ea76729c31afc5db26 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Fri, 2 Sep 2022 00:35:26 +0000 Subject: [PATCH 1/7] [ycable] cleanup logic for creating grpc future ready Signed-off-by: vaibhav-dahiya --- .../ycable/ycable_utilities/y_cable_helper.py | 71 ++++++++++++------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py index c037c22dc..fa5061112 100644 --- a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py +++ b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py @@ -332,7 +332,6 @@ def wrapper(*args, **kwargs): return wrapper - def retry_setup_grpc_channel_for_port(port, asic_index): global grpc_port_stubs @@ -409,35 +408,60 @@ def get_grpc_credentials(type, kvp): return credential -def create_channel(type,level, kvp, soc_ip): +def connect_channel(channel, stub, port): + channel_ready = grpc.channel_ready_future(channel) retries = 3 + for _ in range(retries): + try: + channel_ready.result(timeout=2) + except grpc.FutureTimeoutError: + helper_logger.log_warning("gRPC port {} state changed to SHUTDOWN".format(port)) + else: + break - if type == "secure": - credential = get_grpc_credentials(level, kvp) - target_name = kvp.get("grpc_ssl_credential", None) - if credential is None or target_name is None: - return (None, None) +def create_channel(type,level, kvp, soc_ip, port): + + channel, stub = None, None + def wait_for_state_change(channel_connectivity): + if channel_connectivity == grpc.ChannelConnectivity.TRANSIENT_FAILURE: + helper_logger.log_notice("gRPC port {} state changed to TRANSIENT_FAILURE".format(port)) + if channel_connectivity == grpc.ChannelConnectivity.CONNECTING: + helper_logger.log_notice("gRPC port {} state changed to CONNECTING".format(port)) + if channel_connectivity == grpc.ChannelConnectivity.READY: + helper_logger.log_notice("gRPC port {} state changed to READY".format(port)) + if channel_connectivity == grpc.ChannelConnectivity.IDLE: + helper_logger.log_notice("gRPC port {} state changed to IDLE".format(port)) + if channel_connectivity == grpc.ChannelConnectivity.SHUTDOWN: + helper_logger.log_notice("gRPC port {} state changed to SHUTDOWN".format(port)) + + + if type == "secure": + credential = get_grpc_credentials(level, kvp) + target_name = kvp.get("grpc_ssl_credential", None) + if credential is None or target_name is None: + return (None, None) - GRPC_CLIENT_OPTIONS.append(('grpc.ssl_target_name_override', '{}'.format(target_name))) + GRPC_CLIENT_OPTIONS.append(('grpc.ssl_target_name_override', '{}'.format(target_name))) - channel = grpc.secure_channel("{}:{}".format(soc_ip, GRPC_PORT), credential, options=GRPC_CLIENT_OPTIONS) - else: - channel = grpc.insecure_channel("{}:{}".format(soc_ip, GRPC_PORT), options=GRPC_CLIENT_OPTIONS) + channel = grpc.secure_channel("{}:{}".format(soc_ip, GRPC_PORT), credential, options=GRPC_CLIENT_OPTIONS) + else: + channel = grpc.insecure_channel("{}:{}".format(soc_ip, GRPC_PORT), options=GRPC_CLIENT_OPTIONS) - stub = linkmgr_grpc_driver_pb2_grpc.DualToRActiveStub(channel) - channel_ready = grpc.channel_ready_future(channel) + stub = linkmgr_grpc_driver_pb2_grpc.DualToRActiveStub(channel) - try: - channel_ready.result(timeout=2) - except grpc.FutureTimeoutError: - channel = None - stub = None - else: - break + if channel is not None: + channel.subscribe(wait_for_state_change) + + connect_channel(channel, stub, port) + if channel is None: + helper_logger.log_notice("gRPC port {} channel is None".format(port)) + if stub is None: + helper_logger.log_notice("gRPC port {} stub is None".format(port)) + return channel, stub def setup_grpc_channel_for_port(port, soc_ip): @@ -490,12 +514,7 @@ def setup_grpc_channel_for_port(port, soc_ip): kvp = dict(fvs) - channel, stub = create_channel(type, level, kvp, soc_ip) - - if stub is None: - helper_logger.log_warning("stub was not setup for gRPC soc ip {} port {}, no gRPC soc server running ?".format(soc_ip, port)) - if channel is None: - helper_logger.log_warning("channel was not setup for gRPC soc ip {} port {}, no gRPC server running ?".format(soc_ip, port)) + channel, stub = create_channel(type, level, kvp, soc_ip, port) return channel, stub From c8404397c351fd8d98be3709c0df107a931090fe Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Tue, 6 Sep 2022 23:33:37 +0000 Subject: [PATCH 2/7] add some comments Signed-off-by: vaibhav-dahiya --- .../ycable/ycable_utilities/y_cable_helper.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py index fa5061112..a0601f446 100644 --- a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py +++ b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py @@ -456,12 +456,12 @@ def wait_for_state_change(channel_connectivity): if channel is not None: channel.subscribe(wait_for_state_change) - connect_channel(channel, stub, port) - if channel is None: - helper_logger.log_notice("gRPC port {} channel is None".format(port)) - if stub is None: - helper_logger.log_notice("gRPC port {} stub is None".format(port)) - + #connect_channel(channel, stub, port) + """ + Removing the connect channel call for now, since it is not required for normal gRPC + and all use cases seem to work without it + """ + return channel, stub def setup_grpc_channel_for_port(port, soc_ip): @@ -516,6 +516,11 @@ def setup_grpc_channel_for_port(port, soc_ip): channel, stub = create_channel(type, level, kvp, soc_ip, port) + if stub is None: + helper_logger.log_warning("stub was not setup for gRPC soc ip {} port {}, no gRPC soc server running ?".format(soc_ip, port)) + if channel is None: + helper_logger.log_warning("channel was not setup for gRPC soc ip {} port {}, no gRPC soc server running ?".format(soc_ip, port)) + return channel, stub def put_init_values_for_grpc_states(port, read_side, hw_mux_cable_tbl, hw_mux_cable_tbl_peer, asic_index): From 2681dbfab1ad1082ab38f28759f636f71db70198 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Tue, 6 Sep 2022 23:37:20 +0000 Subject: [PATCH 3/7] fix formatting Signed-off-by: vaibhav-dahiya --- sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py index a0601f446..9f453a007 100644 --- a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py +++ b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py @@ -421,9 +421,11 @@ def connect_channel(channel, stub, port): else: break -def create_channel(type,level, kvp, soc_ip, port): +def create_channel(type, level, kvp, soc_ip, port): channel, stub = None, None + + #Helper callback to get an channel connectivity state def wait_for_state_change(channel_connectivity): if channel_connectivity == grpc.ChannelConnectivity.TRANSIENT_FAILURE: helper_logger.log_notice("gRPC port {} state changed to TRANSIENT_FAILURE".format(port)) @@ -458,8 +460,9 @@ def wait_for_state_change(channel_connectivity): #connect_channel(channel, stub, port) """ - Removing the connect channel call for now, since it is not required for normal gRPC - and all use cases seem to work without it + Comment the connect channel call for now, since it is not required for normal gRPC I/O + and all use cases seem to work without it. + TODO: will need to see if this subroutine call can be ommitted for all use cases """ return channel, stub From 182d6a05f784d49c84e8a6537c76c206421ff717 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Tue, 6 Sep 2022 23:50:07 +0000 Subject: [PATCH 4/7] fix tests Signed-off-by: vaibhav-dahiya --- sonic-ycabled/tests/test_y_cable_helper.py | 5 +++-- sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/sonic-ycabled/tests/test_y_cable_helper.py b/sonic-ycabled/tests/test_y_cable_helper.py index 3f0d78ce2..80e577e7f 100644 --- a/sonic-ycabled/tests/test_y_cable_helper.py +++ b/sonic-ycabled/tests/test_y_cable_helper.py @@ -4962,9 +4962,10 @@ def test_setup_grpc_channel_for_port(self): with patch('ycable.ycable_utilities.y_cable_helper.y_cable_platform_sfputil') as patched_util: patched_util.get_asic_id_for_logical_port.return_value = 0 - rc = setup_grpc_channel_for_port("Ethernet0", "192.168.0.1") + (channel, stub) = setup_grpc_channel_for_port("Ethernet0", "192.168.0.1") - assert(rc == (None, None)) + assert(stub == True) + assert(channel != None) def test_setup_grpc_channels(self): diff --git a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py index 9f453a007..8e5f6d579 100644 --- a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py +++ b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py @@ -461,8 +461,8 @@ def wait_for_state_change(channel_connectivity): #connect_channel(channel, stub, port) """ Comment the connect channel call for now, since it is not required for normal gRPC I/O - and all use cases seem to work without it. - TODO: will need to see if this subroutine call can be ommitted for all use cases + and all use cases work without it. + TODO: check if this subroutine call can be ommitted for all use cases in future enhancements """ return channel, stub From aaddb92f2b9e978039462dd692d9eabf4f04725c Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Tue, 6 Sep 2022 23:54:10 +0000 Subject: [PATCH 5/7] fix LGTM Signed-off-by: vaibhav-dahiya --- sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py | 1 - 1 file changed, 1 deletion(-) diff --git a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py index 8e5f6d579..3af82fef2 100644 --- a/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py +++ b/sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py @@ -423,7 +423,6 @@ def connect_channel(channel, stub, port): def create_channel(type, level, kvp, soc_ip, port): - channel, stub = None, None #Helper callback to get an channel connectivity state def wait_for_state_change(channel_connectivity): From a4c9ae171fd46c5a294514d78192b64455eb49b2 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Wed, 7 Sep 2022 00:10:53 +0000 Subject: [PATCH 6/7] fix tests Signed-off-by: vaibhav-dahiya --- sonic-ycabled/tests/test_y_cable_helper.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sonic-ycabled/tests/test_y_cable_helper.py b/sonic-ycabled/tests/test_y_cable_helper.py index 80e577e7f..54dc1f57c 100644 --- a/sonic-ycabled/tests/test_y_cable_helper.py +++ b/sonic-ycabled/tests/test_y_cable_helper.py @@ -4967,6 +4967,13 @@ def test_setup_grpc_channel_for_port(self): assert(stub == True) assert(channel != None) + def test_connect_channel(self): + + with patch('grpc.channel_ready_future') as patched_util: + + patched_util.result.return_value = 0 + rc = connect_channel(channel, stub, port) + assert(rc == None) def test_setup_grpc_channels(self): From e986868e1a3af474be4a7182b466f5b7592ed8a9 Mon Sep 17 00:00:00 2001 From: vaibhav-dahiya Date: Wed, 7 Sep 2022 00:23:42 +0000 Subject: [PATCH 7/7] fix the tests Signed-off-by: vaibhav-dahiya --- sonic-ycabled/tests/test_y_cable_helper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sonic-ycabled/tests/test_y_cable_helper.py b/sonic-ycabled/tests/test_y_cable_helper.py index 54dc1f57c..528acbffb 100644 --- a/sonic-ycabled/tests/test_y_cable_helper.py +++ b/sonic-ycabled/tests/test_y_cable_helper.py @@ -4972,7 +4972,7 @@ def test_connect_channel(self): with patch('grpc.channel_ready_future') as patched_util: patched_util.result.return_value = 0 - rc = connect_channel(channel, stub, port) + rc = connect_channel(patched_util, None, None) assert(rc == None) def test_setup_grpc_channels(self):