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

Support pid cgroup control in plugin #75

Closed
yylt opened this issue Mar 29, 2024 · 5 comments · Fixed by #76
Closed

Support pid cgroup control in plugin #75

yylt opened this issue Mar 29, 2024 · 5 comments · Fixed by #76
Milestone

Comments

@yylt
Copy link
Contributor

yylt commented Mar 29, 2024

In the current implementation, it is not possible to control pid cgroup in nri plugin. Could pid be included?

// Container (linux) resources.
type LinuxResources struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Memory         *LinuxMemory         `protobuf:"bytes,1,opt,name=memory,proto3" json:"memory,omitempty"`
	Cpu            *LinuxCPU            `protobuf:"bytes,2,opt,name=cpu,proto3" json:"cpu,omitempty"`
	HugepageLimits []*HugepageLimit     `protobuf:"bytes,3,rep,name=hugepage_limits,json=hugepageLimits,proto3" json:"hugepage_limits,omitempty"`
	BlockioClass   *OptionalString      `protobuf:"bytes,4,opt,name=blockio_class,json=blockioClass,proto3" json:"blockio_class,omitempty"`
	RdtClass       *OptionalString      `protobuf:"bytes,5,opt,name=rdt_class,json=rdtClass,proto3" json:"rdt_class,omitempty"`
	Unified        map[string]string    `protobuf:"bytes,6,rep,name=unified,proto3" json:"unified,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
	Devices        []*LinuxDeviceCgroup `protobuf:"bytes,7,rep,name=devices,proto3" json:"devices,omitempty"` // for NRI v1 emulation
}

Reference: https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#pids

@bitmingw
Copy link

bitmingw commented Apr 4, 2024

pid is here

uint32 pid = 12; // for NRI v1 emulation

But it says "for NRI v1 emulation". I don't know if it is actually provided.

@yylt
Copy link
Contributor Author

yylt commented Apr 7, 2024

pid is here

uint32 pid = 12; // for NRI v1 emulation

But it says "for NRI v1 emulation". I don't know if it is actually provided.

If I'm not mistaken, this seems to be metadata for a sandbox or container, it cannot be used for ContainerAdjustment.

@bitmingw
Copy link

bitmingw commented Apr 7, 2024

This value, if provided, should be read only. I don't think there is a way to control which pid to assign to a process.

@yylt
Copy link
Contributor Author

yylt commented Apr 7, 2024

Yes, so what I mean is to add pids in the LinuxResources for controlling the pids cgroup.

Additionally, the pids cgroup is to control the number of PIDs inside the container, which is a good restriction limit for the issue of unhandled process exit signals or starting too many processes .

@yylt yylt mentioned this issue Apr 8, 2024
@klihub
Copy link
Member

klihub commented Apr 8, 2024

In the current implementation, it is not possible to control pid cgroup in nri plugin. Could pid be included?

I think for the cgroup v2 version of the controller we already support setting the max allowed pids using LinuxResources.Unified["pids.max"]. Do you have a use case for this cgroup v1-specific addition ?

@mikebrow mikebrow added this to the 1.0 milestone Aug 1, 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 a pull request may close this issue.

4 participants