Add gRPC command interface - #147
GabyUnalaq wants to merge 9 commits into
Conversation
GabyUnalaq
added
enhancement
GabyUnalaq
changed the title
|
This PR is blocked until the proto fetching method accounts for the grpc_api proto file, needed for the command interface: |
62c1430 to
aefbef1
Compare
inf17101
left a comment
inf17101
left a comment
There was a problem hiding this comment.
Intermediate review state.
| ... | ||
| ``` | ||
|
|
||
| This requires the `grpc` extra: |
There was a problem hiding this comment.
Why do we call the command interface feature grpc in the python sdk and command_interface in the Rust SDK?
The whole feature is called command_interface and grpc is just a protocol that is pluggable in Ankaios.
There was a problem hiding this comment.
Renamed it to "command", and also updated the readme and the examples to exclude the protocol name. Also added some comments to make clear to the user that using the command interface means a grpc connection.
| self._state = CommandInterfaceState.TERMINATED | ||
| if self._call is not None: | ||
| self._call.cancel() | ||
| if self._reader_thread is not None: | ||
| self._reader_thread.join(timeout=2) | ||
| if self._reader_thread.is_alive(): | ||
| self._logger.error("Reader thread did not stop.") | ||
| self._reader_thread = None | ||
| if self._channel is not None: | ||
| self._channel.close() | ||
| self._channel = None | ||
| self._call = None |
There was a problem hiding this comment.
As self._state is a lock, I assume this class should be thread safe. This does not look thread safe. Assume connect and disconnect are called from two threads concurrently. First disconnect is executed until the state is set to TERMINATED. Then connect is executed. As the state is TERMINATED connect will not fail and _call, _read_thread and _channel will be set to the new values. Now disconnect is continued. The new _call, _read_thread and _channel will be terminated, while the original ones are untouched.
There was a problem hiding this comment.
There is indeed an issue here. The problem is that it is identical on the control interface side. My suggestion would be to leave it as is and create another issue with a Pr for fixing this everywhere, so that there are no race conditions regarding the state afterwards.
There was a problem hiding this comment.
Then we should look at the control interface side and check if the problem is there as well. If it is first make a PR to fix it there and only then merge this PR. We should not introduce a new bug, only there is already a similar one somewhere else.
There was a problem hiding this comment.
I have created a PR (#152) to fix them on the control interface side. Once it is merged to main, I will rebase this branch and fix them properly here as well.
There was a problem hiding this comment.
This should be resolved now.
| def _grpc_target(self) -> str: | ||
| """ | ||
| Returns server_url as a bare host:port gRPC channel target. | ||
|
|
||
| :returns: The channel target. | ||
| :rtype: str | ||
| """ | ||
| for prefix in self._URL_SCHEME_PREFIXES: | ||
| if self._server_url.startswith(prefix): | ||
| return self._server_url[len(prefix):] | ||
| return self._server_url | ||
|
|
||
| def _build_channel(self) -> grpc.Channel: |
There was a problem hiding this comment.
Improve the ordering of method definitions in this file. The caller should be above the callee. The callees should be near the caller. The position of the setter/getters is OK. Also they are technically callees, they belong to the field definition in the top.
| response._response = None | ||
| response.content_type = ResponseType.CONTROL_INTERFACE_ACCEPTED | ||
| response.content = None | ||
| logger.debug( |
There was a problem hiding this comment.
Why are we logging here about a received object? Nothing is received here. We only create an object.
There was a problem hiding this comment.
This is the moment when we know the request ID and the type of the response. The same case is in the other constructor-like methods.
There was a problem hiding this comment.
What I do not like, is that message is received by control interface, but the logging is done in response. The logging is a side effect of creating a response object. This is just unexpected.
| @_state.setter | ||
| def _state(self, value: CommandInterfaceState) -> None: | ||
| """ | ||
| Set the current state of the connection. | ||
|
|
||
| :param value: The new state to set. | ||
| :type value: CommandInterfaceState | ||
| """ | ||
| with self._state_lock: | ||
| self._state_value = value | ||
|
|
||
| @property | ||
| def connected(self) -> bool: | ||
| """ | ||
| Check if the gRPC connection is established. | ||
|
|
||
| :returns: True if connected, False otherwise. | ||
| :rtype: bool | ||
| """ | ||
| return self._state == CommandInterfaceState.CONNECTED |
There was a problem hiding this comment.
I have no idea why at the moment a lock is used here at all. I would expect that assignment of self._state is atomic.
There was a problem hiding this comment.
This issue is similar to one above, which can be tackled in a separate PR. Currently it is done in the same way as the control interface side.
There was a problem hiding this comment.
Then we should create an PR for this.
There was a problem hiding this comment.
This should be resolved now.
| with self._state_lock: | ||
| return self._state_value |
There was a problem hiding this comment.
Reading of _state_value is also atomic. We do not need this lock either. I guess we also do not need this getter at all.
There was a problem hiding this comment.
This should be resolved now.
9ddce64 to
e9051f1
Compare
|
This branch has not been deployed
3 participants
Issues: eclipse-ankaios/ank-sdk-rust#35 eclipse-ankaios/ankaios#764
Adds the possibility to connect to the Ankaios server directly, through the gRPC command interface.
Definition of Done
The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.