-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 manage offline files script #8956
add manage offline files script #8956
Conversation
Hi @ErikJiang. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
/ok-to-test
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.
Nice effort, just one comment.
/cc @oomichi
# step 4 : run nginx container server | ||
sudo docker container inspect nginx >/dev/null 2>&1 | ||
if [ $? -ne 0 ]; then | ||
sudo docker run \ |
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.
On Kubernetes ecosystem, we are switching from docker
to nerdctl
to drop docker
support.
This can be out of scope from this pull request because the existing scripts are still using docker
command.
just wanted to confirm what should be our direction.
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.
@oomichi Ok, I see, I can create a separate PR later if we need to switch, since (AFAIK) manage-offline-container-images.sh
also uses docker and I can modify it together. 🤔
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.
Ideally we should not depend on a container manager for image manipulation so while containerd is moving toward nerdctl I would personally advise looking into skopeo for image management and for kubespray as a project to adopt skopeo for our container manapulation logic instead of the per-container-manager approach we have today.
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 makes sense, skopeo looks like a good option, I'm interested in it and plan to look into 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.
Nice information about skopeo.
I checked it a little bit, that seems only for managing container images.
We cannot replace docker run
/nerdctl run
here with skopeo, 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.
we can use code like "https://github.com/automotiveMastermind/numonic/blob/main/hack/build.sh#L91" to support podman , nerdctl and even docker together.
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.
@yankay 🤝 this is a great way, I have updated the code.
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.
PTAL @floryut @oomichi @cristicalin 🤠
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.
Wow this became a long thread. The reason we have the docker run
and nerdctl run
command in there to fetch files is because we don't have an elegant way to get files out of container images so what we do is $command run <image> -v /tmp:/tmp cp <whatever> /tmp/<whatever>
and get the file from the container, it is rather inelegant. My understanding was that nerdctl recently (0.19.0 I think) got the capability to copy files directly from images without the need to run them, maybe skopeo has the same since it is an image manipulation tool.
Overall our current download, cache and offline logic could use a full overhaul with a clean multi-step separation and runtime agnostic code. By this I mean always check the cache for needed items, if items are not in the cache fetch them in a runtime agnostic way and provide a way to populate the cache so that no external connection is needed if the cache is already complete.
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.
@oomichi 😄 are there any additional suggestions?
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
Looks ok to me now. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristicalin, ErikJiang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Thank you for doing this.
/lgtm
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
Signed-off-by: bo.jiang <bo.jiang@daocloud.io>
Signed-off-by: bo.jiang bo.jiang@daocloud.io
What type of PR is this?
/kind feature
What this PR does / why we need it:
this script will download all offline files according to
temp/files.list
and run the nginx container for offline file download.this is helpful for offline deployment.
Which issue(s) this PR fixes:
Fixes #8954
Special notes for your reviewer:
Does this PR introduce a user-facing change?: