Incorporated old dev-branch changes #27

cvetkovic · 2022-07-21T16:43:14Z

The warm-up process has changed and been improved according to our discussions, as well as the trace mode.

Implementation Notes ⚒️

The warm-up is now consists of only two steps, 5min profiling without triggering any invocations, and 10min warm-up invocations.
In trace mode, we generate specific definitions with scheduling resource requests (cpu, mem) for each function, i.e., different functions are truly different and no-longer only differ in service names.
However, in RPS sweeps, the homogeneous functions are still of best-efforts to keep things consistent.
The normalisation of warm-up scale has changed from previously based on total cluster instance limits to cpu limits. Specifically, I wrote an allocation policy MaxMaxAlloc() that's the opposite of max-min fairness.
The cluster endpoints of both the control and data plane have been remapped to 10.X.Y.Z so that no one needs to go to the cloudlab jail.
The throughput of RPS mode got increased as well.
Detailed setup guide is written down in readme.
Unit tests are kept up with the development.

External Dependencies 🍀

We are using the memory-cpu conversion from Google cloud function here for populating the function template on-the-fly.
The upper memory limit is from AWS Lambda here.

Breaking API Changes ⚠️

pkg\atom.go

Generating function runtime specs has changed.
Some constant thresholds have changed.

Resolves #20, #21

pkg/trace/profile.go

README.md

cmd/load.go

cmd/options/warmup.go

pkg/generate/atom.go

test.yaml

workloads/container/trace_func_go.yaml

scripts/util/scale_pod.sh

cmd/options/warmup.go

cvetkovic · 2022-07-21T21:43:27Z

Can we test if the branch is working before merging to make sure we haven't omitted anything in this whole process?

Also, the CI fails because data/traces/test is missing.

ustiugov

The main readme contains a lot of useful information, thanks a lot!

I suggest shortening the main readme, moving the auxiliary script description to a separate doc in the docs/ folder (e.g., moving the health checking scripts there) but leaving a reference to that doc in the main readme.

More feedback will follow today.

README.md

ustiugov

I haven't finished my pass but I am going to continue working on it in next few days (probably Sunday). Please go ahead and apply comments from me and Lazar.

cmd/load.go

cmd/options/warmup.go

pkg/trace/profile.go

pkg/util.go

ustiugov · 2022-07-22T16:12:33Z

scripts/setup/expose_infra_metrics.sh

