Re: pcp updates: pmdadocker

Hi Lukas,

----- Original Message -----
> Hey All,
> 
> If I could get a review on my pmdadocker branch it'd be most
> appreciated.  More qa needs to be added with a fake http request/reply
> but the jist of the pmda is there.

See also qa/common.docker and existing tests that use docker directly,
given the evidently rapid docker API evolution I suspect it will be
particularly important for the PMDA testing to have something talking
to an actual docker daemon as well.

> Changes committed to git://git.pcp.io/lberk/pcp.git pmdadocker
> [...]
>  src/libpcp_web/src/json_helpers.c |   84 +++

(I'm seeing a merge conflict on this file BTW)

The pmjsonInitChar interface is a bit limiting I think - it's assuming
the JSON document fits in the buffer, which is a bit of a limiting API
for a library.  There's also the duplication of the other code ...

An alternative approach to tackle those aspects would be to provide a
callback routine which could fill in the buffer.  Then implementation
of the existing pmjsonInitIndom routine could use that, with a simple
read(2) based buffer-filler (the fd will need to be passed in through
a void* callback parameter).

(Man page update needed here too.)

Unrelated to this change, but do you remember why we went with initial
"json_token_count" as low as 2?  Seems very low; from some recent JSMN
hacking I was doing, something like 256 or 512 might make for a better
starting point?  e.g. the empty JSON document "{}" has 2 tokens.

>  src/pmdas/GNUmakefile             |    2
>  src/pmdas/docker/GNUmakefile      |   67 ++
>  src/pmdas/docker/Install          |   27 +
>  src/pmdas/docker/Remove           |   38 +

(Remove script references the papi PMDA :)

>  src/pmdas/docker/docker.c         | 1004

Few issues here:

- the units for the metrics are not yet complete (all are zeroes).  I'm
  imagining many of these will have the units of the cgroups equivalents
  from pmdas/linux_proc, so those could be used as a reference.  But it
  would be worth checking that docker doesn't change units or anything
  like that (lots of the mem metrics will be in bytes, cpu metrics will
  be in msec/usec/nsec - but these may be scaled up by docker?).

- the locking is not going to work like this - I think it *has* to use
  the "shadow indom" scheme that pmdarpm uses.  Reason for that (and,
  missing in this PMDA so far, but needed), is the instance PDU callback
  also needing access to that indom.  When using pmdaInstance(3), which
  is a very good idea, the pmdaCache will be accessed directly, unlocked.

- the new docker_instance callback will need to do the notready thing as
  well (as fetch does now), so its worth looking at pmdarpm for that too
  and its "Complain every 30 seconds." thing would be good to have here.

And smaller things:

- in the fetch callback, there's a mongo switch with cases 0 through to
  48 falling through to common code - that could be collapsed into a one
  liner like "if idp->item <= 48)"
- there's a missing unlock in docker_fetch() I think, if mem alloc fails
- there's a lock inversion in update_stats_cache() - the code should do
  the unlocks in the reverse order of locking.
- the initial malloc of local_json_metrics looks like a memory leak?  It
  will be overwritten by pmdaCacheLookupName almost straight away, will
  it not?  (not 100% sure, but I think that's the case)

In terms of future extension, I think it'd be great to see:
- use of the events docker API interface to reduce overheads
- replacement of the /var/lib/docker use, for container enumeration, with
  direct API calls
- more metrics that are not available through the --container approach &
  pmdalinux/pmdaproc/pmdaroot already - esp. if there are dockerd internal
  stats about its own operations, I think that would be very useful.

cheers!

--
Nathan

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#14898): https://groups.io/g/pcp/message/14898
View All Messages In Topic (4): https://groups.io/g/pcp/topic/3780835
Mute This Topic: https://groups.io/mt/3780835?uid=174580
New Topic: https://groups.io/g/pcp/post

Change Your Subscription: https://groups.io/g/pcp/editsub?uid=174580

Group Home: https://groups.io/g/pcp
Contact Group Owner: [email protected]

Terms of Service: https://groups.io/static/tos

Unsubscribe: https://groups.io/g/pcp/leave/354243/563757577/xyzzy

-=-=-
pcp mailing list
[email protected]
https://groups.io/g/pcp/messages
-=-=-=-=-=-=-=-=-=-=-=-