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

cgroup-path #109

Closed
ansd opened this issue Nov 12, 2018 · 5 comments
Closed

cgroup-path #109

ansd opened this issue Nov 12, 2018 · 5 comments
Labels

Comments

@ansd
Copy link

ansd commented Nov 12, 2018

TLDR:

  1. prepend bpm to the cgroup path
  2. do not base32 encode the container id

Details:

In a previous story, the decision was to base32 encode the job name for the runc container-id which becomes the cgroup-path in the file /proc/[pid]/cgroup:

hierarchy-ID:controller-list:cgroup-path

I would like to write a sysdig-bosh release which will be colocated on VMs and which should ideally also be able to monitor bpm containers. Sysdig parses the cgroup-path in order to classify the container type (see here for LXC).
For example, the cgroup-path for an LXC container looks like /lxc/my-container:

cat /proc/<pid of lxc container>/cgroup
11:hugetlb:/lxc/my-container

It's the default for LXC (see here):

lxc.cgroup.pattern
Format string used to generate the cgroup path (e.g. lxc/%n).

According to the sysdig code, the same applies to mesos, libvirt-lxc and docker.
Only the bpm container has this base32 encoded cgroup-path like

12:pids:/MJWG6YTTORXXEZI-

for the blobstore for example.

Having prepended something like bpm. to the cgroup-path would allow to parse it easily in sysdig such that the filter csysdig -pc container.type=bpm could match BPM containers.

But even though if the cgroup-path looks like bpm.MJWG6YTTORXXEZI-, the output for above filter would show a container column with the name MJWG6YTTORXXEZI- rather than blobstore.

Therefore, it would also be better to not base32 encode the whole container name, but encode maybe only those chars which do not match the regex ^[\w+-\.]+$ (see code). This would probably require to have a special delimiter char for the decoding process in order to identify the encoded chars.

See also Slack discussion for more context.

If the above two requirements are okay with you, I could also create a PR.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/161874197

The labels on this github issue will be updated when the story is started.

ansd added a commit to ansd/bpm-release that referenced this issue Dec 7, 2018
Also, prepend "bpm-" to the container ID.

Close cloudfoundry#109
ansd added a commit to ansd/bpm-release that referenced this issue Dec 7, 2018
Also, prepend "bpm-" to the container ID.

Close cloudfoundry#109
ansd added a commit to ansd/bpm-release that referenced this issue Dec 7, 2018
Also, prepend "bpm-" to the container ID.

Close cloudfoundry#109
ansd added a commit to ansd/bpm-release that referenced this issue Dec 7, 2018
Also, prepend "bpm-" to the container ID.

Close cloudfoundry#109
@xoebus
Copy link
Contributor

xoebus commented Feb 5, 2019

Just merged a new encoding format for BPM job IDs. It should go out in the next release. Let us know if it's missing anything.

Thanks for the issue!

@ansd
Copy link
Author

ansd commented Feb 18, 2019

@xoebus many thanks for changing the container-id.

As a side note:
After testing your change I realise that the cgroup-path is not just the container-id anymore:

bosh/0:~# cat /proc/<blobstore pid>/cgroup
12:devices:/system.slice/runc-bpm-blobstore.scope

So, the cgroup-path is /system.slice/runc-bpm-blobstore.scope rather than just bpm-blobstore.
This was introduced by pass --systemd-cgroup when on a systemd machine.
/system.slice/runc- comes from here and .scope comes from here.

(The parent (system.slice), ScopePrefix (runc), and the appended scope (vs slice) are configurable.)

It makes parsing the cgroup-path in sysdig less straightforward.
However, I think that parsing the cgroup-path for bpm- on systemd and non-systemd machines still identifies the BPM container type. And the container-id (from sysdig perspective) is then the string between bpm- and either .scope or the end of the string.

ansd added a commit to ansd/sysdig that referenced this issue Feb 19, 2019
Identify BPM containers by "bpm-" prefix in cgroup-path.

See also cloudfoundry/bpm-release#109
and cloudfoundry/bpm-release@ea7fef1
ansd added a commit to ansd/sysdig that referenced this issue Feb 19, 2019
Identify BPM containers by "bpm-" prefix in cgroup-path.

See also cloudfoundry/bpm-release#109
and cloudfoundry/bpm-release@ea7fef1
ansd added a commit to ansd/sysdig that referenced this issue Feb 19, 2019
Identify BPM containers by "bpm-" prefix in cgroup-path.

See also cloudfoundry/bpm-release#109
and cloudfoundry/bpm-release@ea7fef1

sysdig-CLA-1.0-contributing-entity: SAP SE
sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>
@xoebus
Copy link
Contributor

xoebus commented Feb 21, 2019

Changing the prefix will change behavior as it's changing the cgroup hierarchy. We're going to probably need to change that soon as we start doing CPU share limiting which is very dependent on the hierarchy. I don't know if we can make any guarantees about what that path will be.

@ansd
Copy link
Author

ansd commented Feb 25, 2019

@xoebus hmm I'm not that deep into how CPU share limiting works and how the cgroup hierachy gets affected, and I don't know whether the following suggestion makes sense but wouldn't it be possible that the cgroup-path stays somewhat stable and BPM identifiable by introducing a bpm root cgroup like Mesos is doing it for example (see https://mesos.readthedocs.io/en/latest/configuration/):

--cgroups_root=VALUE	Name of the root cgroup. (default: mesos)

LXC seems to also have this root cgroup (see https://linuxcontainers.org/lxc/manpages/man5/lxc.system.conf.5.html):

lxc.cgroup.pattern
Format string used to generate the cgroup path (e.g. lxc/%n).

That way you could introduce any other hierarchy (including CPU share limiting) below this root cgroup.

ansd added a commit to ansd/sysdig that referenced this issue May 13, 2019
Identify BPM containers by "bpm-" prefix in cgroup-path.

See also cloudfoundry/bpm-release#109
and cloudfoundry/bpm-release@ea7fef1

sysdig-CLA-1.0-contributing-entity: SAP SE
sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>
gnosek pushed a commit to draios/sysdig that referenced this issue May 20, 2019
* Support BPM container type

Identify BPM containers by "bpm-" prefix in cgroup-path.

See also cloudfoundry/bpm-release#109
and cloudfoundry/bpm-release@ea7fef1

sysdig-CLA-1.0-contributing-entity: SAP SE
sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>

* Update userspace/libsinsp/container_engine/bpm.cpp

Co-Authored-By: ansd <[email protected]>

* Ensure BPM container ID contains only valid chars

sysdig-CLA-1.0-contributing-entity: SAP SE
sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>

* Remove dependency to stdlib regex

sysdig-CLA-1.0-contributing-entity: SAP SE
sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants