Prepare cpuset for import into kubernets/utils #113744

iancoolidge · 2022-11-08T15:26:20Z

Preparation for cpuset to be moved into k/utils

Various API improvements from code review for library prep.

/kind cleanup

k8s-ci-robot · 2022-11-08T15:26:29Z

Hi @iancoolidge. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2022-11-08T15:26:30Z

Welcome @iancoolidge!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

iancoolidge · 2022-11-08T15:27:16Z

@thockin

iancoolidge · 2022-11-08T15:27:24Z

/sig-node

pacoxu · 2022-11-21T03:27:16Z

/ok-to-test
/release-note-none

k8s-ci-robot · 2023-01-04T08:05:17Z

LGTM label has been added.

Git tree hash: abe12a81e7990ce9390b79fc29fc92969bd9854a

iancoolidge · 2023-01-04T19:43:37Z

Sorry for noise. Cleaned up code in test/e2e_node/cpu_manager_test.go a bit to use frameworks.ExpectNoError instead of dragging in klog and exit calls.

iancoolidge · 2023-01-04T20:24:34Z

/retest

klueska · 2023-01-05T10:07:30Z

pkg/kubelet/cm/cpuset/cpuset.go

 // CPUSet is a thread-safe, immutable set-like data structure for CPU IDs.
 type CPUSet struct {
 	elems map[int]struct{}
 }



It's still unclear to me why we don't just implement this in terms of a sets.Set[int] from the apimachinery/pkg/util/sets package. Most of the functions are already implemented (so would be a simple passthrough), and the one's that aren't can easily be implemented in terms of the others.

I had previously suggested this in kubernetes/utils#267 (comment), but no one responded.

The only reason I can see not doing this is if that would prevent us from moving the implementation to kubernetes/utils. I don't know what the requirements are there in terms of package imports.

/cc @thockin

Hi Kevin,

First, I agree that is a good goal (it is acknolwedged in the package comment, and has been discussed and not ignored, despite how your comment thread appears)

If you review my issue though, the problem I'm trying to solve is exactly exposing this in k/utils, so, depending on something not available in k/utils seems counter productive to the primary goal.

To me, a sensible strategy is:

keep current implementation of cpuset

move cpuset to k/utils

move set to k/utils also

reimplement cpuset in terms of set at that point

iancoolidge · 2023-01-06T16:29:36Z

/retest

thockin

Just niggles in comments

pkg/kubelet/cm/cpuset/cpuset.go

This allows Union to implement UnionAll easily.

Removes exit/fatal from cpuset library. Usage in podresources test was not necessary. Library reference in cpu_manager_test was moved to a local function, and converted to use e2e test framework error catching.

These are rarely used and can be accommodated with a trivial helper.

In 'set', conversions to slice are done also, but with different names: ToSliceNoSort() -> UnsortedList() ToSlice() -> List() Reimplement List() in terms of UnsortedList to save some duplication.

FilterNot is only used in this file, and is trivially converted to a 'filter' call site by inverting the predicate. Filter is only used in this file, so don't export it.

All usage of builder pattern is convertible to cpuset.New() with the same or fewer lines of code. Migrate Builder.Add to a private method of CPUSet, with a comment that it is only intended for internal use to preserve immutable propoerty of the exported interface. This also removes 'require' library dependency, which avoids non-standard library usage.

Describe use cases (node IDs, HT siblings, etc) Call out novelty (Linux CPU list parse/dump) Describe future work (relax immutable, refactor to use 'set')

thockin · 2023-01-07T00:12:11Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-01-07T00:12:16Z

LGTM label has been added.

Git tree hash: 70b2cdf3089ea351684a45577e2aa33181c30b2c

k8s-ci-robot · 2023-01-07T00:12:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iancoolidge, thockin

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:

~~cmd/kubelet/OWNERS~~ [thockin]
~~pkg/kubelet/cm/OWNERS~~ [thockin]
~~test/e2e_node/OWNERS~~ [thockin]

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

k8s-triage-robot · 2023-01-07T02:58:19Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 8, 2022

k8s-ci-robot added area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2022

k8s-ci-robot requested review from klueska and mrunalp November 8, 2022 15:27

iancoolidge changed the title ~~Devel cpuset set~~ Prepare cpuset for import into kubernets/utils Nov 8, 2022

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Nov 9, 2022

SergeyKanzhelev added this to Triage in SIG Node PR Triage Nov 9, 2022

SergeyKanzhelev moved this from Triage to Archive-it in SIG Node CI/Test Board Nov 9, 2022

SergeyKanzhelev moved this from Triage to Waiting on Author in SIG Node PR Triage Nov 9, 2022

iancoolidge mentioned this pull request Nov 11, 2022

Import 'cpuset' from kubernetes repository in kubernetes/utils kubernetes/utils#267

Merged

iancoolidge marked this pull request as ready for review November 17, 2022 21:44

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2022

k8s-ci-robot requested a review from pacoxu November 17, 2022 21:48

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2023

iancoolidge force-pushed the devel-cpuset-set branch from 0996be7 to 0d233f3 Compare January 4, 2023 19:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2023

k8s-ci-robot requested a review from thockin January 4, 2023 19:42

klueska reviewed Jan 5, 2023

View reviewed changes

thockin reviewed Jan 6, 2023

View reviewed changes

pkg/kubelet/cm/cpuset/cpuset.go Outdated Show resolved Hide resolved

iancoolidge added 8 commits January 6, 2023 23:32

cpuset: Convert Union arguments to variadic

824bd57

This allows Union to implement UnionAll easily.

cpuset: Remove 'MustParse' method

67a057d

Removes exit/fatal from cpuset library. Usage in podresources test was not necessary. Library reference in cpu_manager_test was moved to a local function, and converted to use e2e test framework error catching.

cpuset: Remove *Int64 methods

a0c989b

These are rarely used and can be accommodated with a trivial helper.

cpuset: Make 'ToSlice*' methods look like 'set' methods

e5143d1

In 'set', conversions to slice are done also, but with different names: ToSliceNoSort() -> UnsortedList() ToSlice() -> List() Reimplement List() in terms of UnsortedList to save some duplication.

cpuset: hide 'Filter' API

768b1ec

FilterNot is only used in this file, and is trivially converted to a 'filter' call site by inverting the predicate. Filter is only used in this file, so don't export it.

cpuset: Rename 'NewCPUSet' to 'New'

f3829c4

cpuset: Add package comment

5533e49

Describe use cases (node IDs, HT siblings, etc) Call out novelty (Linux CPU list parse/dump) Describe future work (relax immutable, refactor to use 'set')

iancoolidge force-pushed the devel-cpuset-set branch from 0d233f3 to 5533e49 Compare January 6, 2023 23:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2023

k8s-ci-robot merged commit 8ce77a5 into kubernetes:master Jan 7, 2023

SIG Node CI/Test Board automation moved this from Archive-it to Done Jan 7, 2023

SIG Node PR Triage automation moved this from Needs Approver to Done Jan 7, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 7, 2023

klueska mentioned this pull request Jan 11, 2023

Add bart0sh as a sig-node reviewer #114989

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare cpuset for import into kubernets/utils #113744

Prepare cpuset for import into kubernets/utils #113744

iancoolidge commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

iancoolidge commented Nov 8, 2022

iancoolidge commented Nov 8, 2022

pacoxu commented Nov 21, 2022 •

edited

k8s-ci-robot commented Jan 4, 2023

iancoolidge commented Jan 4, 2023

iancoolidge commented Jan 4, 2023

klueska Jan 5, 2023

iancoolidge Jan 5, 2023

iancoolidge commented Jan 6, 2023

thockin left a comment

thockin commented Jan 7, 2023

k8s-ci-robot commented Jan 7, 2023

k8s-ci-robot commented Jan 7, 2023

k8s-triage-robot commented Jan 7, 2023

Prepare cpuset for import into kubernets/utils #113744

Prepare cpuset for import into kubernets/utils #113744

Conversation

iancoolidge commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

iancoolidge commented Nov 8, 2022

iancoolidge commented Nov 8, 2022

pacoxu commented Nov 21, 2022 • edited

k8s-ci-robot commented Jan 4, 2023

iancoolidge commented Jan 4, 2023

iancoolidge commented Jan 4, 2023

klueska Jan 5, 2023

Choose a reason for hiding this comment

iancoolidge Jan 5, 2023

Choose a reason for hiding this comment

iancoolidge commented Jan 6, 2023

thockin left a comment

Choose a reason for hiding this comment

thockin commented Jan 7, 2023

k8s-ci-robot commented Jan 7, 2023

k8s-ci-robot commented Jan 7, 2023

k8s-triage-robot commented Jan 7, 2023

pacoxu commented Nov 21, 2022 •

edited