-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add cloudsuite web serving benchmark #143
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I'm not a fan of dumping large files into the repo, especially binaries. Usually we include a README that explain how to build and run the benchmark, including instructions how to fetch "external" files, for example a deps/
repository (see LevelDB example, that also includes patching the file). Could you follow a similar structure?
benchkit/communication/__init__.py
Outdated
@@ -702,6 +703,10 @@ def remote_host(self) -> str | None: | |||
def is_local(self) -> bool: | |||
return False | |||
|
|||
@property | |||
def get_ipaddress(self) -> str: | |||
return self._ssh_host_info["hostname"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is not necessarily an IP address. Do you need IP addresses or hosts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modify this to use the SSH_CONNECTION environment variable to get the IP address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a README instruction and removed the *.jar file. Is that following the structure you proposed? Could you check that the instructions work on your side as well?
a9a4a5e
to
31624cf
Compare
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Lourenço de Lima Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
41b283b
to
be5acc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is not mergeable in this state. There are hardcoded paths.
|
||
self.platform.comm.shell( | ||
command=f"printf {seeds_str} > _tmp_benchkit_seeds.txt", | ||
current_dir="~/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this path does not work in general, it's a bash idiosyncracy. Also, why do you dump this file in the home directory of the current user? I'd prefer to put it somewhere in /tmp/benchkit/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a /tmp/benchkit_cloudsuite
directory following the example of postgres
current_dir="~/", | ||
) | ||
|
||
# 1) Move fabandriver.jar and Web20Driver.java.in into the right places |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put these comments at the right place where these operations are actually executed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved this and also expanded a bit on each step taken. Could you check if the current version is better?
else: | ||
print("[WARNING!!!] faban_built docker is already built, skipping build_bench") | ||
|
||
# 1) docker build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
record_data_dir: PathType, | ||
) -> None: | ||
filename = "/home/drc/rchehab/faban_output/TH_*-TM_1000-TY_THINKTIME-DS_fixed/*/detail.xan" | ||
filename_dir = "/home/drc/rchehab/faban_output/TH_*-TM_1000-TY_THINKTIME-DS_fixed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardcoded paths here. That won't generalize on other machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also used the tmp/benchkit_cloudsuite/
path here and in a few other places with hardcoded paths
} | ||
|
||
if src_dir is None: | ||
raise ValueError("A src_dir argument must be defined manually.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please specify of which repo. For now the user is left without information of what src it must provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is still unclear IMHO. This is for the faban
repo, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is for the cloudsuite repository. I've just added a link to the github in the error message
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
self._write_to_record_data_dir(file_content, "detail.xan", record_data_dir) | ||
|
||
self.platform.comm.shell( | ||
f"sudo rm -rf {filename_dir}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is too dangerous.
Do you really need this to be removed? And with sudo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of important to remove it.
Essentially, this is an output directory generated by a redirection of a container-internal directory to the native system. I.e., that TH-*-TM-1000-*/ directory
.
The docker command that creates this redirect seems to create it using the root user.
Perhaps there is a way so docker creates this as a normal user. At the moment, I haven't thought of a better way to solve this -- but I've renamed the directory to output_dir
to make it a bit more obvious what is being removed
} | ||
|
||
if src_dir is None: | ||
raise ValueError("A src_dir argument must be defined manually.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is still unclear IMHO. This is for the faban
repo, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problematic sudo rm -rf
Signed-off-by: Rafael Chehab <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost good to go.
Co-authored-by: Antonio Paolillo <[email protected]> Signed-off-by: Rafael Lourenço de Lima Chehab <[email protected]>
Signed-off-by: Rafael Chehab <[email protected]>
Add benchkit support for the
web serving
benchmark of thecloudsuite
benchmark suite, which includes end-to-end Cloud applications.This
web serving
benchmark involves a web server which provides a blog to several users. Users can post blogs, add/remove friends and other operations typical of blogs/social media applications.The backend of this server includes a database application and a memcache application