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 docs about verify pouch cri #493

Merged
merged 1 commit into from
Jan 4, 2018

Conversation

liyanyanli
Copy link
Contributor

@liyanyanli liyanyanli commented Jan 3, 2018

1.Describe what this PR did
This PR adds pouch cri validation details

2.Does this pull request fix one issue?
This is related to #474

3.Describe how you did it
NONE

4.Describe how to verify it
NONE

5.Special notes for reviews
NONE

@codecov-io
Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #493 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #493   +/-   ##
=======================================
  Coverage   19.12%   19.12%           
=======================================
  Files          34       34           
  Lines        1689     1689           
=======================================
  Hits          323      323           
  Misses       1331     1331           
  Partials       35       35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 676e79a...d33225b. Read the comment docs.

@Letty5411
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 4, 2018
Copy link
Collaborator

@allencloud allencloud left a comment

Choose a reason for hiding this comment

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

I left some comments there. Maybe some improvement would make this perfect.

@@ -0,0 +1,100 @@
# Verify pouch CRI by the cri-tools
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change the file name to be pouch_with_cri_verification.go to keep consistent?
And also change the head title of this file.

@@ -0,0 +1,100 @@
# Verify pouch CRI by the cri-tools

[cri-tools](https://github.com/kubernetes-incubator/cri-tools) provides a CLI([crictl](https://github.com/kubernetes-incubator/cri-tools/blob/master/docs/crictl.md)) for CRI-compatible container runtimes. This is an easy way to verify pouch CRI without needing to set up Kubernetes components.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we had better use pouch CRI implementation rather than pouch CRI. As a result, the sentence could be This is an easy way to verify CRI implementation in pouch without setting up all Kubernetes components.

- `logs`: Fetch the logs of a container
- `help`: Shows a list of commands or help for one command

## Config runtime endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I cannot follow what does Config runtime endpoint mean. Could you explain more under this line about what this does and what is the necessity of doing this?

```
## Examples

### Run sandbox with config file
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a verification case, we should provide successful case as the crictl runs sandbox-config.json command below. While I think we still need to provide some unsuccessful cases which show users what happens if CRI implementation misses something.

```



Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove these blank line please, but keep a wrap line at the end of this file.

### Pull image

```bash
# crictl pull docker.io/library/redis:alpine
Copy link
Collaborator

Choose a reason for hiding this comment

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

In bash format, # is the beigining of a command's comment? Could you change a form of this, maybe $ or some others?

@liyanyanli liyanyanli force-pushed the leo branch 2 times, most recently from 7e5d44b to ab3d217 Compare January 4, 2018 06:28
If CRI implemention what features do not support or misses something
```
#crictl ps
FATA[0000] listing containers failed: rpc error: code = Unknown desc = ListContainers Not Implemented Yet
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 nice!

}
}

#crictl runs sandbox-config.json
Copy link
Collaborator

@allencloud allencloud Jan 4, 2018

Choose a reason for hiding this comment

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

Please add a white space between # and crictl.


If CRI implemention what features do not support or misses something
```
#crictl ps
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a white space between # and crictl.

```
### unsuccessful cases

If CRI implemention what features do not support or misses something
Copy link
Collaborator

Choose a reason for hiding this comment

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

If pouch has not fully or correctly implemented some interfaces in CRI, crictl command execution would fail:

@allencloud
Copy link
Collaborator

LGTM, if some nits are fixed.

@allencloud
Copy link
Collaborator

In the future, we will find a better folder place to locate cri-raleted docs.

@allencloud allencloud merged commit b23135c into AliyunContainerService:master Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/docs LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants