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.

Sign up for GitHub

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

Jump to bottom

docs: add doc about cri-annotation-support #2216

Merged
merged 1 commit into from
Sep 17, 2018
Merged

docs: add doc about cri-annotation-support #2216

merged 1 commit into from
Sep 17, 2018

Conversation

starnop
Copy link
Contributor

@starnop starnop commented Sep 7, 2018

Signed-off-by: Starnop starnop@163.com

Ⅰ. Describe what this PR did

Currently, PouchContainer has lots of advantages over other container runtimes. Kubernetes has a hiden way to support this: make user-defined parameters in annotations field in pod's definition.

When CRI manager deals the annotation details, it could pass these parameters to container manager, and container manager definitely implement these features very well.

Now we have supported some fields, such as :

  • Make runtime choosing supported
  • Make lxcfs configurable supported

So a detailed documentation is needed to cover the annotations supported.

Ⅱ. Does this pull request fix one issue?

None.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

None.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@starnop starnop force-pushed the cri-annotation-doc branch 3 times, most recently from fbb1847 to ffefd19 Compare September 7, 2018 10:55
@allencloud
Copy link
Collaborator

Please check the CI failure:

./docs/kubernetes/pouch_cri_annotation_support.md:5: MD007 Unordered list indentation
./docs/kubernetes/pouch_cri_annotation_support.md:1: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:2: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:3: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:4: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:5: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:6: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:7: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:8: MD009 Trailing spaces
./docs/kubernetes/pouch_cri_annotation_support.md:9: MD009 Trailing spaces
......

For more details, you should check https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md @starnop

@starnop starnop force-pushed the cri-annotation-doc branch 13 times, most recently from e5acad4 to 07c63db Compare September 10, 2018 04:57
@pouchrobot pouchrobot added size/XL and removed size/L labels Sep 10, 2018
@starnop starnop force-pushed the cri-annotation-doc branch 2 times, most recently from ffb3a2d to 51872a7 Compare September 10, 2018 05:10
@codecov-io
Copy link

codecov-io commented Sep 10, 2018 •
edited
Loading

Codecov Report

Merging #2216 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2216      +/-   ##
==========================================
+ Coverage   66.56%   66.62%   +0.05%     
==========================================
  Files         208      208              
  Lines       16756    16756              
==========================================
+ Hits        11153    11163      +10     
+ Misses       4276     4274       -2     
+ Partials     1327     1319       -8
Flag Coverage Δ
#criv1alpha1test 32.97% <ø> (+0.02%) ⬆️
#criv1alpha2test 33.61% <ø> (+0.14%) ⬆️
#integrationtest 39.93% <ø> (+0.01%) ⬆️
#nodee2etest 33.77% <ø> (+0.08%) ⬆️
#unittest 23.85% <ø> (ø) ⬆️
Impacted Files Coverage Δ
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
daemon/containerio/cri_log_file.go 84.31% <0%> (-3.93%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
daemon/containerio/container_io.go 74.58% <0%> (-1.11%) ⬇️
daemon/mgr/container.go 57.59% <0%> (-0.21%) ⬇️
ctrd/container.go 60.23% <0%> (+0.47%) ⬆️
cri/v1alpha2/cri.go 72.52% <0%> (+0.63%) ⬆️
daemon/mgr/container_utils.go 84.33% <0%> (+1.2%) ⬆️
daemon/mgr/spec_linux.go 75.97% <0%> (+1.5%) ⬆️
pkg/meta/store.go 64.06% <0%> (+1.56%) ⬆️
... and 1 more

@starnop starnop force-pushed the cri-annotation-doc branch 4 times, most recently from 113a2d2 to d342014 Compare September 10, 2018 16:40
allencloud
allencloud requested changes Sep 11, 2018
View reviewed changes
docs/kubernetes/pouch_cri_api_annotations.md Outdated
labels:
pouch: pouch
annotations:
io.kubernetes.lxcfs.enabled: "true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this part, you said to support runtime choosing. Thus, this line is incorrect. This is about lxcfs.

docs/kubernetes/pouch_cri_api_annotations.md Outdated
- top
resources:
requests:
memory: "256Mi"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is about runtime choosing, I suggest to delete other unrelated parts in yml file. Only covered what we wish, and that is enough.

docs/kubernetes/pouch_cri_api_annotations.md Outdated
3. After setting up your kubernetes cluster, you can create a deployment like this :

```
# cat pouch.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can specify the yml file's name to pouch-runtime.yml to make it more clear.
Or use pouch-lxcfs.yml to classify.

docs/kubernetes/pouch_cri_api_annotations.md Outdated
# kubectl create -f pouch.yaml
```

4. Use command `pouch ps` to observe the runtime of the container.
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 more validation details to help user know whether it is successful. More specific, please. Thanks a lot.

docs/kubernetes/pouch_cri_api_annotations.md Outdated
3. After setting up your kubernetes cluster, you can create a deployment like this :

```
# cat pouch.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

pouch-lxcfs.yml?

docs/kubernetes/pouch_cri_api_annotations.md Outdated
# kubectl create -f pouch.yaml
```

4. View the results.
Copy link
Collaborator

Choose a reason for hiding this comment

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

More details to be specific if you see what, it means success.

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@starnop starnop force-pushed the cri-annotation-doc branch 2 times, most recently from 3b794c3 to 2fc4d57 Compare September 13, 2018 02:21
@allencloud
Copy link
Collaborator

CI fails with ci/circleci: markdownlint-misspell-shellcheck . @starnop

@starnop starnop force-pushed the cri-annotation-doc branch 2 times, most recently from 4db1238 to de92e30 Compare September 14, 2018 03:03
@pouchrobot pouchrobot added size/XL and removed size/L labels Sep 14, 2018
@starnop starnop force-pushed the cri-annotation-doc branch 8 times, most recently from 899b80a to f553b9b Compare September 14, 2018 03:56
@starnop
docs: add doc about cri-annotation-support
411a89f 
Signed-off-by: Starnop <starnop@163.com>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Sep 17, 2018
@allencloud allencloud merged commit 318e7fd into AliyunContainerService:master Sep 17, 2018
@pouchrobot pouchrobot mentioned this pull request Sep 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allencloud allencloud allencloud approved these changes

Assignees
No one assigned
Labels
areas/docs areas/kubernetes LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@starnop @allencloud @codecov-io @CLAassistant @pouchrobot