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

docs: add getting started w/development guide #667

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

adams0619
Copy link
Contributor

@adams0619 adams0619 commented Sep 24, 2020

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Adds documentation to help with getting cri-tools up and running locally for development

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

- add getting started w/development guide

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 24, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @adams0619!

It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 24, 2020
@adams0619
Copy link
Contributor Author

/kind documentation

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 24, 2020
@adams0619 adams0619 force-pushed the dev-guide branch 2 times, most recently from c94cbb5 to 0e09544 Compare September 25, 2020 01:05
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Hey, thank you for the contribution 🙏

May I ask you to sign the CNCF CLA first?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 25, 2020
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

@adams0619 Thank you for adding this helpful documentation and your first contribution 👍 . I have some feedback provided inline and it is mostly to just simplify the doc by referencing other docs.

@@ -0,0 +1,139 @@
# Getting Started: Developing on `kubernetes`, `cri-tools`, and `containerd`
Copy link
Contributor

Choose a reason for hiding this comment

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

Might read better as: "Developer Getting Started Guide"

@@ -0,0 +1,139 @@
# Getting Started: Developing on `kubernetes`, `cri-tools`, and `containerd`

_**Note**: this documentation was written while running under **`Ubuntu Server 20.04.1 LTS`**_
Copy link
Contributor

Choose a reason for hiding this comment

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

Note sure this disclaimer is necessary as guides in k8s ecosystem are usually pitched at Ubuntu by default.


_**Note**: this documentation was written while running under **`Ubuntu Server 20.04.1 LTS`**_

## Pre-requisites / pre-flight checks
Copy link
Contributor

Choose a reason for hiding this comment

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

Header make more sense as just "Pre-requisites"

/usr/local/bin/$$critest
```

Thanks for making it this far in the guide, I hope this guide was helpful and was able to get you up and running, but if you run into any major un-foreseen issues or find errors within this guide then feel free to open an issue or better yet open a PR with a fix; as always, contributions are welcome.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this sentence as this is covered in project README

Comment on lines 6 to 40

---

Start w/the following setup guide:

[Setup-Guide-for-Kubernetes-Developers](https://developer.ibm.com/articles/setup-guide-for-kubernetes-developers/)

**Skip the `kubernetes` steps from the guide above if you don't plan on doing any development in the `kubernetes` or `crictl` projects.**

---

For the remainder of this guide, we will reference the home directory as `$HOME`.

After following the setup guide from above, before proceeding ensure you have done the following:

- You have a `go` directory within your `$HOME` directory and the `go` binary is part of your environments `$PATH`

```bash
$ ls $HOME
go

$ env | grep go
PATH=$PATH:/usr/local/go/bin:$HOME/go/bin # $PATH here refers to truncated version of additional `env` paths that are unrelated this guide / setup
```

- You have the following dependencies installed:
- `libseccomp-dev` - _a `libseccomp` development library, required for `containerd`_
- `btrfs-tools` - _required by `containerd` for `btrfs` support_
- `pkg-config` - _required for linking with `libseccomp`_

_For `Ubuntu` these can all be installed using the `apt` package manager_

_[Referenced from `cri` repo README](https://github.com/containerd/cri#install-dependencies)_

_**Important Note**: `Go` modules follow a similar structure to projects in Github, i.e. when installing and trying to use `cri-tools` as a local module, it should be installed under a folder structure as follows `go/src/github.com/kubernetes-sigs/cri-tools`. `k8s.io` is the only exception which does not follow this structure for legacy reasons._
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to compress this down a list of the tools that cri-tools need installed in a system:
e.g.

  • Go
  • Docker
  • Build tools

with a link to the install doc of each

Copy link
Contributor

Choose a reason for hiding this comment

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

My reasoning is that better to reference the tools docs as they are more dynamic and open to change. We also have to assume some knowledge of Go. Also the dependencies are for cir in the next section.

Comment on lines 46 to 52
Make note of where you installed `GO` earlier in the setup process.

If you followed the guide from the beginning you may likely already have `containerd` installed, but we will need to install the latest development version.

**Running the development versions of `containerd` is not required if you do not plan on developing for `crictl`, `containerd` or `kubernetes`, you can instead install the latest packaged version for your given OS... for `Ubuntu` run `apt install contianerd`**

_Keep in mind `cri-o` can also be used in place of `containerd/cri`, but for this guide we will be using `containerd/cri`. You may need some additional steps to ensure `cri-o` works, but the setup should be mostly the same. You can also follow the `cri-o` [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version._
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth simplifying as follows:

This guide will use containerd/cri as the container runtime. You may use cri-o instead but you may need some additional steps to ensure it works. You can also follow the cri-o install guide if you prefer to use the packaged version.

Comment on lines 54 to 105
- Clone / Fork the `cri` repo from [https://github.com/containerd/cri](https://github.com/containerd/cri)

- Pulldown the forked version of `cri` into your `$HOME/go/src/github.com/containerd/cri` and set the `upstream` repo to original project repo url.
- Note, as mentioned before that the folder you save the repo (`cri`) to locally must follow the same structure as what you see for the repositories path on `github.com`
- `cd` into your newly cloned `cri` repo on your machine

```bash
$ cd $HOME/go/src/github.com/containerd/cri
$
```

_If you are not running as `root` (which you shouldn't) the commands below may fail on the first try, re-run them by prefixing `sudo` in front to ensure any issues you encounter are not permissions related; example `sudo <command>`_

- Install any necessary build tools using `make`

```bash
$ make install.tools
```

- Install any necessary dependencies using `make`

```bash
$ make install.deps
```

- Build `containerd` from source and install its binary using `make`

```bash
$ service containerd stop # Stop containerd first, if already running
$ make && make install
```

- Run `containerd -v` to validate you are using the development version of `containerd`

```bash
$ containerd -v
WARN[2020-09-18T21:29:27.616910535Z] This customized containerd is only for CI test, DO NOT use it for distribution.
containerd github.com/containerd/containerd a536d06c-TEST
```

- Update the `containerd` config by copying the output from the `containerd config default` command and paste this output to your `containerd` config file which should be located under `/etc/containerd/config.toml`

```bash
$ containerd config default # Remaining output truncated -- copy the output from this command
WARN[2020-09-18T21:37:38.987970869Z] This customized containerd is only for CI test, DO NOT use it for distribution.
version = 2
root = "/var/lib/containerd"
state = "/run/containerd"
...
```

_Background for why this is necessary: `docker` makes use of `containerd`, but when installed it has `cri` disabled, the updated config enables `cri` and allows `docker` to make use of the development `containerd/cri` that you have been installing up to this point_
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is very valuable information on using containerd/cri. I feel however that it should reside in containerd/cri dev guide. Do you mind opening a PR there and updating its guide where necessary.

I feel in here it should just contain:

Follow the containerd/cri dev guide to make and install containerd/cri runtime.

Comment on lines 109 to 37
---

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove


Running `cri-tools` is very similar to the steps followed above

- Clone / Fork repo into `$HOME/go/src/github.com/kubernetes-sigs/cri-tools`
Copy link
Contributor

Choose a reason for hiding this comment

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

You would assume that someone has the repo by now, so not needed

Comment on lines 111 to 113
After completing the above steps you should have `go` & `contianerd` binaries installed, `PATH` properly set, and the necessary `go` modules folder setup in your `$HOME` directory.

Running `cri-tools` is very similar to the steps followed above
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be condensed down into:

Build and install cri-tools as follows:

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 3, 2020
Copy link

@dougsland dougsland left a comment

Choose a reason for hiding this comment

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

I would suggest squash the two patches.

@adams0619
Copy link
Contributor Author

adams0619 commented Oct 7, 2020

@saschagrunert Not sure why the following check (cri-test) is failing for my PR, but don't think my changes should have any effect there... any help would be appreciated.

Error:

Summarizing 6 Failures:

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward in host network 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward in host network 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward in host network 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/runner/work/cri-tools/cri-tools/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

Ran 75 of 83 Specs in 335.644 seconds
FAIL! -- 73 Passed | 2 Failed | 0 Flaked | 0 Pending | 8 Skipped


Ginkgo ran 1 suite in 5m35.67118475s
Test Suite Failed
--- FAIL: TestCRISuite (335.68s)
Error:     cri_test.go:126: critest path: /usr/local/bin/critest
Error:     cri_test.go:161: Failed to run tests in parallel: exit status 1
FAIL
Error: Process completed with exit code 1.

`cri-tools` need the following programs installed in a system in order to work correctly:

- [Go](https://golang.org/doc/install)
- [Docker](https://docs.docker.com/get-docker/)
Copy link
Member

Choose a reason for hiding this comment

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

For which part do we need Docker? I doubt that we require it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree but worth confirming

**Skip the `kubernetes` steps from the guide above if you don't plan on doing any development in the `kubernetes` or `crictl` projects.**

---
`cri-tools` need the following programs installed in a system in order to work correctly:
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase it to mention that we need those for development only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree


Running the development versions of `containerd` is not required if you do not plan on developing for `crictl`, `containerd` or `kubernetes`, you can instead install the latest packaged version for your given OS... i.e. for `Ubuntu` run `apt install contianerd`

This guide will use `containerd/cri` as the container runtime. You may use `cri-o` instead but you may need some additional steps to ensure it works. You can also follow the `cri-o` [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This guide will use `containerd/cri` as the container runtime. You may use `cri-o` instead but you may need some additional steps to ensure it works. You can also follow the `cri-o` [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version.
This guide will use `containerd/cri` as the container runtime. You may use CRI-O instead but you may need some additional steps to ensure it works. You can also follow the CRI-O [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version.

@saschagrunert
Copy link
Member

Yes the test failures are not coming from this patch, I'll check what's going on.

@saschagrunert
Copy link
Member

saschagrunert commented Oct 8, 2020

Looks like a flake, now it's working :)

**Skip the `kubernetes` steps from the guide above if you don't plan on doing any development in the `kubernetes` or `crictl` projects.**

---
`cri-tools` need the following programs installed in a system in order to work correctly:
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

`cri-tools` need the following programs installed in a system in order to work correctly:

- [Go](https://golang.org/doc/install)
- [Docker](https://docs.docker.com/get-docker/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree but worth confirming

@@ -0,0 +1,66 @@
# Developer Getting Started Guide
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, change of mind. I think the most useable title is: "Developer Guide"


[Setup-Guide-for-Kubernetes-Developers](https://developer.ibm.com/articles/setup-guide-for-kubernetes-developers/)

**Skip the `kubernetes` steps from the guide above if you don't plan on doing any development in the `kubernetes` or `crictl` projects.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove crictl as it is build in cri-tools.


If you followed the guide so far you may likely already have `containerd` installed, but we will need to install the latest development version.

Running the development versions of `containerd` is not required if you do not plan on developing for `crictl`, `containerd` or `kubernetes`, you can instead install the latest packaged version for your given OS... i.e. for `Ubuntu` run `apt install contianerd`
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove crictl as it is build in cri-tools.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this sentence is needed as it seems to conflict with the previous sentence?


## Build & install `containerd/cri`

If you followed the guide so far you may likely already have `containerd` installed, but we will need to install the latest development version.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you followed the guide so far you may likely already have containerd installed, but we will need to install the latest development version. --> You will require the latest development version of containerd.


This guide will use `containerd/cri` as the container runtime. You may use `cri-o` instead but you may need some additional steps to ensure it works. You can also follow the `cri-o` [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version.

- Follow the [`containerd/cri` dev guide](https://github.com/containerd/cri#getting-started-for-developers) to `make` and install `containerd/cri` runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

install --> install the


## Build & install `cri-tools`

You can build and install `cri-tools` following the steps below:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can build and install cri-tools following the steps below: --> You can build and install cri-tools as follows:


You can build and install `cri-tools` following the steps below:

- Install necessary build tools using `make` within the repo directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Install necessary build tools using make within the repo directory --> Install the dependencies:

$ make install.tools
```

- Build `cri-tools` from source and install its binary using `make`
Copy link
Contributor

Choose a reason for hiding this comment

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

Build cri-tools from source and install its binary using make --> Build cri-tools (critest and crictl) and install to common location:

@hickeyma
Copy link
Contributor

Thanks for updating @adams0619. Nearly there, just some more comments inline.

@adams0619
Copy link
Contributor Author

@hickeyma @saschagrunert Updated to address comments... could I get another review on this?

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Just a nit, otherwise LGTM
/approve


## Build & install `containerd/cri`

The latest development version of `containerd` is required.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The latest development version of `containerd` is required.
The latest development version of `containerd` or CRI-O is required.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

2 small nits, otherwise LGTM.


The latest development version of `containerd` is required.

Running the development versions of `containerd` is not required if you do not plan on developing for `containerd` or `kubernetes`, you can instead install the latest packaged version for your given OS... i.e. for `Ubuntu` run `apt install contianerd`
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove as this is confusing with the sentence before it.


This guide will use `containerd/cri` as the container runtime. You may use CRI-O instead but you may need some additional steps to ensure it works. You can also follow the CRI-O [install guide](https://github.com/cri-o/cri-o/blob/master/install.md#install-packaged-versions-of-cri-o) if you prefer to use the packaged version.

- Follow the [`containerd/cri` dev guide](https://github.com/containerd/cri#getting-started-for-developers) to `make` and install the `containerd/cri` runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove - because it should not be bulleted.

…ent locally

Signed-off-by: Adams Ombonga <Adams.Ombonga@ibm.com>
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your effort on this @adams0619

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adams0619, hickeyma, saschagrunert

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 merged commit 4b96616 into kubernetes-sigs:master Nov 2, 2020
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants