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

Docker upload benchmark #19

Merged
merged 8 commits into from
Aug 4, 2020
Merged

Conversation

Sammers21
Copy link
Contributor

The script measures single docker upload performance of Artipie and docker registry, the script writes results into docker.json file which can be used for visualisation on Github Pages.

Related to #18

@Sammers21
Copy link
Contributor Author

@g4s8, can you review the Pull Request?

Copy link
Member

@g4s8 g4s8 left a comment

Choose a reason for hiding this comment

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

@Sammers21 please keep synthetic benchmarks and client based benchmarks in separate directories. Also, please add a script to run these benchmark on existing AWS infrastructure and publish reports to HTML

@Sammers21
Copy link
Contributor Author

@Sammers21 please keep synthetic benchmarks and client based benchmarks in separate directories

@g4s8, this Pull Request does not contain any synthetic benchmarks. It is a topic for future work.

@Sammers21
Copy link
Contributor Author

Sammers21 commented Jul 28, 2020

Also, please add a script to run these benchmark on existing AWS infrastructure

@g4s8, Can I please do it in a separated request?

@Sammers21
Copy link
Contributor Author

Sammers21 commented Jul 28, 2020

publish reports to HTML

@g4s8, please pay attention that reports are going to be published in JSON in order to be visualised with Chats.js.

@Sammers21 Sammers21 requested a review from g4s8 July 28, 2020 09:46
@Sammers21
Copy link
Contributor Author

Sammers21 commented Aug 3, 2020

@g4s8, review the pull request, please.

Copy link
Member

@g4s8 g4s8 left a comment

Choose a reason for hiding this comment

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

@Sammers21 please check my comments

for IP in $PUBLIC_SERVER_IP_ADDR $PUBLIC_CLIENT_IP_ADDR
do
ssh -i aws_ssh_key -oStrictHostKeyChecking=no ubuntu@$IP <<'ENDSSH'
set -x
Copy link
Member

Choose a reason for hiding this comment

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

@Sammers21 don't we need set -e here too?

docker/upload.py Outdated
Comment on lines 31 to 33
f = open("./artipie.yaml", "w+")
f.write(artipie_yml)
f.close()
Copy link
Member

Choose a reason for hiding this comment

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

@Sammers21 it could be wrapped in with construct to close resource properly (similar to try-with-resources in Java):

with open('./artipie.yaml', 'w+') as f:
  f.write(artipie_yml)

docker/upload.py Outdated
Comment on lines 34 to 36
f = open("./configs/my-docker.yaml", "w+")
f.write(my_docker)
f.close()
Copy link
Member

Choose a reason for hiding this comment

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

@Sammers21 same here (with)

Comment on lines +13 to +29
artipie_yml = """
meta:
storage:
type: fs
path: /var/artipie/configs
layout: flat
"""
my_docker = """
repo:
type: docker
storage:
type: fs
path: /var/artipie/data
permissions:
"*":
- "*"
"""
Copy link
Member

Choose a reason for hiding this comment

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

@Sammers21 these variables seems to be constants, and could be moved out of function's scope

Copy link
Contributor Author

@Sammers21 Sammers21 Aug 4, 2020

Choose a reason for hiding this comment

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

@g4s8, why? The only place they seem to be relevantly located is the function body where they are located now.

docker/upload.py Show resolved Hide resolved
docker/upload.py Show resolved Hide resolved
docker/upload.py Outdated Show resolved Hide resolved
@Sammers21 Sammers21 requested a review from g4s8 August 4, 2020 06:35
Copy link
Member

@g4s8 g4s8 left a comment

Choose a reason for hiding this comment

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

@Sammers21 thanks, good too merge

@g4s8 g4s8 merged commit ad4728c into artipie:master Aug 4, 2020
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.

2 participants