Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 14, 2018

@ghost ghost added branch/enterprise-3.9 peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 14, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 14, 2018
@ghost ghost added this to the Next Release milestone Mar 14, 2018
Copy link

@qinpingli qinpingli Mar 15, 2018

Choose a reason for hiding this comment

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

s/pvc.spec.resources.requests/pvc.spec.resources.requests.storage/

Copy link

Choose a reason for hiding this comment

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

This is shop talk, i dont think its relevant to the end user.

@vikram-redhat
Copy link
Contributor

@openshift/team-documentation - PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, the controller applies
(also there may be an extra space between "the" and "controller")

Copy link

Choose a reason for hiding this comment

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

I wouldn't mention any of the 'internal workings' of this, it will confuse the user.. (no controller, no attributes on PVC objects, nothing on how it works.. this should only show the UX..

Copy link
Contributor

Choose a reason for hiding this comment

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

It watches

Copy link

Choose a reason for hiding this comment

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

Would boil this down to say that the PVC status will reflect that the resize is happening (the rest is not end user friendly)

Copy link
Contributor

Choose a reason for hiding this comment

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

goroutine? Should this be 2 separate words? Not sure what this is.

Copy link

Choose a reason for hiding this comment

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

this should not include discussion of the internals except where they are visible to the user (no mention of controller, goroutine, etc..)

Copy link
Contributor

Choose a reason for hiding this comment

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

s/plugin/plug-in

Copy link

Choose a reason for hiding this comment

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

shouldn't mention 'goroutine" and i'd clarrify level-based / edge-based.. thats too shop-talky for end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/plugin/plug-in (a few instances here and across the page)

Copy link

Choose a reason for hiding this comment

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

Shop talk not for end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the content under "Depending on the volume type" be taken out of the nested list format? I guess it just depends on how it looks when built.

Copy link

Choose a reason for hiding this comment

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

Would only mention the user visible parts here, not controller.. would also re-phrase to remove shop talk.

Copy link
Contributor

Choose a reason for hiding this comment

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

