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

Resource policy documentation #6

Merged
merged 7 commits into from
Jun 12, 2023
Merged

Resource policy documentation #6

merged 7 commits into from
Jun 12, 2023

Conversation

jukkar
Copy link
Collaborator

@jukkar jukkar commented Mar 23, 2023

Documentation from cri-rm updated for nri resource policy.

@jukkar jukkar requested a review from klihub March 23, 2023 14:27
@jukkar jukkar force-pushed the docs branch 4 times, most recently from c79cd8f to 45ba643 Compare March 28, 2023 11:16
@jukkar jukkar changed the title [WIP] Resource policy documentation Resource policy documentation Apr 24, 2023
@jukkar jukkar marked this pull request as ready for review April 24, 2023 13:21
@jukkar
Copy link
Collaborator Author

jukkar commented May 31, 2023

ping @askervin, @marquiz, @kad, @klihub

Lets try to review this by the end of the week so we can get this merged sooner than later, so please take a look at these.

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

A few comments 😇

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docs/Dockerfile Outdated Show resolved Hide resolved

## Setting up NRI Resource Policy

### Using NRI Resource Policy Agent and a ConfigMap
Copy link
Collaborator

Choose a reason for hiding this comment

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

This stuff could be moved to the node-agent/dynamic configuration document, if not already covered there(?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest we revisit all the setup, quick-start and relevant docs after the helm chart support is merged which is probably quite soon.

Comment on lines +3 to +12
When you want to try NRI Resource Policy, here is the list of things
you need to do, assuming you already have a Kubernetes\* cluster up and
running, using either `containerd` or `cri-o` as the runtime.

* [Install](installation.md) NRI Resource Policy DaemonSet deployment file.
* Runtime (containerd / cri-o) configuration

For NRI Resource Policy, you need to provide a configuration file. The default
configuration ConfigMap file can be found in the DaemonSet deployment yaml file.
You can edit it as needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this info should be part of the "Installation" doc, if not already covered there

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file somehow feels overlapping/redundant at the moment. More comments below

See the [Node Agent][agent] about how to set up and configure the agent.


## Logging and debugging
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this file should be renamed logging and debugging. Or then move also this section under the agent/configuration doc

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

A few comments 😇

Add documentation files and makefile rules to generate html
documentation for the project.

This is taken from cri-resource-manager and HEAD was at
commit 5db1e8a5fc0ec15780f5ffa6fbb2a9d826cb221b

Signed-off-by: Jukka Rissanen <[email protected]>
The heading level was wrong and was reported during doc
generation.

Signed-off-by: Jukka Rissanen <[email protected]>
The sample configuration files are referenced by the documentation
so we need to have them here.

Signed-off-by: Jukka Rissanen <[email protected]>
User can say "run.sh help" to get information about the
functions available.

Signed-off-by: Jukka Rissanen <[email protected]>
This is just a raw conversion and fixes so that "make docs"
will pass and generate the documentation. Subsequent commits
will do more changes to the actual content.

Signed-off-by: Jukka Rissanen <[email protected]>
Changing the documentation so that it will describe how the
nri-resource-policy plugin works.

Also reshuffling the files around to better fit the overall
documentation structure.

Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar
Copy link
Collaborator Author

jukkar commented Jun 6, 2023

A few comments

Thanks, I updated most of the documentation. The installation/setup/quick-start needs still some work but that needs to wait the #45.

@klihub
Copy link
Collaborator

klihub commented Jun 6, 2023

A few comments innocent

A big thanks for the thorough review !

Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this and addressing the remaining comment and doing other restructuring with further PRs. How about others ? @marquiz @kad @fmuyassarov @askervin ?

@marquiz
Copy link
Collaborator

marquiz commented Jun 6, 2023

Thinking from the users perspective, IMO the one thing that needs to be in reasonable shape are the deployment/installation instructions. Easy-to-follow step-by-step insrtuctions how to get the plugins running in a cluster. Something like

IMAGE_REPO=my-custom-repo
make image
make image-push
kubectl apply -k deployment/overlays/topology-aware

Another concern/question: how to track the unresolved comments if this gets merged?

@fmuyassarov
Copy link
Collaborator

Thinking from the users perspective, IMO the one thing that needs to be in reasonable shape are the deployment/installation instructions. Easy-to-follow step-by-step insrtuctions how to get the plugins running in a cluster. Something like

IMAGE_REPO=my-custom-repo
make image
make image-push
kubectl apply -k deployment/overlays/topology-aware

Another concern/question: how to track the unresolved comments if this gets merged?

I'm going to submit another documentation RP which will include that and other ways of installing the plugins.

@marquiz
Copy link
Collaborator

marquiz commented Jun 8, 2023

I'm going to submit another documentation RP which will include that and other ways of installing the plugins.

That's cool. I suggest to drop all the installation/quick start stuff from this PR, then. WDYT?

These are to be rewritten by a Helm chart support commits
coming in near future.

Signed-off-by: Jukka Rissanen <[email protected]>
@marquiz
Copy link
Collaborator

marquiz commented Jun 9, 2023

I think we could merge this. If we do, @jukkar could you create an issue tracking the unresolved comments?

@jukkar
Copy link
Collaborator Author

jukkar commented Jun 9, 2023

create an issue tracking the unresolved comments?

Created #58 for tracking pending work.

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub merged commit 4a40df1 into containers:main Jun 12, 2023
@jukkar jukkar deleted the docs branch June 19, 2023 11:21
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.

5 participants