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

[NO TESTS NEEDED] Disable docker and alias to podman in FCOS ignition #11703

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

n1hility
Copy link
Member

This PR disables the docker.sock systemd unit and sets up an overlay to replace /usr/bin/docker with a symlink to podman as discussed in [1]. A future PR will add support for dial-stdio, which would result in docker compatibility for those using docker cli ssh connections (not the local proxy/forwarding approach which is handled differently).

[1] #11643 (comment)

[NO TESTS NEEDED] since this is specific to podman machine(no testing infra yet)

Signed-off-by: Jason Greene [email protected]

@baude
Copy link
Member

baude commented Sep 22, 2021

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021
@baude
Copy link
Member

baude commented Sep 22, 2021

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 22, 2021

Any reason to not link docker.sock -> podman.sock?

@mheon
Copy link
Member

mheon commented Sep 22, 2021

/lgtm
/hold

If we want the link, that seems like a reasonable followon PR

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
@Luap99
Copy link
Member

Luap99 commented Sep 22, 2021

Do we really want an overlay fs over /usr/bin? Could this have unwanted side effects, e.g updates no longer working.
This also breaks the strict coreos file system layout, https://docs.fedoraproject.org/en-US/fedora-coreos/storage/#_mounted_filesystems

@n1hility
Copy link
Member Author

Do we really want an overlay fs over /usr/bin? Could this have unwanted side effects, e.g updates no longer working.
This also breaks the strict coreos file system layout, https://docs.fedoraproject.org/en-US/fedora-coreos/storage/#_mounted_filesystems

I assumed it wouldn't, but this could be a faulty assumption. I'll see if I can check (unless someone else who knows FCOS better than me knows off the top of their head).

The overlay is the only way I know of to do this without relying on a custom FCOS build, since it's part of the base stream and the FS is read-only. I assume we don't want to go the route of a custom build since it would require maintaining a new stream.

@n1hility
Copy link
Member Author

Actually, I might also be able to do this with overrides and just pull the package.

@n1hility
Copy link
Member Author

Spoke with FCOS devs if they had any other suggestions:

They don't have a great solution to this problem, but do plan to address it in the future. The following options exist today

  1. OverlayFS like in this PR- they said it's safe across updates but recommended 2 or 3 instead. It can get overridden if someone sshs in and does rpm-ostree usroverlay
  2. Bind mount (e.g. mount --bind /usr/bin/podman /usr/bin/docker
  3. Pull the package via overrides and a runonce systemd approach like I mentioned above, that will work post-updates
  4. Do 3 but also install podman-docker. They recommended against layering because it will add network fetches all over the place.

I'll do a revision that goes with approach 3.

BTW I also asked about filesystem conventions, and both /opt and /usr/local are linked from var, so within policy.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2021
@n1hility
Copy link
Member Author

Any reason to not link docker.sock -> podman.sock?

done

I'll do a revision that goes with approach 3.

and done :)

@rhatdan
Copy link
Member

rhatdan commented Sep 23, 2021

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 23, 2021

@baude @Luap99 @dustymabe PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM, but definitely want a head nod from one or more of the folks @rhatdan called out.

Copy link
Contributor

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

requested a few changes. As mentioned you'll need to use systemd-tmpfiles for the socket symlink. Here's the butane config I was using recently when testing this:

variant: fcos
version: 1.3.0
storage:
  files:
    - path: /etc/tmpfiles.d/podman-docker.conf
      mode: 0644
      contents:
        inline: |
          # Create a symlink from the docker socket to the podman socket.
          # Taken from https://github.com/containers/podman/blob/main/contrib/systemd/system/podman-docker.conf
          L+  /run/docker.sock   -    -    -     -   /run/podman/podman.sock
systemd:
  units:
    - name: docker.service
      enabled: false
      mask: true
    - name: docker.socket
      enabled: false
      mask: true
    - name: podman.socket
      enabled: true

pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
@n1hility n1hility force-pushed the disable-fcos-moby branch 2 times, most recently from 50cabb0 to 755eff8 Compare September 24, 2021 06:56
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Some requested changes.. Also do we need to explicitly enable podman.socket? At least on my system it's not enabled (but my system is an old system).

pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
pkg/machine/ignition.go Outdated Show resolved Hide resolved
Signed-off-by: Jason Greene <[email protected]>
Co-authored-by: Dusty Mabe <[email protected]>
@n1hility
Copy link
Member Author

Some requested changes.. Also do we need to explicitly enable podman.socket? At least on my system it's not enabled (but my system is an old system).

Thanks @dustymabe, applied those. Podman sock is already enabled here:

Enabled: boolToPtr(true),

@n1hility n1hility requested a review from dustymabe September 24, 2021 20:41
Comment on lines +201 to +202
After=network-online.target
Wants=network-online.target podman.socket
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, dustymabe, n1hility

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

@rhatdan
Copy link
Member

rhatdan commented Sep 29, 2021

/lgtm
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit d987f26 into containers:main Sep 29, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants