Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow usage of tcp socket between agent and worker #818

Merged
merged 9 commits into from
Jan 20, 2025

Conversation

jfonseca-aneo
Copy link
Contributor

@jfonseca-aneo jfonseca-aneo commented Jan 14, 2025

Motivation

Java does not support grpc communication between the worker and the agent over unix sockets, this PR builds on the changes introduced in aneoconsulting/ArmoniK.Api#583 to enable the option to use a tcp socket instead.

Description

Mimic the changes in aneoconsulting/ArmoniK.Api#583 and enlarged the kestrel configuration options in the AgentHandler such that it also accepts to listen on a tcp socket. In addition this PR separates the volumes used for shared data and unix socket. Before the PR both of them where mounted on \cache, the updated mount point for shared data is \comm.

Testing

Terraform deployment and just file have been modified to take into account a new variable socket_type, that can be set to either unixdomainsocket or tcp so the deployment configures which socket type should be employed accordingly. With these changes the same set of tests that ran for unix sockets can be executed but using a tcp socket instead .

Impact

The desired life cycle behavior in a deployment with the whole infrastructure is that the worker starts before the agent and stops after it. This is currently enforced by checking the absence/existence of the unix socket created by the agent. With the changes of this PR, in the case of choosing a tcp socket for the connection, the unix socket is not created. Hence, the infra should be adapted accordingly to guarantee the desired worker/agent life cycle. One option is to transform the worker container in Kubernetes POD into a sidecar container, but this will require the direct use of Helm charts since the Kubernetes provider we use for terraform does not support this yet.

Additional Information

Not applicable

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have thoroughly tested my modifications and added tests when necessary.
  • Tests pass locally and in the CI.
  • I have assessed the performance impact of my modifications.

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2025

CLA assistant check
All committers have signed the CLA.

terraform/variables.tf Outdated Show resolved Hide resolved
@jfonseca-aneo jfonseca-aneo marked this pull request as ready for review January 15, 2025 16:12
- Mount a separate volume for the shared data
- Do not mount the volume destinated for the unix socket if tcp is
  chosen as socket type for the communication agent/worker
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.

4 participants