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

Remove reference to deprecated allow-privileged flag for kubelet #55

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

rptaylor
Copy link
Contributor

@rptaylor rptaylor commented Apr 3, 2024

The --allow-privileged flag does not exist for kubelet:
https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/

It was removed: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.15.md#node

However I still don't understand why privileged pods are only required if a daemonset is used. How else can the plugin be deployed besides a daemonset, and how does it work when it is unprivileged?

This also fixes some minor language issues.

@rptaylor
Copy link
Contributor Author

rptaylor commented Apr 3, 2024

Or does it really mean that privileged mode is only required when health checks are run by the daemonset?

@y2kenny
Copy link
Contributor

y2kenny commented Apr 5, 2024

Thanks for the clean up @rptaylor, it is much appreciated. I wrote this long time ago and I probably had the impression (likely incorrect) that privileged container for daemonset is somehow different from privileged container in a regular pod.

And yes, I meant to say privileged mode is needed with health check because of this:
https://github.com/ROCm/k8s-device-plugin/blob/master/cmd/k8s-device-plugin/main.go#L104

The device plugin (without health check) usually doesn't need to be privileged because it just does the allocation without accessing the hw itself. I might need to rethink how I do health check.

I would be happy to merge this as is or if you want to further rephrase base on what I wrote here I would be happy to wait as well. Please let me know.

@rptaylor
Copy link
Contributor Author

rptaylor commented Apr 5, 2024

@y2kenny Updated, ready to go.
Could you also have a look at #29 (comment) and #57 if you get a chance? Thanks!

@y2kenny-amd y2kenny-amd merged commit 0a820b8 into ROCm:master Apr 5, 2024
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