perf: Parallelize downloadContainerdWasmShims function / fix download error bug #4126

zachary-bailey · 2024-02-29T19:23:18Z

What type of PR is this?

/kind performance

What this PR does / why we need it:

This PR changes the execution of downloadContainerdWasmShims to make use of parallel processing. It increases the speed of the function by 5 minutes on average.

Which issue(s) this PR fixes:

This is one of a variety of incremental performance improvements that will ultimately increase the speed of VHD Build times.

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

coveralls · 2024-02-29T19:25:31Z

Pull Request Test Coverage Report for Build 8653558851

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 79.266%

Totals
Change from base Build 8652187564:	0.0%
Covered Lines:	2355
Relevant Lines:	2971

💛 - Coveralls

lilypan26 · 2024-02-29T23:53:15Z

parts/linux/cloud-init/artifacts/cse_install.sh

        fi
    done
+    wait ${pids[@]}
+    for shim_version in $CONTAINERD_WASM_VERSIONS; do
+        chmod 755 "$containerd_wasm_filepath/containerd-shim-wws-${binary_version}-v1"


out of curiosity, why can't this part be parallelized too? also i think you want to redeclare binary_version in the for loop here?

@lilypan26 Hey! Thank you for the tip on binary_version. I initially took it out because I read the variable would still be in scope, but didn't take into account that its value needs to change as the for loop iterates so it can set permissions on each version.

For the permissions part, the first two downloads (spin / slight) are running sequentially and complete in about a second, so they will definitely be downloaded by the time the program reaches the code that sets permissions for their directories. For the last permission, I had to ensure that the 3 background processes (downloading shim_wws x 3) were complete before reaching that code, otherwise it could attempt to set permissions on a directory that doesn't exist on the VHD yet. I currently only have wws parallelized because downloading the first two and setting their permissions only takes about a second for each binary version.

If you think it makes more sense to parallelize everything for speed or stylistic reasons, please let me know! And thanks again.

ah i see, then yea for safety we should only access the directory after we are sure it is created.

Although for spin and slight, even if they are much faster than wws i think it wouldn't hurt to parallelize those are well and then move all of the chmods to the for loop afterwards. This might be more consistent and readable, but up to you. If not, i would at least add a comment where you are downloading wws and explain that you are parallelizing wws download and only changing the permissions after you are sure the file is there.

@lilypan26 I implemented your suggestions, thanks!

That being said, I noticed a flakey little error that has been happening for some time now (before my changes) regarding setting permissions. Im looking into it then will submit another PR.

ganeshkumarashok · 2024-03-11T18:56:09Z

parts/linux/cloud-init/artifacts/cse_install.sh

        if [[ $(isARM64) == 1 ]]; then
            containerd_wasm_url="https://acs-mirror.azureedge.net/containerd-wasm-shims/${shim_version}/linux/arm64"
        fi

        if [ ! -f "$containerd_wasm_filepath/containerd-shim-spin-${shim_version}" ] || [ ! -f "$containerd_wasm_filepath/containerd-shim-slight-${shim_version}" ]; then
            retrycmd_if_failure 30 5 60 curl -fSLv -o "$containerd_wasm_filepath/containerd-shim-spin-${binary_version}-v1" "$containerd_wasm_url/containerd-shim-spin-v1" 2>&1 | tee $CURL_OUTPUT >/dev/null | grep -E "^(curl:.*)|([eE]rr.*)$" && (cat $CURL_OUTPUT && exit $ERR_KRUSTLET_DOWNLOAD_TIMEOUT)
            retrycmd_if_failure 30 5 60 curl -fSLv -o "$containerd_wasm_filepath/containerd-shim-slight-${binary_version}-v1" "$containerd_wasm_url/containerd-shim-slight-v1" 2>&1 | tee $CURL_OUTPUT >/dev/null | grep -E "^(curl:.*)|([eE]rr.*)$" && (cat $CURL_OUTPUT && exit $ERR_KRUSTLET_DOWNLOAD_TIMEOUT)
