-
Notifications
You must be signed in to change notification settings - Fork 733
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
Support BPM container type #1319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, unless the BPM container ids are hex strings. Then you'd need to truncate them to 12 hex digits to match other runtimes (and we're about to push a refactor of the cgroup matching code which will simplify the common case quite a bit)
tinfo->m_container_id = container_info.m_id; | ||
if (!manager->container_exists(container_info.m_id)) | ||
{ | ||
container_info.m_name = container_info.m_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do BPM containers have any metadata except for an id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By metadata, do you mean these runc annotations?
Looking into some config.json
as for example /var/vcap/data/bpm/bundles/blobstore/blobstore/config.json
, I can't find annotations
. Therefore I guess that BPM containers do not have any metadata. @xoebus please correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean things like Docker has labels (arbitrary key-value pairs), container names, container image names (both human-readable and a sha256 hash).
Looks like e.g. the job/process names encoded in the container id could work as labels.
Looking at the linked issue, if the container id is a Base-32 encoded string, maybe decode it into the container name (if it cannot contain characters like spaces etc.) or into a label (if it's a completely arbitrary string)? Edit: according to https://www.pivotaltracker.com/n/projects/2070399/stories/150819404 the decoded id contains a job name and a process name. I think they would make fine container labels :) |
@gnosek many thanks for your review :) Regarding your last comment, the issue I linked is a bit misleading. Before v1.0.3 of BPM, container IDs were base32 encoded. From v1.0.3 onwards, the encoding in my comment above applies. However, according to @xoebus, this encoding format can change any time. Regarding to put the job name and process name into labels / annotations: This would be a change on BPM side, right? And sysdig would then read those labels? Where would these labels then be used in sysdig? Before proceeding with this PR, we first need to ensure that the BPM cgroup-path has a somewhat stable format. (Maybe I was a bit too early.) Currently, there is no guarantee at all that the cgroup-path format and the container ID stays the same (see cloudfoundry/bpm-release#109 (comment)). |
Not really, you can pull the metadata from any place that makes sense for a particular runtime (we're e.g. calling the Docker API or reading from rkt JSON files etc.)
Hmm I don't see labels exposed to (c)sysdig userspace actually so right now this would benefit users of the commercial platform. However, it should be feasible to add e.g. filters like
Depending on the BPM release lifecycle, old versions may be supported for a long time so for full points, we'd have to parse the old formats anyway (see e.g. the at least two different rkt layouts). Anyway, since I don't pretend to know anything about BPM, I leave these considerations to you. From the sysdig side, the patch looks fine as is. |
TIL that RunC has annotations! I think we can probably annotate the containers with useful information if that would help? |
Regarding the labels:
True. If a new BPM version has some different cgroup-path format, the worst which can happen is that sysdig cannot identifiy the container anymore. In that case, I would need to adapt the parsing. That said, I would be very happy to bring this patch in. |
@gnosek, is there anything I can do to bring this feature in? |
Sorry for taking my time getting back to you :) We're going through some churn in the container code right now but the things I'd ask you before merging would be: |
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]>
Co-Authored-By: ansd <[email protected]>
sysdig-CLA-1.0-contributing-entity: SAP SE sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>
sysdig-CLA-1.0-contributing-entity: SAP SE sysdig-CLA-1.0-signed-off-by: David Ansari <[email protected]>
@gnosek thanks for your response :) I rebased and removed the stdlib regex dependency. I tested that it still works by:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Identify BPM containers by "bpm-" prefix in cgroup-path.
See also cloudfoundry/bpm-release#109 and cloudfoundry/bpm-release@ea7fef1.
This PR makes it possible to monitor BPM containers on BOSH director and BOSH deployed VMs.
I tested this change by co-deploying https://github.com/ansd/sysdig-boshrelease on the BOSH director.
is then identifying processes belonging to BPM containers.
Also, I validated on the director that selecting the Containers View in
csysdig
is showing the containers corresponding to the output ofbpm list
.