Skip to content
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

download crictl at preload and runtime #18362

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ComradeProgrammer
Copy link
Member

@ComradeProgrammer ComradeProgrammer commented Mar 12, 2024

FIX #18359

Before:

minikube use the crictl installed in the kic image

After:

  1. sudo crictl --version is executed on the host to check whether the version of crictl corresponds to k8s version
  2. when starting the minikubeif preload is false, and the crictl version is not correct, we download the crictl and replace the original crictl in the host. When k8s verison is v1.a.b, then crictl v1.a.0 will be downloaded. When generating the preload, we shall detect the latest matching version of crictl and download it.
  3. if crictl has been downloaded before and found in the cache in ~/.minikube then the cached binary will be used
  4. crictl is also downloaded and transfered in the preload, in the same way as kubectl and kubelet
    e.g. under such circumstance, after starting minikube cluster, use minikube ssh and we can see that

docker@minikube:$ sudo crictl --version
crictl version v1.28.0
docker@minikube:$ sudo crictl  ps
CONTAINER           IMAGE                                                                                                CREATED             STATE               NAME                      ATTEMPT             POD ID              POD
8eae2728f85d3       97e04611ad43405a2e5863ae17c6f1bc9181bdefdaa78627c432ef754a4eb108                                     46 minutes ago      Running             coredns                   0                   0a580df21c36b       coredns-5dd5756b68-bc2dw
dbe2b7b442f48       66749159455b3f08c8318fe0233122f54d0f5889f9c5fdfb73c3fd9d99895b51                                     46 minutes ago      Running             storage-provisioner       0                   5e63873caa6c9       storage-provisioner
53383548192c3       docker.io/kindest/kindnetd@sha256:61f9956af8019caf6dcc4d39b31857b868aaab80521432ddcc216b805c4f7988   46 minutes ago      Running             kindnet-cni               0                   7e29f779adce9       kindnet-fg6j2
905443e23b753       3ca3ca488cf13fde14cfc4b3ffde0c53a8c161b030f4a444a797fba6aef38c39                                     47 minutes ago      Running             kube-proxy                0                   b521393332a9e       kube-proxy-c5wxp
1825671293c0f       9961cbceaf234d59b7dcf8a197a024f3e3ce4b7fe2b67c2378efd3d209ca994b                                     47 minutes ago      Running             kube-controller-manager   0                   aa296e1d2ffe9       kube-controller-manager-minikube
8489b99abcd33       04b4c447bb9d4840af3bf7e836397379d65df87c86e55dcd27f31a8d11df2419                                     47 minutes ago      Running             kube-apiserver            0                   d90bf0f26db9f       kube-apiserver-minikube
50b93cafff067       05c284c929889d88306fdb3dd14ee2d0132543740f9e247685243214fc3d2c54                                     47 minutes ago      Running             kube-scheduler            0                   852b3009621c4       kube-scheduler-minikube
dbc57b61ae284       9cdd6470f48c8b127530b7ce6ea4b3524137984481e48bcde619735890840ace                                     47 minutes ago      Running             etcd                      0                   75c186d1ea62b       etcd-minikube

@spowelljr @medyagh

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @ComradeProgrammer. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 12, 2024
@k8s-ci-robot k8s-ci-robot requested review from medyagh and prezha March 12, 2024 00:48
hack/preload-images/generate.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/bsutil/binaries.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/bsutil/binaries.go Outdated Show resolved Hide resolved
pkg/minikube/download/binary.go Outdated Show resolved Hide resolved
pkg/minikube/download/binary.go Outdated Show resolved Hide resolved
pkg/minikube/download/binary.go Outdated Show resolved Hide resolved
@ComradeProgrammer ComradeProgrammer force-pushed the crictl branch 3 times, most recently from c654883 to 46cd29d Compare March 13, 2024 20:00
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 13, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2024
pkg/minikube/bootstrapper/kubeadm/kubeadm.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/bsutil/binaries.go Outdated Show resolved Hide resolved
@ComradeProgrammer
Copy link
Member Author

revised. now we use ssh to run crictl --version to check the crictl version, and we also use checkcache

pkg/minikube/cruntime/cri.go Outdated Show resolved Hide resolved
Copy link
Member

@spowelljr spowelljr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need something like PreloadExists to check if the crictl binary exists before trying to download it. Right now if we try starting with a Kubernetes version that doesn't have a matching crictl version (v1.30.0-beta.0 for example) the entire start fails due to the crictl download failing (as crictl v1.30.0 doesn't exist yet).

❌  Exiting due to K8S_INSTALL_FAILED: Failed to update cluster: update primary control-plane node: transferring crictl: downloading crictl: download failed: https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.30.0/crictl-v1.30.0-linux-arm64.tar.gz: getter: &{Ctx:context.Background Src:https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.30.0/crictl-v1.30.0-linux-arm64.tar.gz Dst:/Users/powellsteven/.minikube/cache/linux/arm64/v1.30.0-beta.0/crictl.download Pwd: Mode:2 Umask:---------- Detectors:[0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40] Decompressors:map[bz2:0x140005a0fa0 gz:0x140005a0fa8 tar:0x140005a0f50 tar.bz2:0x140005a0f60 tar.gz:0x140005a0f70 tar.xz:0x140005a0f80 tar.zst:0x140005a0f90 tbz2:0x140005a0f60 tgz:0x140005a0f70 txz:0x140005a0f80 tzst:0x140005a0f90 xz:0x140005a0fc0 zip:0x140005a0fe0 zst:0x140005a0fc8] Getters:map[file:0x140024f7830 http:0x140024e2aa0 https:0x140024e2af0] Dir:false ProgressListener:0x106be2e80 Insecure:false DisableSymlinks:false Options:[0x103788bd0]}: bad response code: 404

╭───────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                           │
│    😿  If the above advice does not help, please let us know:                             │
│    👉  https://github.com/kubernetes/minikube/issues/new/choose                           │
│                                                                                           │
│    Please run `minikube logs --file=logs.txt` and attach logs.txt to the GitHub issue.    │
│                                                                                           │
╰───────────────────────────────────────────────────────────────────────────────────────────╯

So we should check for the existance of the binay first, and it it doesn't exist, log that we're not downloading matching crictl version due to it not existing and continue on using whatever version is already on the machine.

@ComradeProgrammer
Copy link
Member Author

We need something like PreloadExists to check if the crictl binary exists before trying to download it. Right now if we try starting with a Kubernetes version that doesn't have a matching crictl version (v1.30.0-beta.0 for example) the entire start fails due to the crictl download failing (as crictl v1.30.0 doesn't exist yet).

❌  Exiting due to K8S_INSTALL_FAILED: Failed to update cluster: update primary control-plane node: transferring crictl: downloading crictl: download failed: https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.30.0/crictl-v1.30.0-linux-arm64.tar.gz: getter: &{Ctx:context.Background Src:https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.30.0/crictl-v1.30.0-linux-arm64.tar.gz Dst:/Users/powellsteven/.minikube/cache/linux/arm64/v1.30.0-beta.0/crictl.download Pwd: Mode:2 Umask:---------- Detectors:[0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40 0x106c5af40] Decompressors:map[bz2:0x140005a0fa0 gz:0x140005a0fa8 tar:0x140005a0f50 tar.bz2:0x140005a0f60 tar.gz:0x140005a0f70 tar.xz:0x140005a0f80 tar.zst:0x140005a0f90 tbz2:0x140005a0f60 tgz:0x140005a0f70 txz:0x140005a0f80 tzst:0x140005a0f90 xz:0x140005a0fc0 zip:0x140005a0fe0 zst:0x140005a0fc8] Getters:map[file:0x140024f7830 http:0x140024e2aa0 https:0x140024e2af0] Dir:false ProgressListener:0x106be2e80 Insecure:false DisableSymlinks:false Options:[0x103788bd0]}: bad response code: 404

╭───────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                           │
│    😿  If the above advice does not help, please let us know:                             │
│    👉  https://github.com/kubernetes/minikube/issues/new/choose                           │
│                                                                                           │
│    Please run `minikube logs --file=logs.txt` and attach logs.txt to the GitHub issue.    │
│                                                                                           │
╰───────────────────────────────────────────────────────────────────────────────────────────╯

So we should check for the existance of the binay first, and it it doesn't exist, log that we're not downloading matching crictl version due to it not existing and continue on using whatever version is already on the machine.

Fixed. Now it is checked whether the download results in 404, and if so, no errors will be returned and whatever version is already on the machine will be used

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2024
pkg/minikube/bootstrapper/kubeadm/kubeadm.go Outdated Show resolved Hide resolved
pkg/minikube/download/binary.go Outdated Show resolved Hide resolved
pkg/minikube/download/binary.go Outdated Show resolved Hide resolved
pkg/minikube/bootstrapper/bsutil/binaries.go Outdated Show resolved Hide resolved
pkg/minikube/cruntime/cri.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComradeProgrammer
Once this PR has been reviewed and has the lgtm label, please assign medyagh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2024
hack/update/github.go Outdated Show resolved Hide resolved
hack/preload-images/generate.go Outdated Show resolved Hide resolved
hack/preload-images/generate.go Show resolved Hide resolved
pkg/minikube/download/binary.go Show resolved Hide resolved
pkg/minikube/bootstrapper/bsutil/binaries.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2024
Co-authored-by: Steven Powell <[email protected]>
@medyagh medyagh changed the title feat: download crictl at preload and runtime download crictl at preload and runtime Sep 18, 2024
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ComradeProgrammer

it is much better if we convert this PR into two PRS
1- adding the critcle to the preload genreation
2- Folow up PR that will use that, so we can test it and see if it is making anything slower

@@ -86,3 +87,32 @@ func Binary(binary, version, osName, archName, binaryURL string) (string, error)
}
return targetFilepath, nil
}

// CrictlBinary download the crictl tar archive to the cache folder and untar it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this for only when --preload is false? if yes please add it to the comment

sm := sysinit.New(k.c)

if err := bsutil.TransferBinaries(cfg.KubernetesConfig, k.c, sm, cfg.BinaryMirror); err != nil {
return errors.Wrap(err, "downloading binaries")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt this be tranferring crictlc? make it more specific, so when the error bubles up we find it easier

@@ -945,6 +946,27 @@ func (k *Bootstrapper) UpdateNode(cfg config.ClusterConfig, n config.Node, r cru

klog.Infof("kubelet %s config:\n%+v", kubeletCfg, cfg.KubernetesConfig)

sm := sysinit.New(k.c)

if err := bsutil.TransferBinaries(cfg.KubernetesConfig, k.c, sm, cfg.BinaryMirror); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is added ? I thought this was downloading critctl, why this was needed to be added here?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add matching Kubernetes Version for crictl to the preload
7 participants