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

include Spright Parking benchmark (docker-compose local) #880

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jchua99
Copy link

@jchua99 jchua99 commented Mar 8, 2024

the spright parking benchmark requires

  • 5 services which are part of the function chain
  • 1 relay
  • 1 nginx server to act as a reverse proxy

the relevant deployment is contained in dc-parking.yaml

The nginx server is built by a docker image with relevant config file in spright-parking/docker/nginx-local. This is to facilate the allocation of ip address.

The function chain ip is allocated statically instead of dynamically, as config cannot be updated on runtime, compared to the original knative method of deploying with an istio service mesh handling the networking portion.

@jchua99 jchua99 requested a review from lrq619 March 8, 2024 14:08
@lrq619 lrq619 marked this pull request as ready for review May 9, 2024 03:14
Signed-off-by: Jason Chua <chua0990@e.ntu.edu.sg>
Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

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

  • use git lfs for binary files
  • Add copyrights to all source files
  • update the Dependabot config of the repo
  • Remove the compiled server binary from git, add to gitignore
  • Is knative setup tested or not? If not, we cannot merge this code before it is done
  • I do not see any CI tests, cannot merge as is



## Running this benchmark (using knative)
### TODO: Need to test it on knative
Copy link
Member

Choose a reason for hiding this comment

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

Done? Open issue if cannot be done by next week


## Running this benchmark (using knative)
### TODO: Need to test it on knative
Below contents haven't been tested on knative
Copy link
Member

Choose a reason for hiding this comment

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

This is not acceptable for the code to be merged

access_log /var/log/nginx/access.log main;

sendfile on;
#tcp_nopush on;
Copy link
Member

Choose a reason for hiding this comment

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

Clean up commented code here and below

COPY default.conf /etc/nginx/conf.d/default.conf
COPY nginx_setup.sh ./setup/nginx_setup.sh

# COPY sprightparking.crt /etc/ssl/cert/sprightparking.crt
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment or clean up

access_log /var/log/nginx/access.log main;

sendfile on;
#tcp_nopush on;
Copy link
Member

Choose a reason for hiding this comment

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

Clean up

stream.write(input_data)
stream.close()

# ingressIP = subprocess.check_output("docker inspect parking-proxy -f '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' ", shell=True).decode('UTF-8')
Copy link
Member

Choose a reason for hiding this comment

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

Clean up

@@ -0,0 +1,10 @@
grpcio==1.45.0
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to update the Dependabot config of the repo

template:
metadata:
annotations:
# autoscaling.knative.dev/minScale: "1"
Copy link
Member

Choose a reason for hiding this comment

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

Clean up

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.

3 participants