OCPNODE-2864: Set ExecCPUAffinity #9286
Conversation
|
@bitoku: This pull request references OCPNODE-2864 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
bafa8ef to
b0c10de
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9286 +/- ##
==========================================
+ Coverage 66.95% 67.28% +0.32%
==========================================
Files 198 199 +1
Lines 27426 27508 +82
==========================================
+ Hits 18364 18509 +145
+ Misses 7545 7453 -92
- Partials 1517 1546 +29 🚀 New features to boost your workflow:
|
|
/retest |
|
/approve looks good! |
|
Just a note as I am on vacation and only noticed the change. It might be a good idea to sync this with #9223 where I proposed we allow configuring the housekeeping cpus using an annotation like I know the first cpu is traditional, but this would give us more flexibility. We can always default to the first cpu (+ hyperthreading siblings?) when the annotation is missing of course. |
|
Thanks @MarSik . |
1cffaf0 to
6795b02
Compare
|
/retest |
| BeforeEach(func() { | ||
| sbox := sandbox.NewBuilder() | ||
| createdAt := time.Now() | ||
| sbox.SetCreatedAt(createdAt) | ||
| sbox.SetID("sandboxID") | ||
| sbox.SetName("sandboxName") | ||
| sbox.SetLogDir("test") | ||
| sbox.SetShmPath("test") | ||
| sbox.SetNamespace("") | ||
| sbox.SetKubeName("") | ||
| sbox.SetMountLabel("test") | ||
| sbox.SetProcessLabel("test") | ||
| sbox.SetCgroupParent("") | ||
| sbox.SetRuntimeHandler("") | ||
| sbox.SetResolvPath("") | ||
| sbox.SetHostname("") | ||
| sbox.SetPortMappings([]*hostport.PortMapping{}) | ||
| sbox.SetHostNetwork(false) | ||
| sbox.SetUsernsMode("") | ||
| sbox.SetPodLinuxOverhead(nil) | ||
| sbox.SetPodLinuxResources(nil) | ||
| err = sbox.SetCRISandbox(sbox.ID(), make(map[string]string), map[string]string{}, &types.PodSandboxMetadata{}) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| sbox.SetPrivileged(false) | ||
| sbox.SetHostNetwork(false) | ||
| sbox.SetCreatedAt(createdAt) | ||
| sbNoShared, err = sbox.GetSandbox() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
Thanks for adding the unit tests. Instead of nesting the Describe and duplicating the BeforeEach, it would be cleaner to restructure the original "Precreate hook" test to use the "Context"/"BeforeEach" pattern (see other tests in this file) and pass in the crioannotations for each test, which seems to be the only thing that differs here.
There was a problem hiding this comment.
updated. let me know how it looks.
There was a problem hiding this comment.
Looks great - thanks for updating/extending the unit tests.
313af6a to
1d88711
Compare
|
you've got a merge conflict |
|
@cri-o/cri-o-maintainers PTAL |
| // Don't set ExecCPUAffinity, which means using runtime default. | ||
| default: | ||
| // This shouldn't happen because there's config validation. | ||
| log.Errorf(ctx, "Unknown ExecCPUAffinityType %s is used. Falling back to default.", h.execCPUAffinity) |
There was a problem hiding this comment.
if this shouldn't happen we should panic here because it's a coding error that should be caught by testing
There was a problem hiding this comment.
It makes sense, but I wonder if we can find it in test because cri-o runs as a service, and get restarted easily.
I'd rather returning error instead of panic, which don't break entire system, but still have a chance to catch coding errors I guess.
|
/override ci/prow/ci-e2e-evented-pleg still LGTM, one more nit if you have time but not a blocker do any other @cri-o/cri-o-maintainers want to TAL? |
|
@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-e2e-evented-pleg 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-sigs/prow repository. |
Signed-off-by: Ayato Tokubi <[email protected]>
a46fd45 to
4dd7fb9
Compare
|
/test ci-cgroupv2-e2e-crun |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/skip |
|
/retest |
|
@cri-o/cri-o-maintainers PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, haircommander, saschagrunert 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:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@bitoku: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
/cherry-pick release-1.33 |
|
@bitoku: #9286 failed to apply on top of branch "release-1.33": 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-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This implements https://issues.redhat.com/browse/OCPNODE-2864
Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPNODE-2864
Special notes for your reviewer:
Does this PR introduce a user-facing change?