@@ -59,5 +59,6 @@ server_exec() {
 	echo 'Done setting up monitoring components'

 	server_exec 'cd loader; bash scripts/setup/patch_init_scale.sh'
+	server_exec 'cd loader/data; wget -q -O traces.zip "https://www.dropbox.com/scl/fo/x4ct0kzzjnn6we7h31md4/h?dl=1&rlkey=llxj5f5i3uz9rvd9h3m573ueg" && unzip traces.zip -d traces'


we may want to have a tiny trace for running tests in CI. it should also be in Git LFS but separately from the main archive.

scripts/util/scale_pod.sh

workloads/container/trace_func_go.yaml

HongyuHe · 2022-07-23T03:59:40Z

Thank you all!

ustiugov

When addressing comments for a PR with so many changes, please follow the following protocol, which would allow us to inspect every change carefully. For each comment each reviewer makes, (1) push the changes if needed, (2) respond to the comment so the reviewer can take a look and see if the reviewer agrees with the change, indicating that by resolving the comment. Since we are doing that with many comments in batches, this protocol should not cause much delay in finalizing the code.

pkg/function/deploy.go

ustiugov

ok, I finished my pass. Please address all the comments now, starting with the parser, then load generator, results parser, etc. Please stage it (i.e., one-two logical components at a time) so that we can parallelize implementation and code review efforts effectively.

I don't think I was able to scrutinize everything so we'll need to iterate after this PR is merged.

pkg/function/deploy.go

ustiugov · 2022-07-24T14:23:38Z

pkg/function/deploy.sh

@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+CONFIG_FILE=$1


as mentioned in deploy.go. this functionality should be written in Golang.

pkg/generate/atom.go

pkg/generate/trace_load.go

pkg/generate/atom.go

HongyuHe · 2022-07-24T23:04:30Z

Thank you all. All suggestions applied, except for those I don't know how to.
Feel free to change anything

README.md

pkg/generate/atom.go

Signed-off-by: Lazar Cvetković <[email protected]>

Signed-off-by: Hongyu <[email protected]>

Signed-off-by: Lazar Cvetković <[email protected]>

ustiugov

the commit LGTM

Signed-off-by: Lazar Cvetković <[email protected]>

Signed-off-by: Hongyu <[email protected]>

HongyuHe · 2022-08-14T12:05:10Z

Thanks everyone!

I've applied the suggestions except for those asking for more detailed documentation. I will add them gradually.

The PR is good to me as well!

ustiugov

This PR cannot be merged before the remaining comments are addressed. Most of them are really easy to address (just add appropriate comments).

Duration and memory characteristics choice logic has a bug. These issues have the highest priority to fix.

Also, if you need to argue with the review, please discuss potential concerns constructively.

cmd/load.go

cmd/options/warmup.go

pkg/function/deploy.go

pkg/generate/atom.go

pkg/generate/trace_load.go

pkg/test/warmup_test.go

pkg/trace/model.go

pkg/trace/parse.go

ustiugov · 2022-08-14T13:53:13Z

as far as I understand, this PR does not change the integration tests you wrote, it just runs them in the CI as opposed to locally. Please investigate why the tests break and fix them. If you have any questions on the github actions side, please let me know and I'd try to explain.

Signed-off-by: Hongyu <[email protected]>

Signed-off-by: Dohyun Park <[email protected]>

Signed-off-by: Hongyu <[email protected]>

ustiugov

Excellent job! Thank you for improving the comments and readability and for adding a test for the parser.

I left quite minor comments but they should not take more than 30min to fix altogether.

README.md

pkg/generate/trace_load.go

pkg/trace/parse.go

scripts/setup/create_singlenode_container.sh

Signed-off-by: Hongyu <[email protected]>

cvetkovic requested a review from HongyuHe July 21, 2022 16:43

cvetkovic assigned HongyuHe Jul 21, 2022

cvetkovic requested review from ustiugov and removed request for HongyuHe July 21, 2022 16:43

cvetkovic commented Jul 21, 2022

View reviewed changes

cvetkovic linked an issue Jul 21, 2022 that may be closed by this pull request

Trace format mismatch #16

Closed

ustiugov requested changes Jul 22, 2022

View reviewed changes

HongyuHe force-pushed the resolve_dev branch from 524aea1 to 75f6ce6 Compare July 22, 2022 13:03

ustiugov reviewed Jul 22, 2022

View reviewed changes

ustiugov reviewed Jul 24, 2022

View reviewed changes

pkg/function/deploy.go Outdated Show resolved Hide resolved

pkg/function/deploy.go Outdated Show resolved Hide resolved

ustiugov requested changes Jul 24, 2022

View reviewed changes

ustiugov reviewed Jul 25, 2022

View reviewed changes

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

sk1tter reviewed Jul 28, 2022

View reviewed changes

pkg/generate/atom.go Outdated Show resolved Hide resolved

pkg/generate/atom.go Outdated Show resolved Hide resolved

cvetkovic and others added 3 commits July 29, 2022 15:55

Trace folder moved to Git LFS

c6f0437

Signed-off-by: Lazar Cvetković <[email protected]>

Resolve branch conflicts

0f725df

Signed-off-by: Hongyu <[email protected]>

Apply suggestions

c5f1d93

Signed-off-by: Hongyu <[email protected]>

ustiugov force-pushed the resolve_dev branch from 532802e to c5f1d93 Compare July 29, 2022 13:55

cvetkovic added 3 commits July 29, 2022 16:32

Fixed runtime and memory specification generator

396e4cf

Signed-off-by: Lazar Cvetković <[email protected]>

Token permissions in README

daa7a0a

Signed-off-by: Lazar Cvetković <[email protected]>

IAT distribution choice - not hardcoded equidistant anymore

23bcfda

Signed-off-by: Lazar Cvetković <[email protected]>

ustiugov reviewed Aug 2, 2022

View reviewed changes

cvetkovic added 6 commits August 3, 2022 16:16

traceload.go - IAT tick overflow bugfix

2d3979c

Signed-off-by: Lazar Cvetković <[email protected]>

Force control plane placement on master

532beea

Signed-off-by: Lazar Cvetković <[email protected]>

Trace plotter deployment fix

e9030f5

Signed-off-by: Lazar Cvetković <[email protected]>

Taint/untaint stdin bugfix

7539d24

Signed-off-by: Lazar Cvetković <[email protected]>

Put taint on master node on setup completion

3a747f9

Signed-off-by: Lazar Cvetković <[email protected]>

Disable trace plotter setup on startup

a366316

Signed-off-by: Lazar Cvetković <[email protected]>

cvetkovic and others added 8 commits August 10, 2022 18:12

Propagate withTracing to invoker

5f4ca1f

Signed-off-by: Lazar Cvetković <[email protected]>

Updated yaml file

8ecaa21

Signed-off-by: Lazar Cvetković <[email protected]>

Fixed YAML

0821edf

Signed-off-by: Lazar Cvetković <[email protected]>

Updated README

c738794

Signed-off-by: Lazar Cvetković <[email protected]>

Addressed Dmitrii's comments

da1bbd0

Signed-off-by: Lazar Cvetković <[email protected]>

Fixed indentation

68b3071

Signed-off-by: Lazar Cvetković <[email protected]>

Renaming script

e43bd27

Signed-off-by: Lazar Cvetković <[email protected]>

Add more descriptions and cover equality specs (again)

872c797

Signed-off-by: Hongyu <[email protected]>

HongyuHe force-pushed the resolve_dev branch from a61b1a5 to 872c797 Compare August 14, 2022 12:04

ustiugov requested changes Aug 14, 2022

View reviewed changes

HongyuHe and others added 4 commits August 14, 2022 22:32

Apply the 2nd round suggestions

d852313

Signed-off-by: Hongyu <[email protected]>

add data race test for GenerateExecutionSpecs

d5be7b9

Signed-off-by: Dohyun Park <[email protected]>

add mutex to protect against concurrent access to rand.Rand

427549f

Signed-off-by: Dohyun Park <[email protected]>

Fix CI tests

ce16e13

Signed-off-by: Hongyu <[email protected]>

HongyuHe force-pushed the resolve_dev branch from 545e6f4 to ce16e13 Compare August 16, 2022 11:22

vhive-serverless deleted a comment from HongyuHe Aug 17, 2022

ustiugov requested changes Aug 17, 2022

View reviewed changes

ustiugov added the loader-1.0 label Aug 17, 2022

HongyuHe force-pushed the resolve_dev branch 2 times, most recently from d065c94 to 6ce03a7 Compare August 17, 2022 17:50

Apply 3rd-round suggestions

41d32c0

Signed-off-by: Hongyu <[email protected]>

HongyuHe force-pushed the resolve_dev branch from 6ce03a7 to 41d32c0 Compare August 17, 2022 17:57

ustiugov approved these changes Aug 17, 2022

View reviewed changes

ustiugov merged commit f941ab1 into main Aug 17, 2022

ustiugov deleted the resolve_dev branch August 17, 2022 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporated old dev-branch changes #27

Incorporated old dev-branch changes #27

cvetkovic commented Jul 21, 2022 •

edited by ustiugov

Loading

cvetkovic commented Jul 21, 2022 •

edited

Loading

ustiugov left a comment

ustiugov left a comment

ustiugov Jul 22, 2022

HongyuHe commented Jul 23, 2022

ustiugov left a comment

ustiugov left a comment

ustiugov Jul 24, 2022

HongyuHe commented Jul 24, 2022

ustiugov left a comment

HongyuHe commented Aug 14, 2022

ustiugov left a comment

ustiugov commented Aug 14, 2022 •

edited

Loading

ustiugov left a comment

Incorporated old dev-branch changes #27

Incorporated old dev-branch changes #27

Conversation

cvetkovic commented Jul 21, 2022 • edited by ustiugov Loading

Implementation Notes ⚒️

External Dependencies 🍀

Breaking API Changes ⚠️

cvetkovic commented Jul 21, 2022 • edited Loading

ustiugov left a comment

Choose a reason for hiding this comment

ustiugov left a comment

Choose a reason for hiding this comment

ustiugov Jul 22, 2022

Choose a reason for hiding this comment

HongyuHe commented Jul 23, 2022

ustiugov left a comment

Choose a reason for hiding this comment

ustiugov left a comment

Choose a reason for hiding this comment

ustiugov Jul 24, 2022

Choose a reason for hiding this comment

HongyuHe commented Jul 24, 2022

ustiugov left a comment

Choose a reason for hiding this comment

HongyuHe commented Aug 14, 2022

ustiugov left a comment

Choose a reason for hiding this comment

ustiugov commented Aug 14, 2022 • edited Loading

ustiugov left a comment

Choose a reason for hiding this comment

cvetkovic commented Jul 21, 2022 •

edited by ustiugov

Loading

cvetkovic commented Jul 21, 2022 •

edited

Loading

ustiugov commented Aug 14, 2022 •

edited

Loading