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

docs: rework development guide #1126

Merged
merged 1 commit into from
Sep 2, 2022
Merged

docs: rework development guide #1126

merged 1 commit into from
Sep 2, 2022

Conversation

mythi
Copy link
Contributor

@mythi mythi commented Aug 31, 2022

Currently, each individual plugin README documents roughly the same
daily development steps to git clone, build, and deploy. Re-purpose
the plugin READMEs more towards cluster admin type of documentation
and start moving all development related documentation to DEVEL.md.

The same is true for e2e testing documentation which is scattered
in places where they don't belong to. Having all day-to-day
development Howtos is good to have in a centralized place.

Finally, the cleanup includes some harmonization to plugins'
table of contents which now follows the pattern:

Signed-off-by: Mikko Ylinen [email protected]

@mythi
Copy link
Contributor Author

mythi commented Aug 31, 2022

/cc @eero-t @msivosuo @skaajas

DEVEL.md Outdated Show resolved Hide resolved
DEVEL.md Show resolved Hide resolved
README.md Show resolved Hide resolved
cmd/dlb_plugin/README.md Outdated Show resolved Hide resolved
cmd/fpga_plugin/README.md Outdated Show resolved Hide resolved
cmd/gpu_plugin/README.md Show resolved Hide resolved
cmd/qat_plugin/README.md Show resolved Hide resolved
cmd/qat_plugin/README.md Show resolved Hide resolved
cmd/sgx_plugin/README.md Show resolved Hide resolved
cmd/vpu_plugin/README.md Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1126 (de9d387) into main (ba99896) will not change coverage.
The diff coverage is n/a.

❗ Current head de9d387 differs from pull request most recent head fe0aa30. Consider uploading reports for the commit fe0aa30 to get more accurate results

@@           Coverage Diff           @@
##             main    #1126   +/-   ##
=======================================
  Coverage   52.99%   52.99%           
=======================================
  Files          40       40           
  Lines        4357     4357           
=======================================
  Hits         2309     2309           
  Misses       1921     1921           
  Partials      127      127           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Currently, each individual plugin README documents roughly the same
daily development steps to git clone, build, and deploy. Re-purpose
the plugin READMEs more towards cluster admin type of documentation
and start moving all development related documentation to DEVEL.md.

The same is true for e2e testing documentation which is scattered
in places where they don't belong to. Having all day-to-day
development Howtos is good to have in a centralized place.

Finally, the cleanup includes some harmonization to plugins'
table of contents which now follows the pattern:

* [Introduction](#introduction)
(* [Modes and Configuration Options](#modes-and-configuration-options))
* [Installation](#installation)
    (* [Prerequisites](#prerequisites))
    * [Pre-built Images](#pre-built-images)
    * [Verify Plugin Registration](#verify-plugin-registration)
* [Testing and Demos](#testing-and-demos)
    * ...

Signed-off-by: Mikko Ylinen <[email protected]>
@mythi
Copy link
Contributor Author

mythi commented Sep 2, 2022

@eero-t @bart0sh can you approve if no further changes are requested. I'd prefer to have this merged (I can handle the scripts change #1131 afterwards)

Copy link
Member

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

@eero-t
Copy link
Contributor

eero-t commented Sep 2, 2022

/lgtm

@eero-t
Copy link
Contributor

eero-t commented Sep 2, 2022

Summary of items discussed above which were scoped out to further PRs:

  • upgrade to k8s 1.25.0 #1131:
    • Add module dep script to scripts/
  • Project Documentation Improvements #1045:
    • Split main README prerequisites for building & running into separate sections
    • Document container managers config options for whether container device access rights are inherited from the target node host, or from pod security context
    • Add operator instructions to all plugin docs

Copy link
Contributor

@tkatila tkatila left a comment

Choose a reason for hiding this comment

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

lgtm

@bart0sh
Copy link
Member

bart0sh commented Sep 2, 2022

@mythi github doesn't allow me to merge this PR. Feel free to merge it.

Copy link
Contributor

@ozhuraki ozhuraki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hj-johannes-lee hj-johannes-lee left a comment

Choose a reason for hiding this comment

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

LGTM, but..
nitpick: Do we capitalize also some other parts which are not sentences.?

@@ -37,7 +30,7 @@ The components together implement the following features:
- orchestration of FPGA programming
- access control for FPGA hardware

## Component overview
### Component overview
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me fix these in a separate PR

@@ -70,7 +63,7 @@ Kubernetes:
The repository also contains an [FPGA helper tool](../fpga_tool/README.md) that may be useful during
development, initial deployment and debugging.

## FPGA modes
### Modes and Configuration options
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?


The below sections cover `DPDK` and `OpenSSL` demos, both of which utilise the
QAT device plugin under Kubernetes.

#### DPDK QAT demos
### DPDK QAT demos
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?

@@ -289,7 +169,7 @@ $ kubectl get pods
> **Note**: If the `igb_uio` VF driver is used with the QAT device plugin,
> the workload be deployed with `SYS_ADMIN` capabilities added.

##### Manual test run
#### Manual test run
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?

$ kubectl logs qat-dpdk-test-crypto-perf-tc1
$ kubectl logs qat-dpdk-test-compress-perf-tc1
```

> **Note**: for `test-crypto1` and `test-compress1` to work, the cluster must enable
[Kubernetes CPU manager's](https://kubernetes.io/docs/tasks/administer-cluster/cpu-management-policies/) `static` policy.

#### OpenSSL QAT demo
### OpenSSL QAT demo
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?

@@ -306,22 +186,22 @@ $ dpdk-test-crypto-perf -l 6-7 -w $QAT1 \

> **Note**: Adapt the `.so` versions to what the DPDK version in the container provides.

##### Automated test run
#### Automated test run
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we capitalize all of these?

@mythi
Copy link
Contributor Author

mythi commented Sep 2, 2022

@mythi github doesn't allow me to merge this PR. Feel free to merge it.

@bart0sh it should work now

Copy link
Contributor

@uniemimu uniemimu left a comment

Choose a reason for hiding this comment

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

lgtm

@bart0sh bart0sh merged commit f0dd952 into intel:main Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants