-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
*: use podman instead of docker #207
Conversation
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.
/lgtm
images/tectonic-builder/README.md
Outdated
@@ -10,5 +10,5 @@ jobs today for testing purposes. | |||
Example: | |||
|
|||
```sh | |||
docker build -t quay.io/coreos/tectonic-builder:v1.33 -f images/tectonic-builder/Dockerfile . | |||
podman build -t quay.io/coreos/tectonic-builder:v1.33 -f images/tectonic-builder/Dockerfile . |
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.
One day it should be called 'PodManfile' instead of Dockerfile.
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.
One day it should be called 'PodManfile' instead of Dockerfile.
They're going for backwards-compat. More on this in containers/buildah#851.
LGTM, what could go wrong? :) |
builder-run
Outdated
@@ -1,6 +1,6 @@ | |||
#!/bin/bash -e | |||
# | |||
# A utility script for executing arbitrary local scripts via docker. | |||
# A utility script for executing arbitrary local scripts via podman. |
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.
This script is from coreos/tectonic-installer#609, but doesn't seem to have ever been used anywhere. With installer/scripts
gone since coreos/tectonic-installer#3067, I'd guess we can just drop builder-run
entirely.
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.
👍 to drop whatever we can :)
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.
... I'd guess we can just drop builder-run entirely.
I've spun this off into #229.
@@ -4,7 +4,7 @@ set -e | |||
echo "Rendering Kubernetes core manifests..." | |||
|
|||
# shellcheck disable=SC2154 | |||
/usr/bin/docker run \ | |||
/usr/bin/podman 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.
This breaks Container Linux, no? I'll launch the smoke tests to see... [edit: @crawford already launched them :)]
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.
Yep. We'll need to remove CL support before we can merge this.
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.
#221 is ready to go (once openshift/release#1317 merges).
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.
@ashcrow can you also update bootkube.service so that it no longer depends on docker.service
?
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.
Sure. Will update shortly.
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.
⬆️
# See https://github.com/kubernetes/kubernetes/issues/43292 | ||
|
||
echo "Starting etcd certificate signer..." | ||
|
||
# shellcheck disable=SC2154 | ||
SIGNER=$(/usr/bin/docker run -d \ | ||
SIGNER=$(/usr/bin/podman run -d \ |
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.
podman
has an issue with --tmpfs
I think
Sep 05 19:13:13 dev-bootstrap bash[1448]: container create failed: container_linux.go:336: starting container process caused "process_linux.go:399: container init caused \"rootfs_linux.go:58: mounting \\\"/var/lib/containers/storage/overlay-containers/47c973458ee9cfdbc09dc50511ea73f7e03f89f21c5d53cab69b11b46f812d99/userdata/tmpfs\\\" to rootfs \\\"/var/lib/containers/storage/overlay/a5858d7251a0798e7a86412cc38865b6526d9634334c641f0dc46da03d70e675/merged\\\" at \\\"/tmp/runctmpdir916880328\\\" caused \\\"tmpcopyup: failed to move mount /tmp/runctmpdir916880328 to /var/lib/containers/storage/overlay/a5858d7251a0798e7a86412cc38865b6526d9634334c641f0dc46da03d70e675/merged/tmp: invalid argument\\\"\""
Sep 05 19:13:13 dev-bootstrap bash[1448]: : internal libpod error
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.
containers/podman#1413 should work around it
There is probably an underlying runc bug we should triage and fix, but this should unblock you
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.
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.
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.
@mrunalp sure. Let me know where we can snag it and we can make sure it makes it in.
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.
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.
current versions in RHCOS build 5165
# rpm-ostree status
State: idle
AutomaticUpdates: disabled
Deployments:
● ostree://rhcos:openshift/3.10/x86_64/os
Version: 4.0.5165 (2018-09-05 10:06:59)
Commit: 119e6fb7d7fa12d8685dd5e641bf23b56a3a76e81b7394c055f51d0388edf239
# rpm-ostree db list 119e6fb7d7fa12d8685dd5e641bf23b56a3a76e81b7394c055f51d0388edf239 | grep -e runc -e cri-o -e podman
cri-o-1.11.2-1.git3eac3b2.el7.x86_64
podman-0.7.3-1.git0791210.el7.x86_64
runc-1.0.0-37.rc5.dev.gitad0f525.el7.x86_64
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.
Let me know if something needs to be bumped, and where to get it if it is in a non-standard location.
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.
@mrunalp is this build available in the right places? Where is that being tracked? What BZ?
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.
@lsm5 is on it and we should have it today.
/retest Please review the full test history for this PR and help us cut down flakes. |
/lgtm cancel We know why this is failing and don't want the bots banging away on retests. |
@wking rebased |
rebased, but still doesn't work, right? podman is still busted?
|
Looks like your |
That's correct. I got the new location this morning and am verifying we can switch. |
We have another/different failure....
But then |
Can you drop a |
|
Looks like a problem pinging the registry, but those error messages aren't being printed properly - probably needs a fix on our end |
Correction, the error printing is in c/image - I'll see if it can't be improved |
i think this is all fixed in newer versions of podman @mheon |
@baude Negative on the c/image error - it looks like a network flake, but the error message is honestly terrible and needs to be fixed |
I'm very interested to know what the network error was. As I added I didn't have this problem with docker, but it could be that docker is just slower and so it works... |
Signed-off-by: Steve Milner <[email protected]>
Updated with a few fixes after testing this locally. |
/lgtm |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, ashcrow, crawford, rajatchopra 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 |
--rm \ | ||
--network host \ |
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.
Can you explain why you need this?
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.
It wasn't able to access the outside network without. I thought I heard that this was expected behavior with podman (though, I don't know where I heard that).
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.
this should have worked....
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.
podman should work fine without requiring --network=host. What version of podman were you testing with?
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.
@rhatdan podman version 0.7.3
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.
podman version 0.7.3
If this is a known issue with an upstream fix, we may be able to drop this after #253 takes us up to RHCOS 4.0.5595-1 with:
$ curl -s http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/cloud/4.0.5595-1/pkglist.txt | grep podman
podman-0.8.5-2.gitdc5a711.el7.x86_64
Thank you openshift/os#288 :).
Do we want to update the remaining docker instances as well? $ git log --oneline -1 origin/pr/207
c234fc3 *: use podman instead of docker
$ git grep -i docker c234fc3 | grep -v vendor/
c234fc3:config.tf:This is known to be a "Docker pull secret" as produced by the docker login [1] command.
c234fc3:config.tf:[1] https://docs.docker.com/engine/reference/commandline/login/
c234fc3:examples/tectonic.aws.yaml:# This is known to be a "Docker pull secret" as produced by the docker login [1] command.
c234fc3:examples/tectonic.aws.yaml:# [1] https://docs.docker.com/engine/reference/commandline/login/
c234fc3:examples/tectonic.libvirt.yaml:# This is known to be a "Docker pull secret" as produced by the docker login [1] command.
c234fc3:examples/tectonic.libvirt.yaml:# [1] https://docs.docker.com/engine/reference/commandline/login/
c234fc3:hack/go-fmt.sh: docker.io/openshift/origin-release:golang-1.10 \
c234fc3:hack/go-lint.sh: docker.io/openshift/origin-release:golang-1.10 \
c234fc3:hack/go-vet.sh: docker.io/openshift/origin-release:golang-1.10 \
c234fc3:images/installer-origin-release/Dockerfile.ci:# This Dockerfile is used as a CI job to build smoke tests for OpenShift installer
c234fc3:images/tectonic-installer/Dockerfile:# Docker build does not follow symlinks (see
c234fc3:images/tectonic-installer/Dockerfile:# from bazel-bin/tectonic.tar.gz to the docker build context folder of your
c234fc3:images/tectonic-installer/Dockerfile.ci:# This Dockerfile is a used as a CI job to validate Tectonic installer on CentOS
c234fc3:installer/pkg/validate/validate.go: return errors.New("overlaps with default Docker Bridge subnet (172.17.0.0/16)")
c234fc3:installer/pkg/validate/validate_test.go: {"172.17.1.2/20", "overlaps with default Docker Bridge subnet (172.17.0.0/16)"},
c234fc3:modules/bootkube/resources/manifests/pull.json: "type": "kubernetes.io/dockerconfigjson",
c234fc3:modules/bootkube/resources/manifests/pull.json: ".dockerconfigjson": "${pull_secret}"
c234fc3:modules/tectonic/resources/manifests/ingress/pull.json: "type": "kubernetes.io/dockerconfigjson",
c234fc3:modules/tectonic/resources/manifests/ingress/pull.json: ".dockerconfigjson": "${pull_secret}"
c234fc3:modules/tectonic/resources/manifests/secrets/pull.json: "type": "kubernetes.io/dockerconfigjson",
c234fc3:modules/tectonic/resources/manifests/secrets/pull.json: ".dockerconfigjson": "${pull_secret}"
c234fc3:tests/run.sh:# In future bazel build could be extracted to another job which could be running in docker container like this:
c234fc3:tests/run.sh:# docker run --rm -v $PWD:$PWD:Z -w $PWD quay.io/coreos/tectonic-builder:bazel-v0.3 bazel build tarball smoke_tests
c234fc3:tests/smoke/glide.lock:- name: github.com/docker/distribution
c234fc3:tests/smoke/glide.lock:- name: github.com/docker/engine-api
c234fc3:tests/smoke/glide.lock:- name: github.com/docker/go-connections
c234fc3:tests/smoke/glide.lock:- name: github.com/docker/go-units It looks like we could make a few |
# See https://github.com/kubernetes/kubernetes/issues/43292 | ||
|
||
echo "Starting etcd certificate signer..." | ||
|
||
# shellcheck disable=SC2154 | ||
SIGNER=$(/usr/bin/docker run -d \ | ||
--tmpfs /tmp \ |
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.
i think this was basically for security so that the keys/cert never hit the disk
Awesome job guys, love to see our dependency on Docker shrink. |
We'd removed this in c234fc3 (*: use podman instead of docker, 2018-09-05, openshift#207) to work around [1,2]. Now that RHCOS is up to: $ curl -s http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/cloud/latest/pkglist.txt | grep runc runc-1.0.0-52.dev.git70ca035.el7_5.x86_64 we can restore the option. [1]: openshift/os#284 [2]: containers/podman#1396
NOTE: This is untested ... but in theory it should work 😄
The following items were left alone:
./modules/ignition/resources/services/kubelet.service