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.

Sign up for GitHub

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

Refactor the basic manager Action classes for Action::report() #1560

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Refactor the basic manager Action classes for Action::report() #1560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bb1f53d
Add Cache Manager stream output API
yadij Oct 31, 2023
b0d03bc
Upgrade basic Actions to new display API
yadij Oct 31, 2023
02bcd49
Apply suggestions from code review
yadij Nov 3, 2023
9679054
Rename print() to report()
yadij Nov 8, 2023
646b162
Extra polish
yadij Nov 8, 2023
fcf81af
MOre polish requested by reviewer
yadij Nov 8, 2023
30e3170
More polish requested by reviewer
yadij Nov 8, 2023
77d4df2
Restore old column widths
yadij Nov 9, 2023
2fd67ba
Merge branch 'master' into arc-mgr-ostream-1
yadij Nov 12, 2023
bbff3af
Update Action.h
yadij May 7, 2024
9b3482e
Merge branch 'master' into arc-mgr-ostream-1
yadij May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/mgr/Action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
/* DEBUG: section 16 Cache Manager API */

#include "squid.h"
#include "base/PackableStream.h"
#include "CacheManager.h"
#include "comm/Connection.h"
#include "HttpReply.h"
Expand Down Expand Up @@ -112,11 +113,18 @@ Mgr::Action::fillEntry(StoreEntry* entry, bool writeHttpHeader)
entry->replaceHttpReply(rep);
}

dump(entry);
dump(entry); // TODO: replace with report() when all children are converted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the PR-proposed "just write the report" semantics, it is not possible to convert some of the existing Mgr::Action children (and some future ones). For example, Mgr::FunAction objects that handle asynchronous mgr:objects and mgr:vm_objects reports cannot be converted because their existing dump() methods do more than "just writing the report" and, in non-SMP cases1, do not "write the report" at all. The guts of their dump() actions are essentially one no-output line:

    eventAdd("statObjects", statObjects, state, 0.0, 1);

The fundamental property of asynchronous cache manager actions cannot be removed by conversion or refactoring that might be relevant to this TODO. This TODO goes against core Action architecture/features that require that asynchronous production of some cache manager reports, including dump() calls that may not produce output (because an asynchronous action is required to start producing it) or do not finish producing output (again, because an asynchronous action is required to continue and finish producing output).

N.B. There is more about this problem in the other change request. I am not sure which change request has a better context for this discussion. The other change request has a specific suggestion for addressing both problems.

Footnotes

  1. In the corresponding SMP cases, FunAction::dump() writes a "by kidN {" prefix. ↩

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those bad designs need to be replaced and are part of why this TODO has a condition ("when ...") stated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex: With the PR-proposed "just write the report" semantics, it is not possible to convert some of the existing Mgr::Action children (and some future ones). ... The fundamental property of asynchronous cache manager actions cannot be removed by conversion or refactoring that might be relevant to this TODO. This TODO goes against core Action architecture/features that ...

Amos: Those bad designs need to be replaced and are part of why this TODO has a condition ("when ...") stated.

This change request has already evaluated and rejected that condition, providing detailed reasoning and examples. No new information or arguments have been provided in response to that change request. There is currently no consensus that any future hypothetical refactoring (unspecified in this PR) can justify this TODO existence.


entry->flush();

if (atomic())
entry->complete();
}

void
Mgr::Action::dump(StoreEntry *entry)
{
PackableStream os(*entry);
report(os);
}

7 changes: 6 additions & 1 deletion src/mgr/Action.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include "ipc/forward.h"
#include "mgr/forward.h"

#include <iosfwd>

class StoreEntry;

namespace Mgr
Expand Down Expand Up @@ -76,11 +78,14 @@ class Action: public RefCountable
/// calculate and keep local action-specific information
virtual void collect() {}

/// write manager report output to a stream.
rousskov marked this conversation as resolved.
Show resolved Hide resolved
virtual void report(std::ostream &) {}

