Fix: display online_cpus in compat REST API #15867

boaz0 · 2022-09-20T08:41:43Z

Does this PR introduce a user-facing change?

None

zhangguanzhang · 2022-09-20T09:56:21Z

PR does not include changes in the 'tests' directory

vrothberg

Thanks! Please add a test and a description of what the change fixes.

baude · 2022-09-27T12:17:22Z

@boaz0 could you please add a test for your PR? It is failing because no test was added.

baude · 2022-09-27T12:18:25Z

and also a proper commit message

boaz0 · 2022-09-27T18:30:42Z

Sure, I'll be working on it today and tomorrow. Sorry for the late response.

boaz0 · 2022-09-29T19:05:48Z

@edsantiago can you review the test I added.

I wasn't sure if to create a new bats file only for this use case
Maybe there are better ways to test this.

edsantiago · 2022-09-29T19:11:39Z

@boaz0 this looks like it belongs in test/apiv2; see the README.md file there, and other *.at files for examples.

There's a big red kind/api-change button on this PR.

edsantiago · 2022-10-06T16:05:04Z

@containers/podman-maintainers CI is green. Test LGTM (fails on main, passes with this PR). I will remind everyone that this has a big red api-change label.

ashley-cui

Looks like online_cpus are now displayed! I don't know how to confirm that the number is correct though, since it looks like it always displays 0?

@benoitf does this look correct? Do we have an example where we expect online_cpus to be non-zero?

curl --unix-socket /run/user/1000/podman/podman.sock http:/v1.41/containers/$myContainer/stats?stream=false | jq .cpu_stats
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1105    0  1105    0     0  73829      0 --:--:-- --:--:-- --:--:-- 78928
{
  "cpu_usage": {
    "total_usage": 67664000,
    "usage_in_kernelmode": 50612,
    "usage_in_usermode": 67613388
  },
  "system_cpu_usage": 53967013582000,
  "online_cpus": 0,
  "cpu": 0,
  "throttling_data": {
    "periods": 0,
    "throttled_periods": 0,
    "throttled_time": 0
  }
}

rhatdan · 2022-10-06T23:28:22Z

@Luap99 @mheon @ashley-cui Does this break API compatibility?

benoitf · 2022-10-07T06:47:24Z

@ashley-cui it should return the number of CPUs of the machine for example

Here is the output with docker if the 'docker vm' has 3 CPUs

curl --silent --unix-socket /var/run/docker.sock "http:/v1.41/containers/$myContainer/stats?stream=false"  | jq .cpu_stats
{
  "cpu_usage": {
    "total_usage": 126467000,
    "usage_in_kernelmode": 72702000,
    "usage_in_usermode": 53764000
  },
  "system_cpu_usage": 90890360000000,
  "online_cpus": 3,
  "throttling_data": {
    "periods": 0,
    "throttled_periods": 0,
    "throttled_time": 0
  }
}

"online_cpus": 3,

by default, podman machine have 2 cpus assigned so it should return 2 instead of 0

ashley-cui · 2022-10-07T13:31:52Z

@rhatdan It shouldn't, as this just shows online_cpus: 0 rather than hiding it if the CPUs is zero.

Though, if I understand @benoitf 's comment correctly, it looks like the CPU's should be non-zero, and we're still displaying 0 CPUs so I'm not sure if this PR correctly addresses the problem.

boaz0 · 2022-10-08T16:33:10Z

@benoitf @ashley-cui the number of CPUs is read from the cgroup file inside the container. I can look more and see what went wrong.

ashley-cui · 2022-10-10T13:39:45Z

Thanks @boaz0!

ashley-cui · 2022-10-17T20:55:37Z

@giuseppe Would you happen to know why the CPUs is always displayed as 0?

flouthoc · 2022-10-18T04:38:48Z

I think this might need cpuacct enabled but to calculate number of available CPU should we just parse cpu.max for nested cgroup container and do $MAX/$PERIOD where $MAX and $PERIOD are parsed from cpu.max. But @giuseppe can confirm this better.

lukasmrtvy · 2022-10-18T09:26:46Z

@flouthoc cpuacct controller isnt available for cgroups v2

flouthoc · 2022-10-18T09:38:09Z

@flouthoc cpuacct controller isnt available for cgroups v2

@lukasmrtvy Yes my suggestion for v2 and where cpuacct is not available was to compute from cpu.max i.e $MAX/$PERIOD but I could be wrong with this assumption so I'd wait for others to confirm.

rhatdan · 2022-10-18T13:45:53Z

@giuseppe PTAL ^^

giuseppe · 2022-10-19T08:24:00Z

Could we use sched_getaffinity(2) to calculate the number of online CPUs?

Something like:

package main

import (
	"fmt"
	"golang.org/x/sys/unix"
)

func getNumOnlineCPUs() (int, error) {
	var cpuSet unix.CPUSet
	if err := unix.SchedGetaffinity(0, &cpuSet); err != nil {
		return -1, err
	}
	return cpuSet.Count(), nil
}

func main(){
	n, err := getNumOnlineCPUs()
	if err != nil {
		panic(err)
	}
	fmt.Printf("Online CPUs: %d\n", n)
}

with the code above I have:

$ ./online_cpus 
Online CPUs: 8

$ taskset -c 1,2 ./online_cpus 
Online CPUs: 2

$ taskset -c 1 ./online_cpus 
Online CPUs: 1

It doesn't require any privilege to run

boaz0 · 2022-10-21T07:41:57Z

@giuseppe I am probably wrong but

./bin/podman run --cpus 2 busybox sh -c 'while true;do sleep 1s; echo "hello";done'

tells me I have 8 online_cpus, I was hoping to have 2.
I also tried

./bin/podman run --cpuset-cpus 0,3 busybox sh -c 'while true;do sleep 1s; echo "hello";done'

but got the same results.

This is the code:

func StatsContainer(w http.ResponseWriter, r *http.Request) {
...
		onlineCPUs, err := libpod.GetOnlineCPUs(0)
		if err != nil {
			logrus.Errorf("Unable to get online cpus: %v", err)
		}
}

func GetOnlineCPUs(pid uint64) (int, error) {
	var cpuSet unix.CPUSet
	if err := unix.SchedGetaffinity(int(pid), &cpuSet); err != nil {
		return -1, err
	}
	return cpuSet.Count(), nil
}
``

giuseppe · 2022-10-21T07:57:51Z

you'll need to replace the PID here: onlineCPUs, err := libpod.GetOnlineCPUs(0) from 0 to the PID of the container process

boaz0 · 2023-05-16T06:56:57Z

@rhatdan @vrothberg can you run CI? I did some changes, I hope it will make CI green again.
Thanks.

vrothberg · 2023-05-16T08:02:44Z

Tests aren't happy yet

boaz0 · 2023-05-16T09:44:40Z

@giuseppe I am trying to set --cpuset-cpus=0 for testing the API but it gives me this error message:

Error: OCI runtime error: crun: the requested cgroup controller `cpuset` is not available

Do you have an idea what I am doing wrong?

edsantiago · 2023-05-16T11:15:16Z

--cpuset requires root. See examples in 20-containers.at for running tests only when root.

boaz0 · 2023-05-18T11:57:35Z

Thanks @edsantiago

rhatdan · 2023-05-18T21:16:51Z

@giuseppe @vrothberg @TomSweeneyRedHat @mheon @edsantiago PTAL

edsantiago · 2023-05-18T21:21:10Z

LGTM but I'm not comfortable approving it. I did confirm that the new test fails on main (i.e. that it tests the new functionality).

libpod/stats_linux.go

mheon · 2023-05-18T22:33:46Z

One comment from me, otherwise LGTM

boaz0 · 2023-05-30T09:00:03Z

Thanks @mheon. Updated the code.

edsantiago · 2023-05-30T11:19:35Z

@boaz0 please squash your commits as a courtesy to future maintainers, thank you

Luap99

And as @edsantiago said please squash the commits.

Luap99 · 2023-05-30T11:28:35Z

pkg/api/handlers/compat/containers_stats.go

@@ -80,6 +80,10 @@ func StatsContainer(w http.ResponseWriter, r *http.Request) {
 			ThrottlingData: docker.ThrottlingData{},
 		}
 	}
