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

Documentation for opaque integer resources (alpha feature in 1.5) #1783

Merged
merged 1 commit into from
Dec 2, 2016
Merged

Conversation

ConnorDoyle
Copy link
Contributor

@ConnorDoyle ConnorDoyle commented Nov 24, 2016

Feature issue: 76
Implementation PR: 31652

cc @kubernetes/sig-scheduling @timothysc

Also touches multiple instances of the API object path for pod containers. Can break this up into a separate commit/PR on request.


This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2016
@davidopp davidopp assigned davidopp and unassigned thockin Nov 24, 2016
Second, users must request the opaque resource in pods.

To advertise a new opaque integer resource, the cluster operator should
submit a `PATCH` HTTP request to the API server to specify the available
Copy link
Member

Choose a reason for hiding this comment

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

you should mention specifically that you patch the capacity, even though the next sentence mentions it.

quantity on a node in the cluster. After this operation, the node's
`status.capacity` will include a new resource. The `status.allocatable`
field is updated automatically with the new resource asychronously by
the Kubelet.
Copy link
Member

Choose a reason for hiding this comment

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

mention that it can be used in scheduling only once this happens.

@davidopp
Copy link
Member

LGTM

I had two minor nits but since the doc deadline is very soon I think it's OK as is and you can fix that later.

@ConnorDoyle
Copy link
Contributor Author

ConnorDoyle commented Nov 24, 2016 via email

@ConnorDoyle
Copy link
Contributor Author

@davidopp addressed your comments (amended commit)

@ConnorDoyle
Copy link
Contributor Author

cc @kubernetes/docs

@@ -269,6 +269,86 @@ LastState: map[terminated:map[exitCode:137 reason:OOM Killed startedAt:2015-07-0

We can see that this container was terminated because `reason:OOM Killed`, where *OOM* stands for Out Of Memory.

## Alpha Features

Choose a reason for hiding this comment

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

This isn't rendered correctly, at least from my browser:

screen shot 2016-11-28 at 10 57 04 am

Choose a reason for hiding this comment

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

@ConnorDoyle @davidopp Did you also see the same?

Copy link
Member

Choose a reason for hiding this comment

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

Yup I see it this way too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you using the GHFM preview or viewing the netify rendered output (see https://deploy-preview-1783--kubernetes-io-vnext-staging.netlify.com/docs/user-guide/compute-resources/)

Copy link
Member

Choose a reason for hiding this comment

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

I see the same issue in github as others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aveshagarwal please read my comment. Github markdown preview and jekyll aren't the same. Also, the point at which the rendering is broken isn't part of the change set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -269,6 +269,86 @@ LastState: map[terminated:map[exitCode:137 reason:OOM Killed startedAt:2015-07-0

We can see that this container was terminated because `reason:OOM Killed`, where *OOM* stands for Out Of Memory.

## Alpha Features

### Opaque Integer Resources (Alpha Feature)
Copy link
Member

@timothysc timothysc Nov 28, 2016

Choose a reason for hiding this comment

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

Seems weird to add "Alpha Feature" to the title when it's under the TOC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

One or the other. Either don't have an Alpha Features heading (and mark features Alpha), or don't include "Alpha" in each feature's individual heading.

Also, avoid nesting headings directly under one another; that's indicative of structural problems. If you have a header with nothing under it but another header, one or the other is not necessary.


The HTTP request below advertises 5 "foo" resources on node `k8s-node-1`.

_NOTE: `~1` is the encoding for the character `/` in the patch path._
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be confusing to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which part, this doc or the way slash is encoded in JSON-pointer?

Copy link
Member

Choose a reason for hiding this comment

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

slash encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would a link to the JSON-pointer RFC help?

Copy link
Member

Choose a reason for hiding this comment

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

You'll never hear me say 'no' on the argument of add verbosity for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will add some more info about it (fact that JSON-patch makes use of JSON-pointer may be fairly niche)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@@ -269,6 +269,86 @@ LastState: map[terminated:map[exitCode:137 reason:OOM Killed startedAt:2015-07-0

We can see that this container was terminated because `reason:OOM Killed`, where *OOM* stands for Out Of Memory.

## Alpha Features

### Opaque Integer Resources (Alpha Feature)
Copy link
Contributor

Choose a reason for hiding this comment

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

One or the other. Either don't have an Alpha Features heading (and mark features Alpha), or don't include "Alpha" in each feature's individual heading.

Also, avoid nesting headings directly under one another; that's indicative of structural problems. If you have a header with nothing under it but another header, one or the other is not necessary.


### Opaque Integer Resources (Alpha Feature)

Opaque integer resources were introduced as an alpha feature in Kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

Stick with present tense; no past or future.

"Kubernetes version 1.5 introduces Opaque integer resources. Opaque integer resources allow cluster operators to advertise new node-level resources that would be otherwise unknown to the system."

Move the last sentence: "So far only resource accounting is implemented" into a note about the Alpha state underneath the first paragraph, like so:

"Note: Opaque integer resources are Alpha in Kubernetes version 1.5. Only resource accounting is implemented; node-level isolation is still under active development."

@ConnorDoyle
Copy link
Contributor Author

@devin-donnelly thanks for the review comments, please take another look.

@devin-donnelly
Copy link
Contributor

Awesome, thanks. I can go ahead and merge this for 1.5.

@devin-donnelly devin-donnelly merged commit 5b0c6dc into kubernetes:release-1.5 Dec 2, 2016
@ConnorDoyle ConnorDoyle deleted the oir-alpha branch December 2, 2016 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants