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

Memory QoS example plugin #115

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Conversation

askervin
Copy link
Collaborator

No description provided.

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.

Thanks @askervin for the the new plugin!

I didn't look at the code yet but a few notes outside of that:

  • could you integrate the documentation in docs/ instead of a separate readme file
  • deployment files are missing
    • Helm
    • kustomize

@askervin askervin force-pushed the 5Qw_memory_qos_example branch 2 times, most recently from 40e0c94 to 659ef33 Compare September 27, 2023 14:05
@askervin
Copy link
Collaborator Author

doc separated, kustomize added. @fmuyassarov is helping with help charts. I'd suggest merging this before the memtierd-nri plugin, so that I can rebase the memtierd-nri plugin on top of this and connect its documentation properly to the "memory plugins" document section that is introduced in this patch.

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.

go.mod needs updating/tidying.

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.

Quick comments below

cmd/plugins/memory-qos/test-pod.yaml Outdated Show resolved Hide resolved
cmd/plugins/memory-qos/sample-config.yaml Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
test/e2e/files/nri-memory-qos-deployment.yaml.in Outdated Show resolved Hide resolved
docs/memory/memory-qos.md Outdated Show resolved Hide resolved
docs/memory/memory-qos.md Outdated Show resolved Hide resolved
docs/memory/memory-qos.md Outdated Show resolved Hide resolved
cmd/plugins/memory-qos/main.go Outdated Show resolved Hide resolved
cmd/plugins/memory-qos/main.go Show resolved Hide resolved
@askervin askervin force-pushed the 5Qw_memory_qos_example branch 3 times, most recently from 2d85fc8 to 3adb872 Compare September 29, 2023 12:03
@askervin
Copy link
Collaborator Author

@klihub , @marquiz , big thanks on all these findings! I'll apply them and other next to PR #117. In my perspective this PR is now ready for merge. I can finalize PR #117 (link the documentation properly) once this PR is merged.

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.

Thanks @askervin
LGTM

@klihub
Copy link
Collaborator

klihub commented Sep 29, 2023

@askervin Do you lack a sign-off in some commits ? The DCO check fails.

Signed-off-by: Antti Kervinen <[email protected]>
@klihub klihub merged commit 246e1f7 into containers:main Sep 29, 2023
2 checks passed
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.

3 participants