-
Notifications
You must be signed in to change notification settings - Fork 207
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
feat: add oras binary to cache #4691
Conversation
I will add the make generate test files after review so that it is easier to review |
@@ -262,6 +263,13 @@ for p in ${packages[*]}; do | |||
capture_benchmark "download_${name}" | |||
done | |||
|
|||
downloadContainerdWasmShims |
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.
what is this change ? missing a rebase ?
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, in the future WasmShims is one of the packages that will be downloaded via oras, so the install needs to happen after oras is already installed. Moved so that I could verify that this change doesn't hurt anything
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.
^ my line of thinking here is wrong. Oras will get installed during build so when it is actually needed during provisioning, it is already installed. So the order does not actually matter - since it will be installed and ready to use during provisioning
@@ -118,6 +118,22 @@ installCredentalProvider() { | |||
rm -rf ${CREDENTIAL_PROVIDER_DOWNLOAD_DIR} | |||
} | |||
|
|||
# TODO (alburgess) have oras version managed by dependant or Renovate | |||
installOras() { |
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.
should we also add call to this somewhere in cse_main.sh to ensure it gets installed at runtime if the underlying VHD doesn't already have it cached?
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 only case where we need oras installed is for a network isolated cluster. So, if it is not cached, it won't be able to be installed on those clusters - since it is needed to install everything else.
As a general question for my understanding - @Devinwong added the loop inside install_dependencies.sh for installing the packages in components.json. Are any of these packages installed via cse?
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.
Some do. For example, in the for-loop for containerd
case, it calls installStandaloneContainerd
. And in cse_main.sh around ln124, it also calls installContainerRuntime
which will eventually calls installStandaloneContainerd
.
But some don't. Actually recently Paul removed the codes to install cni from cse. So now it will only use what cached in the VHD.
For your case, if oras is not cached as expected, when it comes to network isolated cluster bootstrapping the node, it still won't be able to install oras, right?
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.
Right I don't think a NIC will have the ability to install oras
9866e1d
to
5f13fe2
Compare
Cache Oras on the VHD
What type of PR is this?
What this PR does / why we need it: ORAS will be needed for installing packages for network isolated clusters
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note:
Oras is included in the download section of release-notes