-
Notifications
You must be signed in to change notification settings - Fork 1.9k
WIP: update_service: Add docs for the OpenShift Update Service #26219
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
Conversation
|
@wking hey - is this for 4.6? |
|
It's a separate product, so not tied to the OCP core. But yeah, should go GA alongside the early OCP 4.6.z. |
9b258e8 to
2ae8db1
Compare
|
CI:
But after gutting a number of |
Happy to add you @wking :). |
LalatenduMohanty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to add how to install the operator and the OSUS image in this docs?
update_service/graph-data.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least two OpenShift update release payloads into container image registries reachable by the target clusters I do not think this correct. OSUS can start without any primary or secondary metadata and we have tested this in the past. Also having this type of prerequisite is not user friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a point in an Update Service that serves only empty graphs, and phrasing this as a prereq gives us space to talk about these data inputs.
Is there a way to get previews on my PRs without getting pinged when folks need real docs maintainers? |
I added you to the preview bot. If you rebase against master and then restart the build, you should get a preview. Some quick things to keep in mind:
Let me know if you run into any issues. |
|
Hey @wking @kalexand-rh - are there any updates on this getting in for this GA? |
I have time to take a look today if @wking says it's ready. I think he wanted to take a pass at fixing the modules before I gave feedback. |
d88502d to
adf5ca0
Compare
|
The preview will be available shortly at: |
kalexand-rh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pick whatever the official name for the service/Operator is and be consistent. This is actually looking good. Ping me if you want to talk about anything or need to know reasons why. :)
update_service/index.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should retitle this assembly to something more descriptive.
update_service/index.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need an intro sentence that sets the context for the assembly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this assembly is "everything you need to be an Update Service admin", and I include modules/update-service-overview.adoc to give folks context about what the Update Service is about. I'm not sure what other context to provide, unless we shard this out into several assemblies, in which case I guess we could explain the current assembly vs. the other Update Service assemblies? I'm agnostic about sharding into assemblies. If you have a particular set of assemblies you'd like to see, just tell me what they are, and I'll pivot to that structure.
update_service/index.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // You can install the {product-title} Update Service by deploying the Update Service operator, using the {product-title} xref FIXME ../../update_service/install/web-console.adoc[web console] or xref FIXME ../../update_service/install/cli.adoc[CLI]. | |
| // You can install the {product-title} Update Service by deploying the Update Service Operator by using the {product-title} xref FIXME ../../update_service/install/web-console.adoc[web console] or xref FIXME ../../update_service/install/cli.adoc[CLI]. |
I'd probably remove the xrefs and say something about "by using one of the following sections." If you keep the xrefs, you'll need to include an explicit anchor to the first heading using the H1 anchor name from the module and the context statement from the assembly. (assuming that you are trying to link to the next two modules.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the , -> by with adf5ca0732 -> 2c77ff9ce2
I will try to sort out the xrefs in a future reroll; I'd like to make it easy for folks to skip forward to view the approach they are interested in with a click here, but I may end up throwing in that towel and using your by using one of the following sections..
adf5ca0 to
2c77ff9
Compare
2c77ff9 to
e747446
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UpdateService requires registry and repository:
The UpdateService "example" is invalid:
* spec.registry: Required value
* spec.repository: Required value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how do you want users to define this? Around line 28, do we also need to set $REGISTRY=registry.example.com and $REPOSITORY=registry.example.com/openshift/ or some other thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UpdateService now only needs releases for this information. In the API docs, I picked quay.io/openshift-release-dev/ocp-release as the example value, but yeah, registry.example.com/openshift would work too.
modules/update-service-overview.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we introduce the OSUS acronym here (or somewhere) for readability and to start socializing the acronym.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, do you know if this is an acceptable acronym to socialize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like socializing acronyms, but don't block on me ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalexand-rh I assume we have an established "standard" in documents about whether and/or when we use acronyms? Assuming so I'm not suggesting we break new ground so just roll with the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jottofar, I've seen hard rules for only product-level initializations. OCP is forbidden, but OLM is fine. CVO is used throughout the docs, so I see no problem with using OSUS. It's a judgement call, but I'm not convinced that it's mine to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If customers are going to be told that this is OSUS, then I think we should go ahead and use the acronym. It's apparently confusing that customer-facing folks refer to IPI and UPI installation methods but docs don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going back and looking at this again I see I have it wrong. I was originally referring to OpenShift Container Platform Update Service. But OSUS, which is used by the team, is the acronym for OpenShift Update Service. That name was chosen, at least partly, because OpenShift Container Platform Update Service is too much of a mouthful. I think the action here is to change all occurrences of OpenShift Container Platform Update Service to OpenShift Update Service. And the latter is short enough to not worry about referring to the acronym. I don't think we want to have OpenShift Container Platform OpenShift Update Service unless we're breaking some consistency rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just remembered {product-title}. So I'm sure we need to keep {product-title} which means change would be to OpenShift Container Platform OpenShift Update Service. As I said, that really is a mouthful and it's used throughout so using OSUS, i.e. OpenShift Container Platform OSUS, seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this from OpenShift Container Platform Update Service to OpenShift Update Service throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be defaulted to A specific namespace on the cluster which appears to be something we can restrict it to as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, did this default behavior change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm agnostic about which namespaces the operator gets installed to, or if it watches all namespaces. @jottofar is more familiar with what our bundle recommends. The text we're commenting on I probably cribbed from the logging docs, which may set things up differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should come up defaulted to single namespace of "openshift-update-service". May be safer to have user leave it defaulted as did not do a lot of testing in the end with any other namespace although can't think of a reason why it would be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this
Specify the name of the Operator channel.
be
Specify the name of the CatalogSource that provides the Operator.
e747446 to
17c7c94
Compare
17c7c94 to
6d253db
Compare
kalexand-rh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, I have some questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the name of the default-operand-namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how do you want users to define this? Around line 28, do we also need to set $REGISTRY=registry.example.com and $REPOSITORY=registry.example.com/openshift/ or some other thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oc -n your-namespace get -o yaml updateservice your-name and look under status.conditions for... I dunno, exactly. We should get some CI or experiments to get more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the right value for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately we have no official graph-images today. The blog post talks through building your own. I am not excited about that approach, but there was only so much we've had time to get working in our image-release pipeline :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the graph data has not released as an image yet, the procedure for building the image is required in this doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, did you decide on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't install the Operator in this way, so I can't update this output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, did this default behavior change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this apply?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing option Enable operator recommended cluster monitoring on this namespace in the 4.6 OperatorHub console page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What credentials does the manual strategy require?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is explained in https://docs.openshift.com/container-platform/4.6/operators/user/olm-installing-operators-in-namespace.html#olm-installing-operators-from-operatorhub_olm-installing-operators-in-namespace which we'll have already linked to as a prereq:
If you select manual updates, when a newer version of an Operator is available, OLM creates an update request. As a cluster administrator, you must then manually approve that update request to have the Operator updated to the new version.
modules/update-service-overview.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, do you know if this is an acceptable acronym to socialize?
Mixing in some precedent from logging/, the Update Service blog post [1], the GitHub docs [2], and some more recent operator CRD changes like [3,4]. Kathryn didn't want us asking the user to poll [5], so I'm using POSIX-shell 'while' loops to poll on the user's behalf. [1]: https://www.openshift.com/blog/openshift-update-service-update-manager-for-your-cluster [2]: https://github.com/openshift/cincinnati-operator/blob/2df239a8486d2ba3aa0d9925e5d505105ab36afe/docs/disconnected-cincinnati-operator.md [3]: openshift/cincinnati-operator#66 [4]: openshift/cincinnati-operator#85 [5]: openshift#26219 (comment)
6d253db to
aa36f88
Compare
|
Deploy preview for osdocs ready! Built with commit cfc495b |
aa36f88 to
5d14dba
Compare
5d14dba to
cfc495b
Compare
| @@ -0,0 +1,121 @@ | |||
| [id="update-service-install-cli_{context}"] | |||
| = Installing the {product-title} Update Service by using the CLI | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session is introducing the way to install the update service operator but the title is saying installing the Update Service. I would suggest to change it to Installing the {product-title} Update Service operator by using the CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I've introduced the sections headers with either Operator or application to distinguish between operator and operand. This PR is obsolete and will be closed. Please see #29630 for latest.
| @@ -0,0 +1,34 @@ | |||
| [id="update-service-install-web-console_{context}"] | |||
| = Installing the {product-title} Update Service by using the web console | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. It's introducing the way to install the Update Service operator but the title is saying Installing the Update Service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I've introduced the sections headers with either Operator or application to distinguish between operator and operand. This PR is obsolete and will be closed. Please see #29630 for latest.
|
/close |
|
@jottofar: Closed this PR. DetailsIn response to this:
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 PR has been superseded by #29630. |
Mixing in some precedent from
logging/, the Update Service blog post, the GitHub docs, and some aspirational v1 goals ;). Still quite a few WIPs and FIXMEs scattered throughout.CC @openshift/openshift-team-cincinnati