test: add benchmarking for test, scan, cleanup, and convert to sig steps #5015
Conversation
zachary-bailey
requested review from
juan-lee,
cameronmeissner,
UtheMan,
ganeshkumarashok,
anujmaheshwari1,
AlisonB319,
Devinwong,
lilypan26,
ShiqianTao,
AbelHu,
junjiezhang1997,
jason1028kr and
djsly
as code owners
zachary-bailey
force-pushed
the
zb/benchmarkTestAndScan
branch
from
c571f6b
to
a9a0254
Compare
Pull Request Test Coverage Report for Build 11279754971Details
💛 - Coveralls |
| declare -a benchmarks_order=() | ||
| VHD_BUILD_PERF_DATA=${BUILD_PERF_DATA_FILE} | ||
|
|
||
| check_array_size() { |
There was a problem hiding this comment.
we already define these here: https://github.com/Azure/AgentBaker/blob/master/parts/linux/cloud-init/artifacts/cse_helpers.sh#L500-L548
could we upload this file to the packer VM and source them within install-deps as well? that way we won't have to worry about updating code in multiple places
There was a problem hiding this comment.
Do you mean remove these functions from cse_helpers.sh and source them to the install scripts from this file only? I like that idea and was initially going in that direction.
The only reason I ended up not wanting to remove them from cse_helpers.sh was because I thought they could be useful for node bootstrapping benchmarking. But looking around yesterday, it seems we already have some infrastructure for logging during the bootstrapping process.
Unfortunately, sourcing cse_helpers.sh to these files (convert, test, scan, etc) fails the step because of the variable assignments, so its a bit of a dilemma. How would you like me to proceed? @cameronmeissner
There was a problem hiding this comment.
should we decouple the variable assignment and the helper functions into two files ? this way one can source the helper functions only ?
There was a problem hiding this comment.
I agree though this is some pretty bad code duplication :(
There was a problem hiding this comment.
Is it possible to just source the functions into cse_helpers from this new file...? That way they are available to scripts that run after the packer vm is deprovisioned, still available in cse_helpers for node bootstrap benchmarking later on, and not duplicated.
vhdbuilder/packer/convert-sig-to-classic-storage-account-blob.sh
Outdated
Show resolved
Hide resolved
zachary-bailey
force-pushed
the
zb/benchmarkTestAndScan
branch
from
d951984
to
2f89659
Compare
zachary-bailey
force-pushed
the
zb/benchmarkTestAndScan
branch
from
2f89659
to
70ae732
Compare
zachary-bailey
force-pushed
the
zb/benchmarkTestAndScan
branch
from
3253fec
to
5778ac1
Compare
| @@ -482,71 +482,6 @@ isMarinerOrAzureLinux() { | |||
| return 1 | |||
| } | |||
|
|
|||
| installJq() { | |||
There was a problem hiding this comment.
isnt this used anywhere else ?
There was a problem hiding this comment.
Note for future: installJq function used to be here but was moved to cse_helpers.sh, evalPackageDownloadURL moved up after removal
What type of PR is this?
/kind test
What this PR does / why we need it:
This PR adds benchmarking to additional scripts in preparation for merging the regression detection program.
Which issue(s) this PR fixes:
Unsatisfactory VHD Build pipeline duration.
Requirements:
Notes
Another CSE file was created in order to prevent having to source cse_helpers.sh into the test/scan/cleanup steps as it would cause them to fail.