-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Provide functionality to start infra containers on the specified set of CPUs #4459
Provide functionality to start infra containers on the specified set of CPUs #4459
Conversation
Hi @cynepco3hahue. Thanks for your PR. I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
docs/crio.8.md
Outdated
**--infra-containers-cpus**="": CPU set to run infra containers, if not specified the CRI-O will use all online CPUs to run infra containers (default: ""). | ||
You can specify CPUs in the Linux CPU list format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I think --infra-cpus
would work too
nit: we should describe that these should be the same as the guaranteed-cpus
passed to the kubelet, if the user wants their workloads to truly have a unique cpuset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, above we use infra-ctr
so we should use that here --infra-ctrs-cpus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about infra-ctr-cpuset
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpus
is too broad and could also refer to amount of cpu shares reserved or cpu quota if we want to add those in the future. So, I think it may be better to be explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, done
internal/criocli/criocli.go
Outdated
@@ -824,3 +839,15 @@ func StringSliceTrySplit(ctx *cli.Context, name string) []string { | |||
|
|||
return values | |||
} | |||
|
|||
// cpuSetParse parses the the string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update this comment a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I forgot to do it, done
internal/oci/oci.go
Outdated
func (r *Runtime) GetInfraCPUs() *cpuset.CPUSet { | ||
return r.config.InfraContainersCPUs | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this really makes sense in the runtime object. Either in the server config (pkg/config/config.go
) or in a special package makes more sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this function.
@haircommander Does it ok to have the infra_ctr_cpus
parameter under the
[runtime]
...
infra_ctr_cpus=<something>
...
or should I move this parameter under the separate section(probably in the future we can have the same functionality for the memory pinning and more).
server/container_create_linux.go
Outdated
if infraCPUs != nil { | ||
specgen.SetLinuxResourcesCPUCpus(infraCPUs.String()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to set this here I believe. what this is doing is settting a new, non-infra container to use infra cpus, if it has cpus set. What we should do is set infra containers to be in those cpus (in server/sandbox_run_linux.go). Or, we can move the infra container to the cpus when the container is created (similar to how a container is in container_update_resources)
does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking about this more, we should unconditionally set the infra container's cpuset if infra-ctr-cpus
is set. we'll have to do that in server/sandbox_run_linux.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Codecov Report
@@ Coverage Diff @@
## master #4459 +/- ##
==========================================
- Coverage 40.09% 40.03% -0.06%
==========================================
Files 116 116
Lines 9378 9391 +13
==========================================
Hits 3760 3760
- Misses 5212 5224 +12
- Partials 406 407 +1 |
/ok-to-test |
59e83ac
to
63c3495
Compare
docs/crio.conf.5.md
Outdated
@@ -225,6 +225,10 @@ the container runtime configuration. | |||
**drop_infra_ctr**=false | |||
Determines whether we drop the infra container when a pod does not have a private PID namespace, and does not use a kernel separating runtime (like kata). | |||
Requies **manage_ns_lifecycle** to be true. | |||
|
|||
**infra-ctrs-cpus**="" | |||
Determines the CPU set to run infra containers, if not specified the CRI-O will use all online CPUs to run infra containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Determines the CPU set to run infra containers, if not specified the CRI-O will use all online CPUs to run infra containers. | |
Determines the CPU set to run infra containers. If not specified, the CRI-O will use all online CPUs to run infra containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ce1fd99
to
428ce99
Compare
/hold |
428ce99
to
fa1be15
Compare
/hold cancel |
6417cff
to
fcf4242
Compare
b6ce24a
to
70c00a9
Compare
/retest |
70c00a9
to
f8fa688
Compare
/retest |
f8fa688
to
269c415
Compare
a2ceff4
to
b7e7ff7
Compare
/retest |
1 similar comment
/retest |
@@ -225,6 +225,11 @@ the container runtime configuration. | |||
**drop_infra_ctr**=false | |||
Determines whether we drop the infra container when a pod does not have a private PID namespace, and does not use a kernel separating runtime (like kata). | |||
Requies **manage_ns_lifecycle** to be true. | |||
|
|||
**infra_ctr_cpuset**="" | |||
Determines the CPU set to run infra containers. If not specified, the CRI-O will use all online CPUs to run infra containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Determines the CPU set to run infra containers. If not specified, the CRI-O will use all online CPUs to run infra containers. | |
Determines the CPU set to run infra containers. If not specified, CRI-O will use all online CPUs to run infra containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/criocli/criocli.go
Outdated
@@ -794,6 +798,11 @@ func getCrioFlags(defConf *libconfig.Config) []cli.Flag { | |||
EnvVars: []string{"CONTAINER_VERSION_FILE_PERSIST"}, | |||
TakesFile: true, | |||
}, | |||
&cli.StringFlag{ | |||
Name: "infra-ctr-cpuset", | |||
Usage: "CPU set to run infra containers, if not specified the CRI-O will use all online CPUs to run infra containers (default: '').", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage: "CPU set to run infra containers, if not specified the CRI-O will use all online CPUs to run infra containers (default: '').", | |
Usage: "CPU set to run infra containers, if not specified CRI-O will use all online CPUs to run infra containers (default: '').", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The new flag should provide a way to specify desired set of CPUs used to run infra containers. Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
b7e7ff7
to
10c9e49
Compare
/retest |
1 similar comment
/retest |
/lgtm |
/cherry-pick release-1.20 |
@haircommander: #4459 failed to apply on top of branch "release-1.20":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Infra containers can start on every online CPU under the system that can be a problem for the latency-sensitive workloads because every context switch can result in latency peaks.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?