Skip to content
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

v1: reduce duplicated code #202

Merged
merged 1 commit into from
Jul 23, 2021
Merged

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 15, 2021

Marking as draft, because this depends on / is a follow-up to #200 (merged)

Code for handling Tasks and Processes, except for "cgroup.procs" vs "tasks", and Tasks being a different type.

This patch:

  • Makes the Task type an alias for Process (as they're identical otherwise)
  • Adds a processType type for the cgroupProcs and cgroupTasks consts. This type is an alias for "string", and mostly for clarity (indicating it's an 'enum').
  • Merges the cgroup.add() and cgroup.addTask() functions, adding a pType argument.
  • Merges the cgroup.processes() and cgroup.tasks() functions, adding a pType argument.
  • Merges the readPids() and readTasksPids() utilities, adding a pType argument.
  • Move locking and validation into cgroup.add(). All places using cgroup.add() were taking a lock and doing this validation, so looks we can move this code into cgroup.add() itself.

Code for handling Tasks and Processes, except for "cgroup.procs" vs "tasks",
and Tasks being a different type.

This patch:

- Makes the Task type an alias for Process (as they're identical otherwise)
- Adds a `processType` type for the `cgroupProcs` and `cgroupTasks` consts. This type
  is an alias for "string", and mostly for clarity (indicating it's an 'enum').
- Merges the `cgroup.add()` and `cgroup.addTask()` functions, adding a `pType` argument.
- Merges the `cgroup.processes()` and `cgroup.tasks()` functions, adding a `pType` argument.
- Merges the `readPids()` and `readTasksPids()` utilities, adding a `pType` argument.
- Move locking and validation into `cgroup.add()`. All places using `cgroup.add()`
  were taking a lock and doing this validation, so looks we can move this code into
  `cgroup.add()` itself.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Contributor

@sameo sameo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this looks good to me.

@thaJeztah
Copy link
Member Author

Thanks for reviewing!

Perhaps @AkihiroSuda or @fuweid could have a look as well (making sure I didn't do anything silly 😂)

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit 2c11864 into containerd:main Jul 23, 2021
@thaJeztah thaJeztah deleted the cgroupv1_less_dry branch July 23, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants