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

Enh/default cri #1059

Merged
merged 10 commits into from
Aug 4, 2021
Merged

Enh/default cri #1059

merged 10 commits into from
Aug 4, 2021

Conversation

grolu
Copy link
Contributor

@grolu grolu commented Jul 9, 2021 •
edited

What this PR does / why we need it:

  • Select containerd as default for cri.Name in shoots with k8s version >= 1.22
  • Make docker container runtime a first-class citizen

Which issue(s) this PR fixes:
Fixes #1039 Fixes #1038

Special notes for your reviewer:

Release note:

The Dashboard no longer adds `docker`to the list of available CRIs. You need to adapt all `CloudProfiles` and explicitly add `docker` to all MachineImageVersions which support it

`Container Runtime` is now a required field for cluster workers and defaulted to `containerd` for cluster kubernetes versions 1.22 and higher. Clusters with older kubernetes versions keep `docker` as default container runtime. If default runtime is not in the list of supported runtimes of a machine image it defaults to the first one specified in the cloud profile

@grolu
Allow modification of Container Runtime for existing workers
3c301a9
@grolu
WIP
89bef1f
@grolu
Merge branch 'enh/modify_cri' into enh/default_cri
fbf650b 
# Conflicts:
#	frontend/src/components/ShootWorkers/ContainerRuntime.vue
#	frontend/src/components/ShootWorkers/WorkerInputGeneric.vue
@grolu
WIP
db09de4
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jul 9, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 9, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 9, 2021
@grolu
Merge branch 'master' into enh/default_cri
db0190d 
# Conflicts:
#	frontend/src/components/ShootWorkers/ContainerRuntime.vue
#	frontend/src/components/ShootWorkers/ManageWorkers.vue
#	frontend/src/components/ShootWorkers/WorkerInputGeneric.vue
#	frontend/src/store/modules/shoots/index.js
#	frontend/src/utils/index.js
#	frontend/tests/unit/utils.spec.js
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 9, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 9, 2021
holgerkoser
holgerkoser reviewed Jul 23, 2021
View reviewed changes
frontend/src/components/ShootWorkers/ContainerRuntime.vue Outdated
Comment on lines 89 to 96
containerRuntime: {
get () {
return get(this.worker, 'cri.name')
},
set (value) {
set(this.worker, 'cri.name', value)
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use the name containerRuntime for the cri.name. In would prefer criName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

holgerkoser
holgerkoser reviewed Jul 28, 2021
View reviewed changes
frontend/src/components/ShootWorkers/ContainerRuntime.vue
}
},
validations,
computed: {
containerRuntimeItems () {
const containerRuntimes = map(this.machineImageCri, 'name')
return uniq([...containerRuntimes, DEFAULT_CONTAINER_RUNTIME])
return map(this.machineImageCri, 'name')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose to rename the property containerRuntimeItems to criNames

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

holgerkoser
holgerkoser reviewed Jul 28, 2021
View reviewed changes
frontend/src/components/ShootWorkers/ContainerRuntime.vue Outdated
},
ociRuntimeItems () {
const containerRuntime = find(this.machineImageCri, ['name', this.containerRuntime])
const ociRuntimes = get(containerRuntime, 'containerRuntimes', [])
return map(ociRuntimes, 'type')
const ociRuntimess = get(containerRuntime, 'containerRuntimes', [])
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also rename ociRuntimeItems to criContainerRuntimeTypes. This helps me to map the property to the corresponding part in the cloudProfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

holgerkoser
holgerkoser reviewed Jul 28, 2021
View reviewed changes
frontend/src/components/ShootWorkers/ContainerRuntime.vue Outdated
const ociRuntimess = get(containerRuntime, 'containerRuntimes', [])
return map(ociRuntimess, 'type')
},
containerRuntime: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename containerRuntime to criName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

holgerkoser
holgerkoser reviewed Jul 28, 2021
View reviewed changes
frontend/src/components/ShootWorkers/ContainerRuntime.vue Outdated
set(this.worker, 'cri.name', value)
}
},
ociRuntimes: {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename ociRuntimes to selectedCriContainerRuntimeTypes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@grolu @holgerkoser
Apply suggestions from code review
0dd541e 
Co-authored-by: Holger Koser <holger.koser@sap.com>
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 2, 2021
@grolu grolu marked this pull request as ready for review August 3, 2021 09:01
@grolu grolu requested a review from petersutter as a code owner August 3, 2021 09:01
@grolu @holgerkoser
Update frontend/src/components/ShootWorkers/ContainerRuntime.vue
7fbffec 
Co-authored-by: Holger Koser <holger.koser@sap.com>
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 4, 2021
petersutter
petersutter approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Aug 4, 2021
holgerkoser
holgerkoser approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

/lgtm

@grolu grolu merged commit 1320d69 into master Aug 4, 2021
@grolu grolu deleted the enh/default_cri branch August 4, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holgerkoser holgerkoser holgerkoser approved these changes

@petersutter petersutter petersutter approved these changes

Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
No milestone
7 participants
@grolu @holgerkoser @petersutter @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot