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 installation instructions #3068

Merged
merged 1 commit into from
May 10, 2019

Conversation

h-vetinari
Copy link

Closes #2250

As talked about at the end of that issue, @TomSweeneyRedHat and @rhatdan agreed with my suggestion to merge the installation instructions from docs/tutorials/podman_tutorial.md with install.md, which I did here.

I tried to not lose any currently existing information along the way, but rather expand it to a full set of commands necessary to build (e.g. ostree). I tested all the instructions on Ubuntu 18.04 and RHEL 7.6 (on an azure VM, without having an active subscription).

For RHEL, I ran into #3045, where the build instructions (before this PR) produced a build that would only run if runc<1.0.0-rc7. In that issue, @rhatdan told me that runc needs to be built with BUILDTAGS="selinux seccomp", but that then also produces a build that is broken. Following the rabbit hole further, and also building podman with BUILDTAGS="selinux seccomp" runs into a selinux issue, which is reproducible for me (cf. this comment by @rhatdan).

I documented the install instructions that allowed me to proceed as far as possible (i.e. all builds suceed, and the failure only occurs at runtime, i.e. sudo podman build), but I presume there will be a few more kinks to iron out regarding this.

I'll comment a few more things in the diff directly.

@openshift-ci-robot openshift-ci-robot added size/L needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2019
@openshift-ci-robot
Copy link
Collaborator

Hi @h-vetinari. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rh-atomic-bot
Copy link
Collaborator

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

Copy link
Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Some more detailed comments about the changes.

@@ -8,114 +8,9 @@ commands with Podman.
**NOTE**: the code samples are intended to be run as a non-root user, and use `sudo` where
root escalation is required.

Copy link
Author

Choose a reason for hiding this comment

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

Everything from this file that was not already available in install.md has been moved there.

@@ -53,8 +52,12 @@ sudo yum module enable -y container-tools:1.0
sudo yum module install -y container-tools:1.0
```

### Installing development versions of podman

Copy link
Author

Choose a reason for hiding this comment

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

New subsection for installing dev builds on Fedora/Ubuntu

A proper description of setting up CNI networking is given in the [`cni` README](cni/README.md).
But the gist is that you need to have some basic network configurations enabled and
CNI plugins installed on your system.
## Building from scratch
Copy link
Author

Choose a reason for hiding this comment

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

Followed by the section to rebuild from scratch...

btrfs-tools \
git \
golang-go \
go-md2man \
iptables \
libassuan-dev \
libc6-dev \
Copy link
Author

Choose a reason for hiding this comment

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

Sorted this package alphabetically

libdevmapper-dev \
libglib2.0-dev \
libc6-dev \
libgpgme11-dev \
libgpgme-dev \
Copy link
Author

Choose a reason for hiding this comment

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

Update deprecated package name

```bash
git clone https://github.com/opencontainers/runc.git $GOPATH/src/github.com/opencontainers/runc
cd $GOPATH/src/github.com/opencontainers/runc
make BUILDTAGS="selinux seccomp"
Copy link
Author

Choose a reason for hiding this comment

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

This build tag has been changed compared to the tutorial due to this comment.


#### Setup CNI networking

A proper description of setting up CNI networking is given in the [`cni` README](cni/README.md).
Copy link
Author

Choose a reason for hiding this comment

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

Maintaining the link to the general description, while also providing a workable minimal setup.