+	onlineCPUs, err := libpod.GetOnlineCPUs(ctnr)
+	if err != nil {
+		utils.InternalServerError(w, err)


this needs a return after the InternalServerError call otherwise we just continue which will result in incorrect REST API responses.

Luap99 · 2023-05-30T11:29:46Z

libpod/stats_linux.go

+	if err != nil {
+		return -1, fmt.Errorf("failed to obtain Container %s PID: %w", container.Name(), err)
+	}


the err != nil branch should come before pid == 0

Luap99 · 2023-05-30T11:31:50Z

test/apiv2/19-stats.at

+#
+
+if root; then
+    podman pull $IMAGE &>/dev/null


the pull is not necessary, podman run will pull if required.

Signed-off-by: Boaz Shuster <[email protected]>

Luap99

LGTM

openshift-ci · 2023-05-31T08:54:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: boaz0, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TomSweeneyRedHat · 2023-05-31T17:50:46Z

LGTM

mheon · 2023-06-01T14:58:44Z

/lgtm

mheon · 2023-06-01T14:58:53Z

Thanks for the contribution @boaz0

openshift-ci bot added the release-note-none label Sep 20, 2022

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Sep 20, 2022

vrothberg reviewed Sep 20, 2022

View reviewed changes

boaz0 force-pushed the closes_15754 branch from 4ace8ae to 25f0865 Compare September 29, 2022 19:04

boaz0 force-pushed the closes_15754 branch from 25f0865 to 1ee9660 Compare September 29, 2022 19:10

boaz0 force-pushed the closes_15754 branch from 1ee9660 to f4ba8d2 Compare October 6, 2022 08:26

boaz0 requested a review from vrothberg October 6, 2022 08:26

boaz0 force-pushed the closes_15754 branch from f4ba8d2 to 43fc44e Compare October 6, 2022 09:11

ashley-cui reviewed Oct 6, 2022

View reviewed changes

edsantiago mentioned this pull request Oct 17, 2022

preserve-fds: file descriptor 4 is not available #15943

Closed

mheon reviewed May 18, 2023

View reviewed changes

libpod/stats_linux.go Show resolved Hide resolved

Luap99 requested changes May 30, 2023

View reviewed changes

boaz0 force-pushed the closes_15754 branch from 30c36d2 to a27fd6c Compare May 30, 2023 20:49

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2023

boaz0 force-pushed the closes_15754 branch from a27fd6c to dc0ea24 Compare May 30, 2023 20:52

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2023

Fix: display online_cpus in compat REST API

5c7d50f

Signed-off-by: Boaz Shuster <[email protected]>

boaz0 force-pushed the closes_15754 branch from dc0ea24 to 5c7d50f Compare May 31, 2023 04:42

Luap99 approved these changes May 31, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023

openshift-ci bot assigned mheon Jun 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2023

openshift-merge-robot merged commit e91f6f1 into containers:main Jun 1, 2023

jonasdemoor mentioned this pull request Aug 4, 2023

Jobs are not correctly restarted on Podman 4.6.0 hashicorp/nomad-driver-podman#277

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 31, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023

Fix: display online_cpus in compat REST API #15867

Fix: display online_cpus in compat REST API #15867

Conversation

boaz0 commented Sep 20, 2022

Does this PR introduce a user-facing change?

zhangguanzhang commented Sep 20, 2022

vrothberg left a comment

Choose a reason for hiding this comment

baude commented Sep 27, 2022

baude commented Sep 27, 2022

boaz0 commented Sep 27, 2022

boaz0 commented Sep 29, 2022

edsantiago commented Sep 29, 2022

edsantiago commented Oct 6, 2022

ashley-cui left a comment

Choose a reason for hiding this comment

rhatdan commented Oct 6, 2022

benoitf commented Oct 7, 2022

ashley-cui commented Oct 7, 2022

boaz0 commented Oct 8, 2022

ashley-cui commented Oct 10, 2022

ashley-cui commented Oct 17, 2022

flouthoc commented Oct 18, 2022

lukasmrtvy commented Oct 18, 2022 • edited Loading

flouthoc commented Oct 18, 2022

rhatdan commented Oct 18, 2022

giuseppe commented Oct 19, 2022

boaz0 commented Oct 21, 2022

giuseppe commented Oct 21, 2022

boaz0 commented May 16, 2023

vrothberg commented May 16, 2023

boaz0 commented May 16, 2023

edsantiago commented May 16, 2023

boaz0 commented May 18, 2023

rhatdan commented May 18, 2023

edsantiago commented May 18, 2023

mheon commented May 18, 2023

boaz0 commented May 30, 2023

edsantiago commented May 30, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 May 30, 2023

Choose a reason for hiding this comment

Luap99 May 30, 2023

Choose a reason for hiding this comment

Luap99 May 30, 2023

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 31, 2023

TomSweeneyRedHat commented May 31, 2023

mheon commented Jun 1, 2023

mheon commented Jun 1, 2023

lukasmrtvy commented Oct 18, 2022 •

edited

Loading