Report cache_peer context in probe and standby pool messages #277

eduard-bagdasaryan · 2024-12-02T15:01:29Z

ERROR: Failed to establish a TCP connection
    error: timeout while trying to established a TCP connection
    destination: ...
    Squid-to-peer TCP connection: ... cache_peer=peerXyz flags=1
    seconds past the establishment deadline: 1

The absence of the usual "current master transaction:..." detail in
certain errors raises “Has Squid lost the current transaction context?”
red flags. In some cases, Squid may have lost that context, but for
cache_peer TCP probes, Squid has not because those probes are not
associated with master transactions. It is difficult to distinguish the
two cases because no processing context is reported. To address those
concerns, we now report current cache_peer probing context (instead of
just not reporting absent master transaction context):

ERROR: ...
    current cache_peer probe: peerXyz

When maintaining a cache_peer standy=N connection pool, Squid has and
now reports both contexts, attributing messages to pool maintenance:

ERROR: ...
    current cache_peer standy pool: peerXyz
    current master transaction: master42

The new PrecomputedCodeContext class handles both reporting cases and
can be reused whenever the cost of pre-computing detailCodeContext()
output is acceptable.

Initial implementation.

rousskov · 2024-12-02T16:24:04Z

src/neighbors.cc

@@ -1113,7 +1113,7 @@ neighborUp(const CachePeer * p)
 {
    if (!p->tcp_up) {
        // TODO: When CachePeer gets its own CodeContext, pass that context instead of nullptr


Looks like this PR completes this TODO:

Suggested change

// TODO: When CachePeer gets its own CodeContext, pass that context instead of nullptr

rousskov · 2024-12-02T16:43:04Z

src/CachePeer.cc

+CachePeerProbeCodeContext::detailCodeContext(std::ostream &os) const
+{
+    if (peer.valid())
+        os << Debug::Extra << "current cache_peer probe: " << peer->name;


Please see our commit a555a85 regarding CachePeer reporting rules.

Suggested change

os << Debug::Extra << "current cache_peer probe: " << peer->name;

os << Debug::Extra << "current cache_peer probe: " << *peer;

rousskov · 2024-12-02T17:08:44Z

src/CachePeer.cc

+    if (peer.valid())
+        return peer->id.detach();
+
+    return ScopedId("CachePeerProbeCodeContext w/o peer");


Let's simplify this code and not add CachePeer::id in this PR. We already have a TODO about adding InstanceId to such classes. Also, the whole idea of having codeContextGist() returning an identifier may be an API bug: We should rethink how FadingCodeContext works, so that we can avoid identifier copying and convert this method into an std::ostream-printing method.

Suggested change

if (peer.valid())

return peer->id.detach();

return ScopedId("CachePeerProbeCodeContext w/o peer");

// Unfortunately, peer->name lifetime is too short. See also: AnyP::PortCfg::codeContextGist().

return ScopedId("cache_peer probe");

rousskov · 2024-12-02T17:12:32Z

src/PeerPoolMgr.h

@@ -18,6 +18,20 @@ class HttpRequest;
 class CachePeer;
 class CommConnectCbParams;

+class PeerPoolMgrContext : public CodeContext


Please see whether this class can be generalized into DetailedCodeContext class that takes an SBuf detail and a c-string gist as constructor parameters (and stores that info in its data members, to be reported using CodeContext APIs). If that works, use DetailedCodeContext instead of CachePeerProbeCodeContext as well.

Eventually, CachePeer will be reference-counted and, hence, may not need a dedicated CodeContext data member, but (re)using DetailedCodeContext for CachePeer now may help us make progress while those (complex) changes are pending (and waiting other/stuck smooth reconfiguration PRs).

eduard-bagdasaryan

Done (30978d0).

eduard-bagdasaryan · 2024-12-03T14:46:49Z

src/CachePeer.cc

+    if (peer.valid())
+        return peer->id.detach();
+
+    return ScopedId("CachePeerProbeCodeContext w/o peer");


eduard-bagdasaryan · 2024-12-03T14:49:45Z

src/CachePeer.cc

+CachePeerProbeCodeContext::detailCodeContext(std::ostream &os) const
+{
+    if (peer.valid())
+        os << Debug::Extra << "current cache_peer probe: " << peer->name;


eduard-bagdasaryan · 2024-12-03T14:51:34Z

src/neighbors.cc

@@ -1113,7 +1113,7 @@ neighborUp(const CachePeer * p)
 {
    if (!p->tcp_up) {
        // TODO: When CachePeer gets its own CodeContext, pass that context instead of nullptr


eduard-bagdasaryan · 2024-12-03T23:42:58Z

src/PeerPoolMgr.h

@@ -18,6 +18,20 @@ class HttpRequest;
 class CachePeer;
 class CommConnectCbParams;

+class PeerPoolMgrContext : public CodeContext


rousskov · 2024-12-04T17:47:25Z

src/neighbors.cc

-        ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);
+    for (p = Config.peers; p; p = p->next) {
+        CallService(p->probeCodeContext, [&] {
+            ipcache_nbgethostbyname(p->host, peerDNSConfigure, p);


CallService() is for switching to "service" context. ipcache_nbgethostbyname() service is not peer-specific -- the DNS lookup service serves all peers and may even share one lookup across multiple service requesting clients. We should be setting the service caller context here, not the service context. I think it would be more appropriate to use explicit CodeContext::Reset() calls in this loop, just like in neighborsUdpPing() loop modified in this PR.

N.B. peerProbeConnect() is a peer-specific "service".

rousskov · 2024-12-04T17:50:13Z

src/PeerPoolMgr.cc

@@ -32,7 +33,8 @@ PeerPoolMgr::PeerPoolMgr(CachePeer *aPeer): AsyncJob("PeerPoolMgr"),
    request(),
    transportWait(),
    encryptionWait(),
-    addrUsed(0)
+    addrUsed(0),
+    context(new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool", *peer)))


Missing delimiter?

Suggested change

context(new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool", *peer)))

context(new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool: ", *peer)))

rousskov · 2024-12-04T19:30:39Z

src/PeerPoolMgr.h

@@ -69,6 +92,9 @@ class PeerPoolMgr: public AsyncJob
    JobWait<Security::BlindPeerConnector> encryptionWait;

    unsigned int addrUsed; ///< counter for cycling through peer addresses
+
+public:
+    DetailedCodeContext::Pointer context;


Adding CodeContext to PeerPoolMgr job is a red flag for me: We have numerous AsyncJobs that do master transaction-related work. Virtually none¹ of them store a CodeContext pointer to report that master transaction -- after their ALE-based code context is established, those jobs automatically get the right context via AsyncCalls and such. DetailedCodeContext is different from ALE-based context, but I see no reason for PeerPoolMgr job to be different with regard to its overall context storage approach.

If we can apply existing ALE CodeContext handling/storage approach to PeerPoolMgr jobs without heroic efforts, let's do that instead of adding this data member.

Footnotes

TunnelStateData stores CodeContext but with a TODO to remove that storage. ↩

Ok, let's move it from the job, but we have to keep it somewhere anyway in order to pass to PeerPoolMgr::checkpoint(). I moved it to CachePeer::standby structure, which looks logical.

Ok, let's move it from the job,

Hm... Its precise location (inside the job) was not the red flag I was trying to describe. The red flag was the addition of that CodeContext data member (as such, in any class), not the addition of PeerPoolMgr::context (specifically). Sorry.

we have to keep it somewhere anyway in order to pass to PeerPoolMgr::checkpoint()

Aha, PeerPoolMgr::Checkpoint() is the piece I was missing! We have to store this CodeContext (somewhere) because PeerPoolMgr::Checkpoint() is not called via events or async calls and, hence, cannot reuse those mechanisms for relaying context back into PeerPoolMgr job. PeerPoolMgr is probably the best location for this context storage because this context is specific to PeerPoolMgr. Please undo that move.

rousskov

I think we are zeroing-in on the final solution here. Please note that some change requests will require more code adjustments than a single GitHub suggestion can express.

rousskov · 2024-12-05T14:22:34Z

src/PeerPoolMgr.h

+
+    void setMasterXaction(const MasterXaction::Pointer &mx) { masterXaction = mx; }


Please do not add this unused method:

Suggested change

void setMasterXaction(const MasterXaction::Pointer &mx) { masterXaction = mx; }

rousskov · 2024-12-05T14:30:21Z

src/PeerPoolMgr.cc

    transportWait(),
    encryptionWait(),
    addrUsed(0)
 {
+    const auto mx = MasterXaction::MakePortless<XactionInitiator::initPeerPool>();
+    context = new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool: ", *peer), mx);


PR code avoids storing CachePeer inside DetailedCodeContext by computing the corresponding detail in advance. Let's assume that decision to precompute was correct.

If possible, please apply the same "let's precompute!" decision to MasterXaction storage, eliminating DetailedCodeContext::masterXaction data member and generalizing/simplifying DetailedCodeContext to better match the new class name/intent.

I added a new SBuf field (storing mx->id) instead.

src/PeerPoolMgr.cc

rousskov · 2024-12-05T14:38:49Z

src/PeerPoolMgr.h

+public:
+    typedef RefCount<DetailedCodeContext> Pointer;
+
+    DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx) : gist_(gist),


Here and elsewhere, please use natural language "items: item1, item2" formatting for labeled lists:

Suggested change

DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx) : gist_(gist),

DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx): gist_(gist),

rousskov · 2024-12-05T14:51:35Z

src/PeerPoolMgr.h

+/// CodeContext for cache_peer related classes
+class DetailedCodeContext : public CodeContext


Assuming other change requests in this review are correct:

Suggested change

/// CodeContext for cache_peer related classes

class DetailedCodeContext : public CodeContext

/// CodeContext with constant details known at construction time

class PrecomputedCodeContext: public CodeContext

And move this declaration to src/base/PrecomputedCodeContext.{h,cc}.

rousskov · 2024-12-05T14:56:48Z

src/PeerPoolMgr.h

@@ -32,6 +56,8 @@ class PeerPoolMgr: public AsyncJob
    explicit PeerPoolMgr(CachePeer *aPeer);
    ~PeerPoolMgr() override;

+    DetailedCodeContext::Pointer context; ///< the pool manager context


Please rename -- there are too many contexts in this context :-).

Suggested change

DetailedCodeContext::Pointer context; ///< the pool manager context

DetailedCodeContext::Pointer codeContext;

I cannot think of any useful documentation for this self-documented declaration. The proposed description in current PR code reads like "foo is foo" to me. Existing official code has many self-documented codeContext data members, so I hope we do not have to describe this one either.

eduard-bagdasaryan

Done (7fd1491).

eduard-bagdasaryan · 2024-12-09T13:39:42Z

src/PeerPoolMgr.cc

    transportWait(),
    encryptionWait(),
    addrUsed(0)
 {
+    const auto mx = MasterXaction::MakePortless<XactionInitiator::initPeerPool>();
+    context = new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool: ", *peer), mx);


I added a new SBuf field (storing mx->id) instead.

eduard-bagdasaryan · 2024-12-09T13:45:38Z

src/PeerPoolMgr.h

@@ -32,6 +56,8 @@ class PeerPoolMgr: public AsyncJob
    explicit PeerPoolMgr(CachePeer *aPeer);
    ~PeerPoolMgr() override;

+    DetailedCodeContext::Pointer context; ///< the pool manager context


eduard-bagdasaryan · 2024-12-09T13:45:49Z

src/PeerPoolMgr.h

+
+    void setMasterXaction(const MasterXaction::Pointer &mx) { masterXaction = mx; }


eduard-bagdasaryan · 2024-12-09T13:53:18Z

src/PeerPoolMgr.h

+/// CodeContext for cache_peer related classes
+class DetailedCodeContext : public CodeContext


eduard-bagdasaryan · 2024-12-09T13:54:44Z

src/PeerPoolMgr.h

+public:
+    typedef RefCount<DetailedCodeContext> Pointer;
+
+    DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx) : gist_(gist),


src/PeerPoolMgr.cc

src/base/PrecomputedCodeContext.cc

rousskov

LGTM with two minor adjustments. Please resolve merge conflicts as well.

Please also review new PR title/description and adjust as needed.

rousskov · 2024-12-10T14:18:46Z

src/PeerPoolMgr.cc

+
+    codeContext = new PrecomputedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool: ", *peer,
+            Debug::Extra, "current master transaction: ", mx->id));
+    // ErrorState, getOutgoingAddress(), and other APIs may require a request.


Let's keep request-related code lines separate:

Suggested change

// ErrorState, getOutgoingAddress(), and other APIs may require a request.

// ErrorState, getOutgoingAddress(), and other APIs may require a request.

rousskov · 2024-12-10T14:22:37Z

src/base/PrecomputedCodeContext.h

+#include "base/InstanceId.h"
+#include "sbuf/SBuf.h"
+
+/// CodeContext with constant details known at construction time


IIRC, compiling this header code requires including <ostream> because this code uses "<<" expressions. Please include that STL header even if it is currently included (directly or indirectly) by the headers above.

Suggested change

/// CodeContext with constant details known at construction time

#include <ostream>

/// CodeContext with constant details known at construction time

IIRC, including <iosfwd> is not enough in such cases.

Use cache_peer context to report probe failures

e231283

Initial implementation.

rousskov requested changes Dec 2, 2024

View reviewed changes

eduard-bagdasaryan added 2 commits December 4, 2024 02:42

Reduced code duplication, introducing a new DetailedCodeContext

2ec9e72

Polished

30978d0

eduard-bagdasaryan commented Dec 3, 2024

View reviewed changes

rousskov requested changes Dec 4, 2024

View reviewed changes

eduard-bagdasaryan added 5 commits December 5, 2024 00:09

Added a missing colon

5615ab9

Moved the PeerPoolMgr job's context to CachePeer

3591b22

Adjusted a couple of CallService cases

656a526

Autoformatted

5c78fac

Partially undone 3591b22

665eb38

rousskov requested changes Dec 5, 2024

View reviewed changes

eduard-bagdasaryan added 3 commits December 9, 2024 16:51

Renamed and simplified DetailedCodeContext

2859bcc

Polished

824636d

Do not CallJobHere1() with a nil job

7fd1491

eduard-bagdasaryan commented Dec 9, 2024

View reviewed changes

rousskov requested changes Dec 9, 2024

View reviewed changes

src/PeerPoolMgr.cc Outdated Show resolved Hide resolved

src/base/PrecomputedCodeContext.cc Outdated Show resolved Hide resolved

eduard-bagdasaryan added 4 commits December 10, 2024 16:12

Simplified, eliminating PrecomputedCodeContext::masterXactionDetail_

78c9ddd

Removed an empty source

d896123

Declare PrecomputedCodeContext in forward.h

fe40f7e

Added a missing header

9df6b9e

rousskov changed the title ~~Use cache_peer context to report probe failures~~ Report cache_peer context in probe and standby pool messages Dec 10, 2024

rousskov approved these changes Dec 10, 2024

View reviewed changes

Minor polishing

a8e6877

	os << Debug::Extra << "current cache_peer probe: " << peer->name;
	os << Debug::Extra << "current cache_peer probe: " << *peer;

	context(new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool", *peer)))
	context(new DetailedCodeContext("cache_peer standby pool", ToSBuf("current cache_peer standby pool: ", *peer)))


		void setMasterXaction(const MasterXaction::Pointer &mx) { masterXaction = mx; }

	DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx) : gist_(gist),
	DetailedCodeContext(const char *gist, const SBuf &detail, const MasterXaction::Pointer &mx): gist_(gist),

		/// CodeContext for cache_peer related classes
		class DetailedCodeContext : public CodeContext

	DetailedCodeContext::Pointer context; ///< the pool manager context
	DetailedCodeContext::Pointer codeContext;

	// ErrorState, getOutgoingAddress(), and other APIs may require a request.

	// ErrorState, getOutgoingAddress(), and other APIs may require a request.

Report cache_peer context in probe and standby pool messages #277

Are you sure you want to change the base?

Report cache_peer context in probe and standby pool messages #277

Conversation

eduard-bagdasaryan commented Dec 2, 2024 • edited by rousskov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduard-bagdasaryan commented Dec 2, 2024 •

edited by rousskov

Loading