Skip to content

Conversation

dagrayvid
Copy link
Contributor

No description provided.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 21, 2021
@netlify
Copy link

netlify bot commented Aug 21, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: f2b8eba

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/61438203895d87000850f129

😎 Browse the preview: https://deploy-preview-35687--osdocs.netlify.app/openshift-enterprise/latest/hardware_enablement/psap-special-resource-operator

@dagrayvid dagrayvid changed the title WIP: Add hardware_enablement category and SRO documentation Add hardware_enablement category and SRO documentation Aug 30, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2021
@dagrayvid dagrayvid force-pushed the add-sro branch 2 times, most recently from 957310a to 44963d0 Compare August 31, 2021 20:30
Copy link
Contributor

Choose a reason for hiding this comment

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

s/cluster/cluster./

Copy link
Contributor

Choose a reason for hiding this comment

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

pods

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you delete the SpecialResource? Do you delete the simple-kmod-local.yaml file? If so, maybe:

To remove the simple-kmod kernel module from the node, delete the simple-kmod-local.yaml file that contains the SpecialResource specification. The kernel module is unloaded when the driver container pod is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this to be more clear.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2021
Copy link
Contributor

@kmccarron-rh kmccarron-rh left a comment

Choose a reason for hiding this comment

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

Here is a first round of edits.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended that you create the Namespace as detailed in the previous section.

@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2021
@dagrayvid
Copy link
Contributor Author

@kmccarron-rh thanks for all of the helpful feedback. I believe I implemented all the suggested changes.

Copy link
Contributor

@kmccarron-rh kmccarron-rh left a comment

Choose a reason for hiding this comment

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

Hi David, a couple of changes.

@dagrayvid dagrayvid force-pushed the add-sro branch 4 times, most recently from d37bb31 to ae685e2 Compare September 15, 2021 16:53
@dagrayvid
Copy link
Contributor Author

Thanks @kmccarron-rh, I implemented those changes and fixed the xref related build errors.

Copy link

@wabouhamad wabouhamad left a comment

Choose a reason for hiding this comment

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

/lgtm @liqcui please review

Copy link

@pacevedom pacevedom left a comment

Choose a reason for hiding this comment

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

Looking good!

Choose a reason for hiding this comment

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

Maybe mention something about SNO not having the image registry, and requiring enable or an external one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a pointer to the additional resources in the prerequisites modules/special-resource-operator-using.adoc to make this more clear, but didn't add a specific mention of SNO because it's not specific to SNO. It also applies to baremetal installs, etc. Let me know if you think more clarification is needed

@kmccarron-rh
Copy link
Contributor

kmccarron-rh commented Sep 16, 2021

@maxwelldb maxwelldb self-requested a review September 17, 2021 13:21
@maxwelldb maxwelldb added peer-review-needed Signifies that the peer review team needs to review this PR branch/enterprise-4.9 labels Sep 17, 2021
@maxwelldb maxwelldb added this to the Future Release milestone Sep 17, 2021
Copy link
Contributor

@maxwelldb maxwelldb left a comment

Choose a reason for hiding this comment

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

Made some comments and suggestions.

There are mod docs structural issues in this PR that have to be addressed prior to merging.

I'd ping the review channel for another pass after restructuring.

Distros: openshift-origin,openshift-enterprise
Topics:
- Name: About hardware enablement on OpenShift
File: about-hardware-enablement
Copy link
Contributor

Choose a reason for hiding this comment

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

To verify: no need for Distros here?


toc::[]

Many applications require specialized hardware or software which depend on kernel modules or drivers. The recommended approach to load out-of-tree kernel modules on {op-system-first} nodes is to use driver containers. To deploy out-of-tree drivers at the same time as cluster installation (day-1) the kmods-via-containers framework can be used. For loading drivers or kernel modules on an existing {product-title} cluster (day-2) {product-title} offers several tools:
Copy link
Contributor

Choose a reason for hiding this comment

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

ISG ⚠️

Suggested change
Many applications require specialized hardware or software which depend on kernel modules or drivers. The recommended approach to load out-of-tree kernel modules on {op-system-first} nodes is to use driver containers. To deploy out-of-tree drivers at the same time as cluster installation (day-1) the kmods-via-containers framework can be used. For loading drivers or kernel modules on an existing {product-title} cluster (day-2) {product-title} offers several tools:
Many applications require specialized hardware or software that depends on kernel modules or drivers. The recommended approach to load out-of-tree kernel modules on {op-system-first} nodes is to use driver containers. To deploy out-of-tree drivers at the same time as cluster installation (day 1), use the kmods-via-containers framework. To load drivers or kernel modules on an existing {product-title} cluster (day 2), {product-title} offers several tools:

I would think about ways to rephrase or eliminate "day 1" and "day 2." We don't use those widely in customer-facing docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

"The recommended approach..." is passive. Could restructure like:

"You can use driver containers to load out-of-tree kernel modules..."

(Just as an example.)

Similar idea for the sentence after it.


Many applications require specialized hardware or software which depend on kernel modules or drivers. The recommended approach to load out-of-tree kernel modules on {op-system-first} nodes is to use driver containers. To deploy out-of-tree drivers at the same time as cluster installation (day-1) the kmods-via-containers framework can be used. For loading drivers or kernel modules on an existing {product-title} cluster (day-2) {product-title} offers several tools:

* The Driver Toolkit is a container image which is a part of every OpenShift release. It contains the kernel packages and other common dependencies needed for building a driver or kernel module. The Driver Toolkit can be used as a base image for driver container image builds on the {product-title}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The Driver Toolkit is a container image which is a part of every OpenShift release. It contains the kernel packages and other common dependencies needed for building a driver or kernel module. The Driver Toolkit can be used as a base image for driver container image builds on the {product-title}.
* The Driver Toolkit is a container image that is part of every OpenShift release. It contains the kernel packages and other common dependencies that are needed to build a driver or kernel module. The Driver Toolkit can be used as a base image for driver container image builds on the {product-title}.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure about the capitalization of "Driver Toolkit." Is this a branding decision?

Many applications require specialized hardware or software which depend on kernel modules or drivers. The recommended approach to load out-of-tree kernel modules on {op-system-first} nodes is to use driver containers. To deploy out-of-tree drivers at the same time as cluster installation (day-1) the kmods-via-containers framework can be used. For loading drivers or kernel modules on an existing {product-title} cluster (day-2) {product-title} offers several tools:

* The Driver Toolkit is a container image which is a part of every OpenShift release. It contains the kernel packages and other common dependencies needed for building a driver or kernel module. The Driver Toolkit can be used as a base image for driver container image builds on the {product-title}.
* The Special Resource Operator (SRO) orchestrates the building and management of driver containers for loading kernel modules and drivers on an existing (day 2) OpenShift or Kubernetes cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The Special Resource Operator (SRO) orchestrates the building and management of driver containers for loading kernel modules and drivers on an existing (day 2) OpenShift or Kubernetes cluster.
* The Special Resource Operator (SRO) orchestrates the building and management of driver containers to load kernel modules and drivers on an existing (day 2) OpenShift or Kubernetes cluster.


* Obtain the image pull secret needed to perform an installation of {product-title}, from the link:https://console.redhat.com/openshift/install/pull-secret[Pull Secret] page on the {cloud-redhat-com} site.
* Install the OpenShift CLI (`oc`).
* You have obtained the image pull secret needed to perform an installation of {product-title}, from the link:https://cloud.redhat.com/openshift/install/pull-secret[Pull Secret] page on the {cloud-redhat-com} site.
Copy link
Contributor

Choose a reason for hiding this comment

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

ISG recommends simple present tense as much as possible. Simple past if necessary.

Ex: "You have the image..."/"You obtained the image..."

+
[source,terminal]
----
mkdir -p chart/simple-kmod-0.0.1/templates
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mkdir -p chart/simple-kmod-0.0.1/templates
$ mkdir -p chart/simple-kmod-0.0.1/templates

All command line sections should start with $ or #.

----


. Use the following SpecialResource manifest to deploy the simple-kmod using the Helm chart that you created in the ConfigMap. The `spec.chart.repository.url` field tells SRO to look for the chart in a ConfigMap. Optionally, uncomment the #debug: true line, to have the YAML files in the chart printed in full in the operator logs and to verify that the logs are created and templated properly. Save this YAML as `simple-kmod-configmap.yaml`
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use callouts to simplify this step.

uri: "https://github.com/openshift-psap/kvc-simple-kmod.git"
----

. Create the SpecialResource with:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Create the SpecialResource with:
. Create the `SpecialResource` [resource]:

Watch for this construction elsewhere.

$ oc create -f simple-kmod-configmap.yaml
----

. The simple-kmod resources are deployed in the namespace `simple-kmod` as specified in the object manifest. After a short time, the build pod for the simple-kmod driver container should start running. After a few minutes, the build should complete and the driver container pods should start Running.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the instruction here?

simple_kmod 16384 0
----

To remove the simple-kmod kernel module from the node, delete the simple-kmod SpecialResource API object with `oc delete`. The kernel module is unloaded when the driver container pod is deleted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a note rather than a LI?

@maxwelldb maxwelldb 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 Sep 17, 2021
@kmccarron-rh
Copy link
Contributor

#36674 -Suggested doc changes are implemented in this PR.

@vikram-redhat
Copy link
Contributor

@kmccarron-rh I am assuming this can now be closed. I will go ahead and close it, but please reach out if that is an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants