Skip to content

systemd: install systemd.conf to enable CPU Accounting#581

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rphillips:feat/add_systemd_accounting
Apr 1, 2019
Merged

systemd: install systemd.conf to enable CPU Accounting#581
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
rphillips:feat/add_systemd_accounting

Conversation

@rphillips
Copy link
Copy Markdown
Contributor

Install system.conf to enable CPU Accounting.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1692131

- What I did
Install a custom system.conf file enabling CPU, BlockIO, and Memory accounting.

ref: https://github.com/openshift/origin/blob/master/contrib/systemd/origin-accounting.conf

- Description for the changelog
Enable CPU Accounting within systemd

/cc @sjenning @derekwaynecarr

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 26, 2019
@rphillips rphillips closed this Mar 26, 2019
@rphillips rphillips reopened this Mar 26, 2019
@rphillips
Copy link
Copy Markdown
Contributor Author

/test e2e-aws

@rphillips
Copy link
Copy Markdown
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2019
@rphillips rphillips force-pushed the feat/add_systemd_accounting branch from e10f1ad to b60a315 Compare March 26, 2019 22:18
@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

3 similar comments
@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

@rphillips
Copy link
Copy Markdown
Contributor Author

/hold cancel

/cc @sjenning @derekwaynecarr @smarterclayton

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2019
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't use origin. Use kubelet-cgroups or something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kubelet-cgroup-accounting.conf

Can you add a description into the system unit so people know why this is there?

@rphillips rphillips force-pushed the feat/add_systemd_accounting branch 2 times, most recently from 7f8028e to 5e8ff60 Compare March 27, 2019 15:32
@rphillips
Copy link
Copy Markdown
Contributor Author

rphillips commented Mar 27, 2019

@smarterclayton thanks. Fixed both comments.

@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

@sjenning
Copy link
Copy Markdown
Contributor

do we want to enable accounting just for {kubelet,crio}.service? enabling accounting globally will incur kernel overhead and will make the stats summary api response much larger.

@sjenning
Copy link
Copy Markdown
Contributor

meh, nevermind. there aren't that many service on rhcos anyway and global memory accounting is already on by default.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2019
@smarterclayton
Copy link
Copy Markdown
Contributor

lgtm as well, approving

@smarterclayton smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2019
@rphillips
Copy link
Copy Markdown
Contributor Author

/test unit

works locally...

@rphillips
Copy link
Copy Markdown
Contributor Author

Looks like master might be broken?

--- FAIL: TestGenerateMachineConfigs (0.17s)
	render_test.go:363: can't find actual file test_data/templates/master/00-master/vsphere/files:

@rphillips rphillips force-pushed the feat/add_systemd_accounting branch from 5e8ff60 to dde204e Compare April 1, 2019 17:24
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2019
@rphillips
Copy link
Copy Markdown
Contributor Author

Had to rebase and regenerate the templates due to a change on master.

@sjenning
Copy link
Copy Markdown
Contributor

sjenning commented Apr 1, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: rphillips, sjenning

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@rphillips
Copy link
Copy Markdown
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit f0a2708 into openshift:master Apr 1, 2019
flokli added a commit to flokli/nixpkgs that referenced this pull request Aug 25, 2019
If this is the default for OpenShift already, we probably can enable it
as well.

see openshift/machine-config-operator#581
vrutkovs pushed a commit to vrutkovs/installer that referenced this pull request Oct 22, 2019
vrutkovs pushed a commit to vrutkovs/installer that referenced this pull request Nov 1, 2019
LorbusChris pushed a commit to LorbusChris/installer that referenced this pull request Jan 20, 2020
LorbusChris pushed a commit to LorbusChris/installer that referenced this pull request Feb 22, 2020
LorbusChris pushed a commit to LorbusChris/installer that referenced this pull request Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants