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

Feature/grpc support #40

Merged

Conversation

salehsedghpour
Copy link
Collaborator

@salehsedghpour salehsedghpour commented Mar 28, 2022

This PR closes #2 .

The procedure for grpc is:

  1. Using generator we create a service.proto based on the input json and add it as a configmap value to the service. The reason for not creating a new configmap is due to the restricted permission of configmaps. We also add the related healthcheck based on protocol.
  2. When we deploy the service, the bash script checks weather it is grpc or http. If it is grpc:
    2.1. it compiles the service.proto after copying the values to the new file with right permissions.
    2.2. it generates the main application of grpc with filling a template.
    2.3. it runs the main application of grpc.

We decided to move all tasks to common folder. The client of grpc is in that folder.

The output of calling the grpc service is:

docker run fullstorydev/grpcurl -plaintext -d '{"data":"test_data"}' 10.96.219.36:80 service_1.end_1
{
  "data": "Hi, end_1 test_data!"
}

@salehsedghpour salehsedghpour requested a review from alekodu March 28, 2022 17:45
"port": "80",
"endpoint": "/end-2",
"endpoint": "end2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any restriction with special symbols? I guess that's why you had to change their format...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for Now I'd suggest to don't use any specific symbols like dashes or underline. We can add the support of these symbols in #25.

Path string `yaml:"path,omitempty"`
Port int `yaml:"port,omitempty"`
} `yaml:"httpGet,omitempty"`
Exec struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this so you can probe readiness in grpc with your own implemented mechanism?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -73,14 +82,17 @@ func CreateDeployment(metadataName, selectorAppName, selectorClusterName string,
func CreateDeploymentWithAffinity(metadataName, selectorAppName, selectorClusterName string, numberOfReplicas int,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions CreateDeployment() and CreateDeploymentWithAffinity() share several lines of similar code... we may consider here how to avoid duplicated code.

Copy link
Collaborator Author

@salehsedghpour salehsedghpour Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed by #41.

RUN apt update
RUN apt install -y jq \
wget \
2to3
Copy link
Member

@alekodu alekodu Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a package for translation from python2 to python3?

Copy link
Collaborator Author

@salehsedghpour salehsedghpour Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In run.sh we uninstall this package. But in general, when we compile the service.proto file with protoc. those generated files have issues with importing each other (it wasn't fixed by adding init.py). The only solution to fix it was to change the import section to something like:

from . import service_pb2

So we need to add from . to imports. The easiest way that I found was using this package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in the future we can fully update the code to python 3...

model/Dockerfile Outdated

WORKDIR /usr/src/app
RUN ls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I'll delete it.

@salehsedghpour salehsedghpour merged commit f359271 into EricssonResearch:main Mar 30, 2022
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.

Support different protocols
2 participants