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.

Sign up for GitHub

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

Jump to bottom

Download once for crio #6998

Merged
merged 3 commits into from
Dec 21, 2020
Merged

Download once for crio #6998

merged 3 commits into from
Dec 21, 2020

Conversation

LuckySB
Copy link
Contributor

@LuckySB LuckySB commented Dec 5, 2020

Download all images on one server and copy it to all others

/kind feature

docker deprecated in k8s 1.20, so time to extend docker features to other environments

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LuckySB

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from bozzo and EppO December 5, 2020 18:37
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2020
@LuckySB LuckySB force-pushed the download_once_crio branch from 47922be to 02033a4 Compare December 5, 2020 18:43
@EppO
Copy link
Contributor

EppO commented Dec 6, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 6, 2020
@LuckySB LuckySB changed the title WIP: Download once for crio Download once for crio Dec 7, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2020
@LuckySB LuckySB force-pushed the download_once_crio branch from 3fa87f1 to 8d42b9c Compare December 7, 2020 11:03
@hafe
Copy link
Contributor

hafe commented Dec 7, 2020

#6853

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 7, 2020

#6853

Very bad PR
with this PR kubesparay fails when install k8s v1.18.x on task install crio-1.19 from repo for version 1.18.

I fix crio cluster upgrade procedure in anothe PR

@hafe
Copy link
Contributor

hafe commented Dec 8, 2020

#6853

Very bad PR
with this PR kubesparay fails when install k8s v1.18.x on task install crio-1.19 from repo for version 1.18.

I fix crio cluster upgrade procedure in anothe PR

Well it was approved by the community and it solves the problem! I don't understand why you mix 1.18 and 1.19 in your comment?
We should not merge prs that consciously break functionality, it would not even be possible with CI in place for this.

@hafe
Copy link
Contributor

hafe commented Dec 8, 2020

And why do you just revert work done by someone else in the same dev cycle without any discussion?

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 8, 2020 •
edited
Loading

#6853

Very bad PR
with this PR kubesparay fails when install k8s v1.18.x on task install crio-1.19 from repo for version 1.18.
I fix crio cluster upgrade procedure in anothe PR

Well it was approved by the community and it solves the problem! I don't understand why you mix 1.18 and 1.19 in your comment?

The community can be wrong too.
Try to take the current master, with your PR and install the cluster version 1.18.12 with cri-o on Centos.

We should not merge prs that consciously break functionality, it would not even be possible with CI in place for this.

I completely agree with you.
Sorry your PR consciously break functionality.

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 8, 2020

@hafe #7003

@hafe
Copy link
Contributor

hafe commented Dec 8, 2020

I still don't understand why you from master (1.19) should be able to deploy 1.18. To me that is a major design flaw in kubespray! If you want 1.18 use the 2.14 branch! There is no CI for this use case. Hard to see the promised "production grade" as long as something not tested in CI is allowed by design

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 8, 2020

master is not only for 1.19. kubespray allow to install all supported vesion of k8s
For this purpose we have variable:

## The minimum version working
kube_version_min_required: v1.17.0

@EppO
Copy link
Contributor

EppO commented Dec 8, 2020 •
edited
Loading

I agree we should match the CRI-O version with the k8s one, as stated in their support matrix. I assume that PR is to make it sure of that?
Support matrix of kubespray in the master branch and the different branches is another story worth discussing but this PR is not the right place IMHO.

@EppO
Copy link
Contributor

EppO commented Dec 8, 2020 •
edited
Loading

I remember I did a similar logic with crictl, see

kubespray/roles/download/defaults/main.yml

Lines 90 to 95 in 00e0f3b

kube_major_version: "{{ kube_version | regex_replace('^v([0-9])+\\.([0-9]+)\\.[0-9]+', 'v\\1.\\2') }}"
crictl_supported_versions:
v1.19: "v1.19.0"
v1.18: "v1.18.0"
v1.17: "v1.17.0"
crictl_version: "{{ crictl_supported_versions[kube_major_version] }}"

maybe we could reuse that for CRI-O?

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 8, 2020

@EppO please see #7003

@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 9, 2020

@hafe PR #6853 broke CI

https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/899036499

@hafe
Copy link
Contributor

hafe commented Dec 9, 2020

@hafe PR #6853 broke CI

https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/899036499

How can something already merged break ci?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2020
@EppO
Copy link
Contributor

EppO commented Dec 9, 2020 •
edited
Loading

@LuckySB: need to rebase this PR since #7003 was merged

@LuckySB LuckySB force-pushed the download_once_crio branch from 8d42b9c to 0f64ac0 Compare December 9, 2020 17:48
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2020
@LuckySB
Copy link
Contributor Author

LuckySB commented Dec 9, 2020

@hafe PR #6853 broke CI
https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/899036499

How can something already merged break ci?

see log.
in your PR set version cri-o-runc, but this pkg removed from repos yesteraday

@floryut
Copy link
Member

floryut commented Dec 21, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2020
@floryut floryut mentioned this pull request Dec 21, 2020
Closed
@k8s-ci-robot k8s-ci-robot merged commit 096bcdd into kubernetes-sigs:master Dec 21, 2020
@RickHaan RickHaan mentioned this pull request Jan 11, 2021
Merged
LuckySB added a commit to southbridgeio/kubespray that referenced this pull request Jan 17, 2021
@LuckySB
Download once for crio (kubernetes-sigs#6998)
42a6b8f 
* download run once feature for CRI-O

* fix typo

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bozzo bozzo Awaiting requested review from bozzo

@EppO EppO Awaiting requested review from EppO

Assignees

@EppO EppO

@floryut floryut

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LuckySB @k8s-ci-robot @EppO @hafe @floryut