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

deployment: refactor config manager to support NRI enabling in CRI-O #120

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

fmuyassarov
Copy link
Collaborator

@fmuyassarov fmuyassarov commented Sep 28, 2023

This PR extends config manager code and the plugins helm charts
so that CRI-O users are also able to enable NRI via our charts if they
wish to. Same parameter is used to opt in for the feature in Helm
charts and we don't require users to indicate what container runtime
is being used. Instead the config manager auto-detects the runtime
and does the necessary changes to its configuration file. In scenarios
with multiple active runtimes (e.g., CRI-O and containerd), the
manager gracefully exits and throws an error.

@fmuyassarov fmuyassarov force-pushed the crio-nri branch 2 times, most recently from 18984c0 to 996e2d4 Compare September 29, 2023 07:31
cmd/config-manager/main.go Outdated Show resolved Hide resolved
@kad
Copy link
Collaborator

kad commented Sep 29, 2023

just as an idea: would it make sense to try to autodetect?

@fmuyassarov fmuyassarov marked this pull request as ready for review October 3, 2023 22:02
@fmuyassarov fmuyassarov changed the title Add an option to activate NRI in CRI-O deployment: refactor config manager to support NRI enabling in CRI-O Oct 3, 2023
@fmuyassarov fmuyassarov requested review from klihub, kad and marquiz October 3, 2023 22:04
@fmuyassarov
Copy link
Collaborator Author

just as an idea: would it make sense to try to autodetect?

Done. PTAL

@fmuyassarov fmuyassarov force-pushed the crio-nri branch 2 times, most recently from 4f760f8 to 1f639c0 Compare October 4, 2023 09:23
@fmuyassarov
Copy link
Collaborator Author

Rebased on top of the latest main.

cmd/config-manager/main.go Outdated Show resolved Hide resolved
cmd/config-manager/main.go Outdated Show resolved Hide resolved
@fmuyassarov
Copy link
Collaborator Author

@klihub @marquiz PTAL, I have updated the patch now.

cmd/config-manager/main.go Outdated Show resolved Hide resolved
@fmuyassarov
Copy link
Collaborator Author

cc @marquiz

@fmuyassarov
Copy link
Collaborator Author

Rebased.

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.

Thx @fmuyassarov. A few small comments below

cmd/config-manager/main.go Outdated Show resolved Hide resolved
cmd/config-manager/main.go Outdated Show resolved Hide resolved
cmd/config-manager/main.go Outdated Show resolved Hide resolved
deployment/helm/balloons/templates/daemonset.yaml Outdated Show resolved Hide resolved
docs/resource-policy/installation.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Few questions.

deployment/helm/topology-aware/values.yaml Outdated Show resolved Hide resolved
deployment/helm/balloons/values.yaml Outdated Show resolved Hide resolved
deployment/helm/balloons/templates/daemonset.yaml Outdated Show resolved Hide resolved
This commit extends config manager code and the plugins helm charts
so that CRI-O users are also able to enable NRI via our charts if they
wish to. Same parameter is used to opt in for the feature in Helm
charts and we don't require users to indicate what container runtime
is being used. Instead the config manager auto-detects the runtime
and does the necessary changes to its configuration file. In scenarios
with multiple active runtimes (e.g., CRI-O and containerd), the
manager gracefully exits and throws an error.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
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

But should we have the same thing for memtierd and memory-qos plugins (and the example plugin)?

@fmuyassarov
Copy link
Collaborator Author

LGTM

But should we have the same thing for memtierd and memory-qos plugins (and the example plugin)?

I'm +1 for that. @kad @klihub @askervin thoughts?

@fmuyassarov
Copy link
Collaborator Author

LGTM

But should we have the same thing for memtierd and memory-qos plugins (and the example plugin)?

Regardless of that, can we perhaps proceeding merging this PR if it looks good enough. And for adding init container to memtierd and memory-qos can be done as separate PR?

@klihub
Copy link
Collaborator

klihub commented Oct 11, 2023

LGTM
But should we have the same thing for memtierd and memory-qos plugins (and the example plugin)?

I'm +1 for that. @kad @klihub @askervin thoughts?

I think it would be more consistent repo-behavior if all the helm charts would allow one to also enable NRI if necessary during deployment.

@marquiz marquiz merged commit 05b42b8 into containers:main Oct 11, 2023
3 checks passed
@marquiz
Copy link
Collaborator

marquiz commented Oct 11, 2023

And for adding init container to memtierd and memory-qos can be done as separate PR?

Let's do that.

@fmuyassarov fmuyassarov deleted the crio-nri branch October 11, 2023 08:42
@fmuyassarov
Copy link
Collaborator Author

submitted #131 for that

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