/** start writing action-specific info to Store entry;
* may collect info during dump, especially if collect() did nothing
* non-atomic() actions may continue writing asynchronously after returning
*/
virtual void dump(StoreEntry *) {}
virtual void dump(StoreEntry *);
kinkie marked this conversation as resolved.
Show resolved Hide resolved

private:
const CommandPointer cmd; ///< the command that caused this action
Expand Down
45 changes: 22 additions & 23 deletions src/mgr/BasicActions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,6 @@ Mgr::IndexAction::IndexAction(const Command::Pointer &aCmd): Action(aCmd)
debugs(16, 5, MYNAME);
}

void
Mgr::IndexAction::dump(StoreEntry *)
{
debugs(16, 5, MYNAME);
}

Mgr::MenuAction::Pointer
Mgr::MenuAction::Create(const Command::Pointer &cmd)
{
Expand All @@ -47,20 +41,26 @@ Mgr::MenuAction::MenuAction(const Command::Pointer &aCmd): Action(aCmd)
debugs(16, 5, MYNAME);
}

/// A table summarizing available Cache Manager actions:
/// table-row = SP 1*VCHAR 1*( HTAB 0*VCHAR )
void
Mgr::MenuAction::dump(StoreEntry* entry)
Mgr::MenuAction::report(std::ostream &os)
{
debugs(16, 5, MYNAME);
yadij marked this conversation as resolved.
Show resolved Hide resolved
Must(entry != nullptr);
const auto &menu = CacheManager::GetInstance()->menu();

typedef CacheManager::Menu::const_iterator Iterator;
const CacheManager::Menu& menu = CacheManager::GetInstance()->menu();
const auto savedFlags = os.flags();
const auto savedFill = os.fill();

for (Iterator a = menu.begin(); a != menu.end(); ++a) {
storeAppendPrintf(entry, " %-22s\t%-32s\t%s\n",
(*a)->name, (*a)->desc,
CacheManager::GetInstance()->ActionProtection(*a));
os << std::left;
for (const auto &a : menu) {
os << ' ' << std::setw(22) << a->name << std::setw(0)
<< '\t' << std::setw(32) << a->desc << std::setw(0)
<< '\t' << CacheManager::GetInstance()->ActionProtection(a)
<< '\n';
}

os.fill(savedFill);
os.flags(savedFlags);
}

Mgr::ShutdownAction::Pointer
Expand All @@ -75,7 +75,7 @@ Mgr::ShutdownAction::ShutdownAction(const Command::Pointer &aCmd): Action(aCmd)
}

void
Mgr::ShutdownAction::dump(StoreEntry *)
Mgr::ShutdownAction::report(std::ostream &)
{
debugs(16, DBG_CRITICAL, "Shutdown by Cache Manager command.");
shut_down(SIGTERM);
Expand All @@ -94,10 +94,10 @@ Mgr::ReconfigureAction::ReconfigureAction(const Command::Pointer &aCmd):
}

void
Mgr::ReconfigureAction::dump(StoreEntry* entry)
Mgr::ReconfigureAction::report(std::ostream &os)
{
debugs(16, DBG_IMPORTANT, "Reconfigure by Cache Manager command.");
storeAppendPrintf(entry, "Reconfiguring Squid Process ....");
os << "Reconfiguring Squid Process ... \n";
reconfigure(SIGHUP);
}

Expand All @@ -113,10 +113,10 @@ Mgr::RotateAction::RotateAction(const Command::Pointer &aCmd): Action(aCmd)
}

void
Mgr::RotateAction::dump(StoreEntry* entry)
Mgr::RotateAction::report(std::ostream &os)
{
debugs(16, DBG_IMPORTANT, "Rotate Logs by Cache Manager command.");
storeAppendPrintf(entry, "Rotating Squid Process Logs ....");
os << "Rotating Squid Process Logs ... \n";
#if defined(_SQUID_LINUX_THREADS_)
rotate_logs(SIGQUIT);
#else
Expand All @@ -137,13 +137,12 @@ Mgr::OfflineToggleAction::OfflineToggleAction(const Command::Pointer &aCmd):
}

void
Mgr::OfflineToggleAction::dump(StoreEntry* entry)
Mgr::OfflineToggleAction::report(std::ostream &os)
{
Config.onoff.offline = !Config.onoff.offline;
debugs(16, DBG_IMPORTANT, "offline_mode now " << (Config.onoff.offline ? "ON" : "OFF") << " by Cache Manager request.");

storeAppendPrintf(entry, "offline_mode is now %s\n",
Config.onoff.offline ? "ON" : "OFF");
os << "offline_mode is now " << (Config.onoff.offline ? "ON" : "OFF") << '\n';
}

void
Expand Down
12 changes: 6 additions & 6 deletions src/mgr/BasicActions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class IndexAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override {}

protected:
IndexAction(const CommandPointer &cmd);
Expand All @@ -39,7 +39,7 @@ class MenuAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override;

protected:
MenuAction(const CommandPointer &cmd);
Expand All @@ -51,7 +51,7 @@ class ShutdownAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override;

protected:
ShutdownAction(const CommandPointer &cmd);
Expand All @@ -63,7 +63,7 @@ class ReconfigureAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override;

protected:
ReconfigureAction(const CommandPointer &cmd);
Expand All @@ -75,7 +75,7 @@ class RotateAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override;

protected:
RotateAction(const CommandPointer &cmd);
Expand All @@ -87,7 +87,7 @@ class OfflineToggleAction: public Action
public:
static Pointer Create(const CommandPointer &cmd);
/* Action API */
void dump(StoreEntry *entry) override;
void report(std::ostream &) override;

protected:
OfflineToggleAction(const CommandPointer &cmd);
Expand Down
11 changes: 6 additions & 5 deletions src/tests/stub_libmgr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const char * Mgr::Action::name() const STUB_RETVAL(nullptr)
static Mgr::Command static_Command;
const Mgr::Command & Mgr::Action::command() const STUB_RETVAL(static_Command)
StoreEntry * Mgr::Action::createStoreEntry() const STUB_RETVAL(nullptr)
void Mgr::Action::dump(StoreEntry *) STUB
static Mgr::Action::Pointer dummyAction;

#include "mgr/ActionParams.h"
Expand All @@ -48,27 +49,27 @@ void Mgr::ActionWriter::start() STUB

#include "mgr/BasicActions.h"
Mgr::Action::Pointer Mgr::MenuAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::MenuAction::dump(StoreEntry *) STUB
void Mgr::MenuAction::report(std::ostream &) STUB
//protected:
//Mgr::MenuAction::MenuAction(const CommandPointer &cmd) STUB

Mgr::Action::Pointer Mgr::ShutdownAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::ShutdownAction::dump(StoreEntry *) STUB
void Mgr::ShutdownAction::report(std::ostream &) STUB
// protected:
//Mgr::ShutdownAction::ShutdownAction(const CommandPointer &) STUB

Mgr::Action::Pointer Mgr::ReconfigureAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::ReconfigureAction::dump(StoreEntry *) STUB
void Mgr::ReconfigureAction::report(std::ostream &) STUB
//protected:
//Mgr::ReconfigureAction::ReconfigureAction(const CommandPointer &) STUB

Mgr::Action::Pointer Mgr::RotateAction::Create(const Mgr::CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::RotateAction::dump(StoreEntry *) STUB
void Mgr::RotateAction::report(std::ostream &) STUB
//protected:
//Mgr::RotateAction::RotateAction(const CommandPointer &) STUB

Mgr::Action::Pointer Mgr::OfflineToggleAction::Create(const CommandPointer &) STUB_RETVAL(dummyAction)
void Mgr::OfflineToggleAction::dump(StoreEntry *) STUB
void Mgr::OfflineToggleAction::report(std::ostream &) STUB
//protected:
//Mgr::OfflineToggleAction::OfflineToggleAction(const CommandPointer &) STUB

Expand Down
Loading