possible extra space before `pvc.Status.Conditions

Copy link

Choose a reason for hiding this comment

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

Would simplify to only UX and omit the flow/details.

Copy link
Contributor

Choose a reason for hiding this comment

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

plug-in

Copy link

Choose a reason for hiding this comment

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

Would remove all shop talk here too. "Once a pod is schedule, the node (avoid kubelet in OSE docs) performs the file system resize is sufficient.

Copy link
Contributor

@ahardin-rh ahardin-rh Mar 16, 2018

Choose a reason for hiding this comment

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

s/loops/cycles (maybe?)

`volume_expand_controller`'s

Copy link
Contributor

Choose a reason for hiding this comment

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

plug-in

Copy link

Choose a reason for hiding this comment

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

more shop talk, mention of API or how the controller works should be avoided.

Copy link
Contributor

Choose a reason for hiding this comment

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

`true`

plug-in

Copy link

Choose a reason for hiding this comment

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

This is in shop-talk form. It should explain specifically how the feature gate works in OSE (where to add the flag etc..)

Copy link
Contributor

Choose a reason for hiding this comment

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

`true`

Copy link
Contributor

Choose a reason for hiding this comment

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

remove "aforementioned"

s/users can/you can
s/their/your

@ahardin-rh ahardin-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 16, 2018
@ahardin-rh
Copy link
Contributor

This topic needs to be added to the _topic_map.yml file so that it builds as part of the doc set. That will also allow you to run asciibinder build to preview the content before we publish. 🌟

Copy link
Member

Choose a reason for hiding this comment

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

I am still not convinced why should Openshift documentation talk about Kubernetes version. We are introducing Volume expansion as a tech. preview in 3.9, that is what we should document. cc @childsb

Copy link

Choose a reason for hiding this comment

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

+1 with @gnufied .... Everything should be in the context of OSE.

Copy link
Member

Choose a reason for hiding this comment

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

Lets add a new section for Expanding volume types with file system. Such as - EBS, GCE-PD, Cinder and RBD. That section should clearly document 2 step process.

Step 1: Edit your PVC to have new size. User should wait for PVC to have condition PersistentVolumeClaimFileSystemResizePending
Step 2: User can describe the PVC and when the PVC has condition PersistentVolumeClaimFileSystemResizePending then user can (re-)start a pod to finish file system resize of volume on node.

Copy link
Member

Choose a reason for hiding this comment

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

It appears that we are documenting internals of Volume Expand Controller here. There is no harm in doing that but the top section of this documentation should start with "How to Expand PVCs". Not with how volume expansion works.

Copy link

Choose a reason for hiding this comment

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

We shouldn't have this, only the UX.. it will confuse users.

Copy link

Choose a reason for hiding this comment

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

should only show the UX (user wont know what volume plugin based api call means)

@childsb
Copy link

childsb commented Mar 16, 2018

I left lots of line item comments, but I dont think these docs have the correct audience... The upstream Kube docs are aimed at platform vendors and deeply technical tire kickers.. Some of the value-add of OSE is making kube user-friendly and much of that is the human-readable/non-geek docs

The end-user shouldn't be exposed to any internals, only the UX they will have.. Docs shouldn't mention of kube, or any implementation details, only how to enable & use the feature, or how errors are reported to user. Components should be framed in the context of OSE and use end-user friendly terms..

@childsb
Copy link

childsb commented Mar 16, 2018

@tmorriso-rh Yes that's probably good. Maybe add where the error on failure is visible.

@gnufied does this feature need to be enabled in 3.9 to use, or is on by default? Are there any extra deployment/setup steps?

@ghost
Copy link
Author

ghost commented Mar 19, 2018

@childsb @gnufied PTAL at the updated file. Thanks.

@qinpingli
Copy link

Now, according the content of the PR, seems it should be linked to here: https://github.com/openshift/openshift-docs/tree/master/install_config/persistent_storage

@ghost
Copy link
Author

ghost commented Mar 28, 2018

@aheslin Ali, can you review this PR to determine where this content should be placed? It's currently targeted for the developer guide. If you need additional details, please see the comments on this Trello card: https://trello.com/c/GdqaM5R3/531-resize-perform-cloudprovider-resize-in-controller

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2018
@gnufied
Copy link
Member

gnufied commented Apr 2, 2018

looks good to me from technical point of view. Assigning to @ahardin-rh for further approval.

@gnufied
Copy link
Member

gnufied commented Apr 2, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2018
@gnufied
Copy link
Member

gnufied commented Apr 2, 2018

/assign @ahardin-rh

@ghost
Copy link
Author

ghost commented Apr 3, 2018

Bump.

@aheslin Ali, can you review this PR to determine where this content should be placed? It's currently targeted for the developer guide. If you need additional details, please see the comments on this Trello card: https://trello.com/c/GdqaM5R3/531-resize-perform-cloudprovider-resize-in-controller

@ahardin-rh Ashley, can you review the link to the Admission Controllers topic? Should this be for 3.7 or 3.9?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tmorriso-rh This path should be:

xref:../architecture/additional_concepts/admission_controllers.adoc#architecture-additional-concepts-admission-controllers[Admission Controllers]

It is also good to run asciibinder build and test the links in your preview build

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2018
@ghost ghost added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Apr 3, 2018
@ghost
Copy link
Author

ghost commented Apr 4, 2018

@gnufied Before merging, can you please verify if this also applies to 3.10, dedicated, or online? Thanks.

@gnufied
Copy link
Member

gnufied commented Apr 4, 2018

@tmorriso-rh it should apply to 3.10 as well. And since online is mostly managed cluster, I am not sure yet if we are going to enable resizing feature in the cluster. I would for now, abstain from making this part of openshift online docs.

@ghost ghost modified the milestones: Future Release, Next Release Apr 5, 2018
@ghost ghost merged commit 3a2e64c into openshift:master Apr 5, 2018
@ghost
Copy link
Author

ghost commented Apr 5, 2018

/cherrypick enterprise-3.9

@openshift-cherrypick-robot

@tmorriso-rh: new pull request created: #8602

In response to this:

/cherrypick enterprise-3.9

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.

@ghost
Copy link
Author

ghost commented Apr 5, 2018

/cherrypick enterprise-3.10

@openshift-cherrypick-robot

@tmorriso-rh: new pull request created: #8603

In response to this:

/cherrypick enterprise-3.10

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.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-3.9 branch/enterprise-3.10 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants