[Proposal] Add cgroups `burst` support

[TonyxSun]

OCI runtime-specs had added support for the burst cgroups configuration (ref).

Propose to add support in NRI to allow greater range of container adjustments and updates.

Similarly, the cpu.idle parameter is not supported either and can be added.

Can work on this as a first issue.

[klihub]

Similarly, the cpu.idle parameter is not supported either and can be added.

@TonyxSun Are you sure, the cpu.idle parameter cannot be adjusted/set using the cgroupv2 unified map parameter ?

[TonyxSun]

Thanks for pointing that out, didn't notice its existence. cpu.max.burst can similarly be set using the unified map, any benefit to having it be supported as a first class field? (only one i can think of is better support for cgroups v1).

Might be good to add mention of this in https://github.com/containerd/nri/blob/main/README.md?plain=1#L221?

[klihub]

Thanks for pointing that out, didn't notice its existence. cpu.max.burst can similarly be set using the unified map, any benefit to having it be supported as a first class field? (only one i can think of is better support for cgroups v1).

Well, on the wire protocol there is no point in adding dedicated fields. But we could consider adding some reasonable coherent set of dedicated receiver functions for pkg/api.ContainerAdjustment, to allow a more natural way for setting v2 cpu.* controller fields than the 'unified' map (or the runc/crun v1-to-v2 conversion). If we decide to do so we have to be careful though, as it would introduce ambiguity by providing multiple ways for setting many of the cpu-related conteoller fields.

Might be good to add mention of this in https://github.com/containerd/nri/blob/main/README.md?plain=1#L221?

Definitely. And maybe an example in the form of a sample plugin...

[TonyxSun]

If we decide to do so we have to be careful though, as it would introduce ambiguity by providing multiple ways for setting many of the cpu-related conteoller fields.

It could be helpful to provide receiver fns for v2 fields with no v1 translation. Maybe we can revisit as needs come up.

One gap for my use case - I might need to support setting the cfs_burst_us v1 field. There isn't a way to do this right now, correct?

And maybe an example in the form of a sample plugin...
Noted 👍🏼

[klihub]

One gap for my use case - I might need to support setting the cfs_burst_us v1 field. There isn't a way to do this right now, correct?

Yes, I think there is no way to do that ATM, especially so if the OCI Spec does not support that v1 field (need to check, can't recall).

[egorikas]

Well, on the wire protocol there is no point in adding dedicated fields. But we could consider adding some reasonable coherent set of dedicated receiver functions for pkg/api.ContainerAdjustment, to allow a more natural way for setting v2 cpu.* controller fields than the 'unified' map (or the runc/crun v1-to-v2 conversion). If we decide to do so we have to be careful though, as it would introduce ambiguity by providing multiple ways for setting many of the cpu-related conteoller fields.

Hey folks! Uber does small steps in NRI-adoption nowadays and one of our NRI-plugins does cfs_burst activation for some pods in out k8s fleet.

While the current implementation gives a lot of freedom and flexibility. It would be lovely to have a set of "helper" methods to tweak some of the common parameters.

And not having multilevel constructor thing :)

	adjustment := &api.ContainerAdjustment{
		Linux: &api.LinuxContainerAdjustment{
			Resources: &api.LinuxResources{
				Unified: map[string]string{
					"cpu.max.burst": XXXX,
				},
			},
		},
	}

[klihub]

Well, on the wire protocol there is no point in adding dedicated fields. But we could consider adding some reasonable coherent set of dedicated receiver functions for pkg/api.ContainerAdjustment, to allow a more natural way for setting v2 cpu.* controller fields than the 'unified' map (or the runc/crun v1-to-v2 conversion). If we decide to do so we have to be careful though, as it would introduce ambiguity by providing multiple ways for setting many of the cpu-related conteoller fields.

Hey folks! Uber does small steps in NRI-adoption nowadays and one of our NRI-plugins does cfs_burst activation for some pods in out k8s fleet.

While the current implementation gives a lot of freedom and flexibility. It would be lovely to have a set of "helper" methods to tweak some of the common parameters.

And not having multilevel constructor thing :)

	adjustment := &api.ContainerAdjustment{
		Linux: &api.LinuxContainerAdjustment{
			Resources: &api.LinuxResources{
				Unified: map[string]string{
					"cpu.max.burst": XXXX,
				},
			},
		},
	}

Well, at least for manipulating the unified cgroup attribute set we do have an extra ContainerAdjustment.AddLinuxUnified(key, value string) defined there ?

With that you can do something like

    const (
        MaxCpuBurst = "cpu.max.burst"
    )

    adjustment := &api.ContainerAdjustment{}
    ...
    adjustment.AddLinuxUnified(MaxCpuBurst, "XXXX")
    ...

which is already slightly better, I think. @egorikas Is this something you had in mind, or were you thinking of some higher-level (and more domain-specific) abstraction ?

[egorikas]

Well, at least for manipulating the unified cgroup attribute set we do have an extra ContainerAdjustment.AddLinuxUnified(key, value string) defined there ?

With that you can do something like

    const (
        MaxCpuBurst = "cpu.max.burst"
    )

    adjustment := &api.ContainerAdjustment{}
    ...
    adjustment.AddLinuxUnified(MaxCpuBurst, "XXXX")
    ...

which is already slightly better, I think. @egorikas Is this something you had in mind, or were you thinking of some higher-level (and more domain-specific) abstraction ?

@klihub Thanks for the quick response! I completely missed that method.

My thinking was more about having specific helpers like adjustment.SetCpu Max, etc., to provide a more fine-grained API for cgroup modifications. In my mind, it should make the API-usage easier.