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

Update the service manifest for Docker #9465

Merged
merged 9 commits into from
Jul 1, 2020

Conversation

hakman
Copy link
Member

@hakman hakman commented Jun 30, 2020

Updating based on latest Docker service manifest:
https://github.com/docker/docker-ce-packaging/blob/360f755c768ebb06c9974705674302098bb9c398/systemd/docker.service

This ended up being a bit bigger than expected, but mostly because a new version of github.com/blang/semver/v4 was needed to parse the docker version, which contains leading 0s.

Summary (with similar commit names):

  • add cgroup related dependencies
  • use "infinity" for LimitNOFILE and LimitNPROC for less accounting overhead
  • remove workaround for old unsupported Docker versions
  • remove workaround for socket activation on CentOS/RHEL distros
  • rearrange manifest to match official manifest
  • add containerd service dependency for newer Docker versions (with new semver as prerequisite)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 30, 2020
@hakman hakman changed the title Update the service manifest for Docker [WIP] Update the service manifest for Docker Jun 30, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2020
@johngmyers
Copy link
Member

Appears to also remove code for unsupported old docker versions and remove a RHEL workaround.

@hakman
Copy link
Member Author

hakman commented Jul 1, 2020

Appears to also remove code for unsupported old docker versions and remove a RHEL workaround.

That is the intent. The unsupported old docker versions were already removed and the RHEL workaround should no longer be needed.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/api area/channels and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2020
@hakman hakman changed the title [WIP] Update the service manifest for Docker Update the service manifest for Docker Jul 1, 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 Jul 1, 2020
@olemarkus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020
@hakman
Copy link
Member Author

hakman commented Jul 1, 2020

/test pull-kops-e2e-kubernetes-aws

1 similar comment
@hakman
Copy link
Member Author

hakman commented Jul 1, 2020

/test pull-kops-e2e-kubernetes-aws

@rifelpet
Copy link
Member

rifelpet commented Jul 1, 2020

overall this looks good to me. regarding the removed docker versions, is there any api validation that we should do there to alert users to upgrade if their manifest is still specifying an older docker version? or what exactly will the user experience be in that case?

@hakman
Copy link
Member Author

hakman commented Jul 1, 2020

@rifelpet The validation was introduced in a previous commit:

if config.Version != nil {
if strings.HasPrefix(*config.Version, "1.1") {
allErrs = append(allErrs, field.Invalid(fldPath.Child("version"), config.Version,
"version is no longer available: https://www.docker.com/blog/changes-dockerproject-org-apt-yum-repositories/"))
} else {
valid := []string{"17.03.2", "17.09.0", "18.03.1", "18.06.1", "18.06.2", "18.06.3", "18.09.3", "18.09.9", "19.03.4", "19.03.8", "19.03.11"}
allErrs = append(allErrs, IsValidValue(fldPath.Child("version"), config.Version, valid)...)
}
}

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020
@rifelpet
Copy link
Member

rifelpet commented Jul 1, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, rifelpet

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 781279e into kubernetes:master Jul 1, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 1, 2020
@hakman hakman deleted the docker-service-update branch July 1, 2020 19:21
k8s-ci-robot added a commit that referenced this pull request Jul 3, 2020
…pstream-release-1.18

Automated cherry pick of #9465: Update the service manifest for Docker
jsafrane added a commit to jsafrane/kops that referenced this pull request Jun 8, 2023
Nothing seems to use it.

It was introduced as docker-ce dependency in
kubernetes#9465, however, docker's RPMs never
depended on it.

I went through
https://github.com/docker/docker-ce-packaging/tree/360f755c768ebb06c9974705674302098bb9c398/rpm
(referred by the PR above) and today's RPMs in
https://github.com/docker/docker-ce-packaging/tree/master/rpm, they don't
require libcgroup.
jsafrane added a commit to jsafrane/kops that referenced this pull request Jun 8, 2023
Nothing seems to use it.

It was introduced as docker-ce dependency in
kubernetes#9465, however, docker's RPMs never
depended on it.

I went through
https://github.com/docker/docker-ce-packaging/tree/360f755c768ebb06c9974705674302098bb9c398/rpm
(referred by the PR above) and today's RPMs in
https://github.com/docker/docker-ce-packaging/tree/master/rpm, they don't
require libcgroup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/channels area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants