Re: [pcp] pcp updates: pmwebd et al

[Nathan Scott <nathans@xxxxxxxxxx>]

Thanks Ken,

----- Original Message -----
> [...]
>        Revert "pmmgr: remove [...] comments about libpcp and containers"

Hmm, this re-revert is NQR. Its also a bit surprising that
you ah weren't informed it had been previously reverted and
that it was not generally agreed with.  Hmmm.  Anyway, here
are some notes based on earlier discussion & activity ...


    Revert "pmmgr: remove [...] comments about libpcp and containers"
    
    The comments remain helpful & appropriate.
    This reverts commit 7c679d92cd3da8e166c58b0b807bc526035e1f9b.

diff --git a/src/pmmgr/pmmgr.cxx b/src/pmmgr/pmmgr.cxx
index 524d2d1..bf7ade6 100644
--- a/src/pmmgr/pmmgr.cxx
+++ b/src/pmmgr/pmmgr.cxx
@@ -527,6 +527,11 @@ pmmgr_job_spec::poll()
                    ++it2) {
                   // XXX: presuming that the container name is safe & needs no 
escape;
                   // on docker, this is ok because the container id is a long 
hex string.
+                  //
+                  // NB: It appears as though the '[?&]container=XXX' suffix 
is recognized
+                  // by libpcp on any general hostspec string, incl. ip 
addresses, hostnames,
+                  // not just the pcp://* URLs generated by 
pmDiscoverServices.  This should
+                  // probably be documented.


I suggested these 'actually' be documented properly in man pages, not just
'probably', but that was ignored.  Someone else updated the man page docs
in the meantime, and this comment was appropriately dropped.


                   pmmgr_hostid subtarget_hostid = it->first + string("--") + 
*it2;
                   // Choose ? or & for the hostspec suffix-prefix, depending
                   // on whether there's already a ?.  There can be only one 
(tm).
@@ -536,6 +541,8 @@ pmmgr_job_spec::poll()
                   known_targets[subtarget_hostid] = subtarget_spec;
               }
           }
+      // XXX: what if there are nested containers?  oh well, libpcp
+      // doesn't handle &container=FOO/BAR
   }


Several times it was explained that this comment doesn't make sense, libpcp
does not (and should not) know anything about containers, let alone these
theoretical nested containers.  It was suggested to change it (mention the
pmdaroot involvement, maybe move to pmdaroot if its really necessary), but
that was not done either.  So it was simply dropped - which remains a good
option still IMO, as this comment is neither helpful nor appropriate (nor
even vaguely correct) as-is.


I've re-re-reverted these changes in the latest merge and pushed a few more
that were kicking around - I think we're all up to date now.  Onward, and a
happy new year to all!

cheers.

--
Nathan