* A copy of the development libraries for `ostree`, either in the form of the `libostree-dev` package from the [flatpak](https://launchpad.net/~alexlarsson/+archive/ubuntu/flatpak) PPA, or built [from source](https://github.com/ostreedev/ostree) (more on that [here](https://ostree.readthedocs.io/en/latest/#building)). As of Ubuntu 18.04, `libostree-dev` is available in the main repositories, and the PPA is no longer required.
Copy link
Author

Choose a reason for hiding this comment

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

This whole block has been split up into the above sections (and expanded with the actual build commands, as in the tutorial).

@@ -174,47 +264,32 @@ As with other Go projects, PODMAN must be cloned into a directory structure like
GOPATH
└── src
└── github.com
└── containers
└── libpod
Copy link
Author

Choose a reason for hiding this comment

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

Resolved a mix of tabs/spaces here

mkdir -p $GOPATH/src/github.com/containers
cd $_ # or cd $GOPATH/src/github.com/containers
git clone https://github.com/containers/libpod # or your fork
cd libpod
Copy link
Author

Choose a reason for hiding this comment

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

Replaced with the build instructions from the tutorial, and added BUILDTAGS="selinux seccomp" due to the discussion in #3045. I also needed to add PREFIX= to make sure all the libraries get linked into the binary correctly (although I'd hope that this could be removed again somehow).

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

Really nice start @h-vetinari thanks for taking this on. Just a few nits here and there.

make
sudo make install PREFIX=/usr
```
For installing or building podman, please see the [installation instructions](install.md).
Copy link
Member

Choose a reason for hiding this comment

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

nit Police! podman s/b "Podman" unless you're using it as a command.

install.md Outdated
@@ -53,8 +52,12 @@ sudo yum module enable -y container-tools:1.0
sudo yum module install -y container-tools:1.0
```

### Installing development versions of podman
Copy link
Member

Choose a reason for hiding this comment

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

Podman

install.md Outdated

A proper description of setting up CNI networking is given in the [`cni` README](cni/README.md).

Using the CNI plugins from above, a most basic network config is achieved with:
Copy link
Member

Choose a reason for hiding this comment

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

suggest s/most basic/more basic/

install.md Show resolved Hide resolved
install.md Outdated
```

Next, clone the source code using:
First, ensure that the go version that is found first on the $PATH (in case you built your own; see above) is sufficiently recent -
Copy link
Member

Choose a reason for hiding this comment

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

can you add a link to the golang line for "above"?

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

can you please squash the commits together?

@h-vetinari
Copy link
Author

@giuseppe: can you please squash the commits together?

No problem. Do you want this for every push or just before the final merge?

@giuseppe
Copy link
Member

giuseppe commented May 6, 2019

No problem. Do you want this for every push or just before the final merge?

just final merge works for me, but I saw you already fixed it. Thanks!

install.md Outdated
@@ -8,15 +8,14 @@
sudo pacman -S podman
```

If you have problems when running podman in [rootless](README.md#rootless) mode follow [these instructions](https://wiki.archlinux.org/index.php/Linux_Containers#Enable_support_to_run_unprivileged_containers_(optional))
If you have problems when running Podman in [rootless](README.md#rootless) mode follow [these instructions](https://wiki.archlinux.org/index.php/Linux_Containers#Enable_support_to_run_unprivileged_containers_(optional))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's just me, but when I first read this when it's resolved to a web page, I missed the link and was looking on this page. Maybe s/these instructions/the instructions here/ and have the word "here" be the link?

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

One small change for consideration, otherwise LGTM.

sudo cp runc /usr/bin/runc
```

#### CNI plugins
Copy link
Member

Choose a reason for hiding this comment

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

nice change to make these proper headings.

Copy link
Author

Choose a reason for hiding this comment

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

Just following what was in the tutorial already. ;-)

@jwhonce
Copy link
Member

jwhonce commented May 6, 2019

@h-vetinari Please add a line like:
Signed-off-by: Jhon Honce [email protected]
to your commit message. That will pass our gating tests and let us know your signing off on the code. Thanks.

@jwhonce
Copy link
Member

jwhonce commented May 6, 2019

/ok-to-test
/approve

@openshift-ci-robot openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2019
@h-vetinari h-vetinari force-pushed the master branch 2 times, most recently from 2790180 to 5168618 Compare May 7, 2019 05:46
@h-vetinari
Copy link
Author

Anything left for me to do here?

@mheon
Copy link
Member

mheon commented May 10, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: h-vetinari, jwhonce, mheon

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2019
@mheon
Copy link
Member

mheon commented May 10, 2019

Tests are green. LGTM on my end
@rhatdan @baude @TomSweeneyRedHat @vrothberg @giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented May 10, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 9ae3221 into containers:master May 10, 2019
@h-vetinari
Copy link
Author

Thanks for merging.

@rhatdan, following the install instructions for building on RHEL should give you a reproducible example for #3045, cf. your comment

@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 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 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. ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve ways to install & documentation for it
10 participants