-            retrycmd_if_failure 30 5 60 curl -fSLv -o "$containerd_wasm_filepath/containerd-shim-wws-${binary_version}-v1" "$containerd_wasm_url/containerd-shim-wws-v1" 2>&1 | tee $CURL_OUTPUT >/dev/null | grep -E "^(curl:.*)|([eE]rr.*)$" && (cat $CURL_OUTPUT && exit $ERR_KRUSTLET_DOWNLOAD_TIMEOUT)
+            if [ "$shim_version" = "v0.8.0" ]; then
+                # Only download the wws shim for v0.8.0, wws shim for v0.3.0 and v0.5.1 fail


Can we remove those two versions in a separate PR? And share it with @Mossaka for a review?

Separate PR for parallelization vs removal of component downloads that 404.
The latter might need a cleanup of the containerd config too.

So it seems that in baker.go, it actually accounts for the fact that wws is not available for v0.3.0 and v0.5.1. It makes no mention of wws until v0.8.0.

Relevant code in lines 1219 - 1237 & 1331 - 1349
https://github.com/Azure/AgentBaker/blob/master/pkg/agent/baker.go

Could this mean that the bug is confined to the downloadContainerdWasmShims function in cse_install.sh, since it doesn't account for wws not being available on v0.3.0 and v0.5.1 in the for loop, but baker.go seems to for the config?

If so, I can change the for loop to treat v0.8.0 as a special case and download wws for it. Or I can delete the other two versions entirely as originally requested. The parallelization PR is in the VHD Build Test Pipeline. @ganeshkumarashok

Deleting the versions that fail during the download would be cleaner as those versions will not be used

zachary-bailey · 2024-03-13T17:16:29Z

@Mossaka Good morning,

I am the engineer working with @ganeshkumarashok to decrease VHD build times in our pipelines.

Can you review this PR and let me know what you think? I removed v0-3-0 and v0-5-1 in baker.go, baker_test.go, and the cse_install.sh file. This is meant to be a fix until the appropriate versions can be put in. Additionally, I am running the downloadContainerdWasmShims function in parallel now in order to increase speed. It passed in the VHD Build Test pipeline with no errors.

The way the function and configs are currently written, the function fails for 5 minutes while it attempts to download wws-shims for v0-3-0 and v0-5-1. Please let me know if you have an input or suggestions! Thanks.

Mossaka

Added a few comments.

Mossaka · 2024-03-13T19:04:54Z

parts/linux/cloud-init/artifacts/cse_install.sh

@@ -16,7 +16,7 @@ SECURE_TLS_BOOTSTRAP_KUBELET_EXEC_PLUGIN_DOWNLOAD_DIR="/opt/azure/tlsbootstrap"
 SECURE_TLS_BOOTSTRAP_KUBELET_EXEC_PLUGIN_VERSION="v0.1.0-alpha.2"
 TELEPORTD_PLUGIN_DOWNLOAD_DIR="/opt/teleportd/downloads"
 TELEPORTD_PLUGIN_BIN_DIR="/usr/local/bin"
-CONTAINERD_WASM_VERSIONS="v0.3.0 v0.5.1 v0.8.0"
+CONTAINERD_WASM_VERSIONS="v0.8.0"


Could we have a follow-up PR to upgrade the shim to the latest version?

Do you want v0-8-0 switched to v0-11-1? Or do you want us to bump all versions up like below:

v0-3-0 ==> vX-X-X
v0-5-1 ==> vX-X-X
v0-8-0 ==> vX-X-X

If so, which versions would map to the older ones?

We are just looking to get the failing curl command to wws for v0-3-0 and v0-5-1 out of the code, as its adding 5 minutes to our build time due to retries.

Thank you for the review

Mossaka · 2024-03-13T19:06:01Z

parts/linux/cloud-init/artifacts/cse_install.sh

@@ -16,7 +16,7 @@ SECURE_TLS_BOOTSTRAP_KUBELET_EXEC_PLUGIN_DOWNLOAD_DIR="/opt/azure/tlsbootstrap"
 SECURE_TLS_BOOTSTRAP_KUBELET_EXEC_PLUGIN_VERSION="v0.1.0-alpha.2"
 TELEPORTD_PLUGIN_DOWNLOAD_DIR="/opt/teleportd/downloads"
 TELEPORTD_PLUGIN_BIN_DIR="/usr/local/bin"
-CONTAINERD_WASM_VERSIONS="v0.3.0 v0.5.1 v0.8.0"


Do we have any users on the older versions of the shim, say 0.3.0? By removing the older versions, are we going to break their existing workflows? If so, should we be more cautious about deprecating older versions and probably notife customers beforehand?

I am fine with deprecation in general but I prefer putting a notification to customers first.

These older versions are getting a 404 on downloads during the VHD builds. Customers won't be affected/already must be affected in that case right?

Only case I can think of is users migrating from an older version working earlier to a new nodepool/VHD image that has the newer version. What would the expected behavior for that be, before this change (as that path existed, because the older version was not available for a while)?

For the notification to users, adding a note in the AKS release notes should be sufficient right?

The spin and slight are downloading successfully for v0-3-0 and v0-5-1, but not wws, which makes me think that wws isn't available for those.

Is it possible they are still using spin / slight for v0-3-0 and v0-5-1 without wws? @Mossaka

If not, then I don't think any customers would have been able to use those versions for quite some time.

Is it possible they are still using spin / slight for v0-3-0 and v0-5-1 without wws?

Yes, we should remove wws from v0.3.0 and v0.5.0 because it wasn't introduced for these versions

maybe what you need is a condition logic saying that if the version if 0-3-0 or 0-5-0, don't install wws shim

Alrighty thanks!

@Mossaka Good morning! I have implemented your suggestions, adding the v3 and v5 versions back in the baker.go/baker_test.go files and creating the conditional in cse_install. Would you mind giving this PR another look?

ganeshkumarashok · 2024-03-13T19:53:02Z

Also @zachary-bailey, could you change the PR title to mention the deletion of the older version too, if you decide to keep both deletion and parallelization in the same PR?

zachary-bailey self-assigned this Feb 29, 2024

zachary-bailey requested review from juan-lee, cameronmeissner, UtheMan, ganeshkumarashok, anujmaheshwari1, AlisonB319, Devinwong, lilypan26, ShiqianTao, AbelHu and junjiezhang1997 as code owners February 29, 2024 19:23

zachary-bailey temporarily deployed to test February 29, 2024 19:23 — with GitHub Actions Inactive

lilypan26 reviewed Mar 1, 2024

View reviewed changes

zachary-bailey force-pushed the zacharybailey/downloadContainerdWasmShimsOptimization branch from 991b2e8 to f27b93e Compare March 11, 2024 16:02

zachary-bailey temporarily deployed to test March 11, 2024 16:02 — with GitHub Actions Inactive

ganeshkumarashok reviewed Mar 11, 2024

View reviewed changes

zachary-bailey temporarily deployed to test March 11, 2024 20:34 — with GitHub Actions Inactive

zachary-bailey force-pushed the zacharybailey/downloadContainerdWasmShimsOptimization branch from 616c5b5 to 635fd14 Compare March 13, 2024 14:50

zachary-bailey requested a review from jason1028kr as a code owner March 13, 2024 14:50

zachary-bailey temporarily deployed to test March 13, 2024 14:50 — with GitHub Actions Inactive

zachary-bailey temporarily deployed to test March 13, 2024 14:56 — with GitHub Actions Inactive

zachary-bailey temporarily deployed to test March 13, 2024 14:59 — with GitHub Actions Inactive

zachary-bailey temporarily deployed to test March 13, 2024 15:00 — with GitHub Actions Inactive

Mossaka reviewed Mar 13, 2024

View reviewed changes

zachary-bailey changed the title ~~perf: Parallelize downloadContainerdWasmShims function~~ perf: Parallelize downloadContainerdWasmShims function / delete older wasm shim versions Mar 13, 2024

zachary-bailey temporarily deployed to test March 18, 2024 15:32 — with GitHub Actions Inactive

Zachary Bailey added 26 commits April 11, 2024 14:33

fix: downloadContainerdWasmShims bug fix

32bce1b

fix: wasmshims versions

cd0b202

fix: wasmshims versions

c5060b4

fix: fix wasmshims version

f6d923a

chore: add wasmshims versins

cefcd82

fix: indentation

5a79809

fix: indentation

7e3766f

fix: indentation

e486527

fix: indentation

73c9d31

fix indentation

bee4abb

fix: indentation

db8f226

fix: indentation

d4c5d99

fix: indentation

9f5e8bf

fix indentation

737fded

fix: indentation

3c3dd9d

fix: revert to remote master version of pkg/agent/baker_test.go

6065b81

chore: indentation

1cdc755

chore: generate

69cfd59

fix: indentation

fb9b12c

chore: generate

1a3d241

fix: code style for string comparison

c737d7f

chore: generate

575859f

fix: string comparison operator styling

bedd5c9

chore: make generate

866d185

perf: remove parallelization

c1d581b

fix: revert changes

1fe6c04

zachary-bailey force-pushed the zacharybailey/downloadContainerdWasmShimsOptimization branch from fd7eb16 to 1fe6c04 Compare April 11, 2024 21:33

zachary-bailey temporarily deployed to test April 11, 2024 21:33 — with GitHub Actions Inactive

zachary-bailey merged commit 1211092 into master Apr 11, 2024
20 checks passed

zachary-bailey deleted the zacharybailey/downloadContainerdWasmShimsOptimization branch April 11, 2024 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Parallelize downloadContainerdWasmShims function / fix download error bug #4126

perf: Parallelize downloadContainerdWasmShims function / fix download error bug #4126

zachary-bailey commented Feb 29, 2024 •

edited

Loading

coveralls commented Feb 29, 2024 •

edited

Loading

lilypan26 Feb 29, 2024

zachary-bailey Mar 1, 2024

lilypan26 Mar 2, 2024

zachary-bailey Mar 6, 2024

ganeshkumarashok Mar 11, 2024

ganeshkumarashok Mar 11, 2024

zachary-bailey Mar 11, 2024 •

edited

Loading

ganeshkumarashok Mar 12, 2024

zachary-bailey commented Mar 13, 2024

Mossaka left a comment

Mossaka Mar 13, 2024

zachary-bailey Mar 13, 2024

zachary-bailey Mar 13, 2024

Mossaka Mar 13, 2024

ganeshkumarashok Mar 13, 2024

ganeshkumarashok Mar 13, 2024

ganeshkumarashok Mar 13, 2024

zachary-bailey Mar 13, 2024

Mossaka Mar 13, 2024

Mossaka Mar 13, 2024

zachary-bailey Mar 14, 2024

zachary-bailey Mar 19, 2024

ganeshkumarashok commented Mar 13, 2024

perf: Parallelize downloadContainerdWasmShims function / fix download error bug #4126

perf: Parallelize downloadContainerdWasmShims function / fix download error bug #4126

Conversation

zachary-bailey commented Feb 29, 2024 • edited Loading

coveralls commented Feb 29, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8653558851

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachary-bailey Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachary-bailey commented Mar 13, 2024

Mossaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshkumarashok commented Mar 13, 2024

zachary-bailey commented Feb 29, 2024 •

edited

Loading

coveralls commented Feb 29, 2024 •

edited

Loading

zachary-bailey Mar 11, 2024 •

edited

Loading