feat: Monitoring follower #119

usmanmani1122 · 2025-01-08T08:37:48Z

Description

This PR adds a new task spwanAcceptance which will run a follower for a chain and can be somewhat controlled by the validator on when to stop. More details in the updated README.

Muneeb147

@usmanmani1122
Can you point to the file(s) having actual changes and where loadgen code is removed?

For example one of them is: tasks/testnet.js where usage of makeLoadgenTask is removed.

Also let's have a follow-up issue/task to actually clean this up and remove redundant unused code.

Muneeb147

Do we need to also expose monitor in package.json scripts just like we have for loadgen and runner?

"runner": "runner/bin/loadgen-runner",
    "loadgen": "agoric deploy loadgen/loop.js"
    "monitor": ...

usmanmani1122 · 2025-01-21T17:08:40Z

Do we need to also expose monitor in package.json scripts just like we have for loadgen and runner?
"runner": "runner/bin/loadgen-runner",
    "loadgen": "agoric deploy loadgen/loop.js"
    "monitor": ...

No, since monitor uses yarn version 4.3.5 (opposed to 1.22.xx in root workspace)

usmanmani1122 · 2025-01-21T17:13:46Z

@usmanmani1122 Can you point to the file(s) having actual changes and where loadgen code is removed?

For example one of them is: tasks/testnet.js where usage of makeLoadgenTask is removed.

Also let's have a follow-up issue/task to actually clean this up and remove redundant unused code.

Only monitor/src/main.js and monitor/src/tasks/testnet.js have any changes
Rest of the files are copied
Changes outside the monitor folder were made in an attempt to pass the CI but it is still failing and I think the CI tests won't pass without making changes in the CI pipeline
@muhammadahmadasifbhatti can you make the follow up task (or add the link here if already created)?

muhammadahmadasifbhatti · 2025-01-21T17:17:16Z

@usmanmani1122 Can you point to the file(s) having actual changes and where loadgen code is removed?
For example one of them is: tasks/testnet.js where usage of makeLoadgenTask is removed.
Also let's have a follow-up issue/task to actually clean this up and remove redundant unused code.

Only monitor/src/main.js and monitor/src/tasks/testnet.js have any changes Rest of the files are copied Changes outside the monitor folder were made in an attempt to pass the CI but it is still failing and I think the CI tests won't pass without making changes in the CI pipeline @muhammadahmadasifbhatti can you make the follow up task (or add the link here if already created)?

I will create a task and add the link here. Can you please provide me with the JIRA project link?

usmanmani1122 · 2025-01-22T05:18:05Z

I will create a task and add the link here. Can you please provide me with the JIRA project link?

Use github issues

Muneeb147 · 2025-02-06T13:23:45Z

@usmanmani1122

This PR adds a monitoring follower (which is just the runner code copied with loadgen code commented out)

^ This is not happening now, right?

usmanmani1122 · 2025-02-08T06:18:27Z

@usmanmani1122

This PR adds a monitoring follower (which is just the runner code copied with loadgen code commented out)

^ This is not happening now, right?

Yes

Muneeb147 · 2025-02-13T12:17:16Z

runner/lib/main.js

+      };
+
+      if (currentStage === 1)
+        await writeMessageToMessageFile(stageConsole, 'ready');


minor suggestion:
Can we keep constants of messages?

const MESSAGES = { READY: 'ready', EXIT: 'exit code..' ... }

Muneeb147 · 2025-02-13T12:24:24Z

runner/lib/main.js

-          tasks.push(spawnClient, spawnLoadgen);
-        }
+        if (!chainOnly) tasks.push(spawnClient, spawnLoadgen);
+        else if (integrateAcceptance) tasks.push(spwanAcceptance);


I was thinking that we should only push spwanAcceptance when we have messageFilePath in env and fail early if not.

I think the flag should contain the path to the message file to avoid having 2 places to check.

Nit: not a fan of lack of curly braces here. Does the prettier / lint config even allow this?

mhofman

From integrating against a3p, I remember we needed to increase the trust period for state sync. Also I suspect that to support invoking the runner without an agoric install in the root, we'll need the patch I wrote to lookup for binaries in SDK_SRC. Both commits are in my standalone-runner branch

I would also love to see an end-to-end CI run with an updated Agoric/agoric-sdk#10816 (unless there is another agoric-sdk PR now?)

runner/lib/main.js

mhofman · 2025-02-14T04:34:17Z

runner/lib/main.js

    },
  });
+  const messageFilePath = process.env.MESSAGE_FILE_PATH;


I would somewhat prefer to pass this path to the command line flag.

mhofman · 2025-02-14T04:36:51Z

runner/lib/main.js

+      /**
+       * @param {RegExp} [regex]
+       */
+      const waitForMessageFromMessageFile = async (regex) => {


Let's move this one to the top where writeMessageToMessageFile is defined, and make it accept a console, Both these functions should actually use a new console:

const { console: acceptanceConsole } = makeConsole('acceptance', out, err);

mhofman · 2025-02-14T04:42:11Z

runner/lib/main.js

-          tasks.push(spawnClient, spawnLoadgen);
-        }
+        if (!chainOnly) tasks.push(spawnClient, spawnLoadgen);
+        else if (integrateAcceptance) tasks.push(spwanAcceptance);


I think the flag should contain the path to the message file to avoid having 2 places to check.

mhofman · 2025-02-14T04:42:56Z

runner/lib/main.js

-          tasks.push(spawnClient, spawnLoadgen);
-        }
+        if (!chainOnly) tasks.push(spawnClient, spawnLoadgen);
+        else if (integrateAcceptance) tasks.push(spwanAcceptance);


Nit: not a fan of lack of curly braces here. Does the prettier / lint config even allow this?

mhofman · 2025-02-14T04:46:27Z

runner/lib/main.js

      logPerfEvent('finish', { stats: runStats });
+      if (integrateAcceptance)
+        writeMessageToMessageFile(topConsole, `exit code ${Number(!!error)}`);


I'm skeptical of using the script itself to write the outcome to the message file. My concern is that if the runner fails for some reason that doesn't go through this, we may get the a3p test stuck.

Now of course if this is an optimistic report and we have a backup one in the launcher, I'm ok with this, but would prefer a comment to that effect.

Removed
I have moved it to the host script which will invoke the runner

mhofman · 2025-02-14T05:04:13Z

runner/lib/main.js

+    /**
+     * @type {Task}
+     *
+     * This task will always run in three stages


Suggested change

* This task will always run in three stages

* This task should always run in three stages

It doesn't seem to hurt if more stages are run, we just immediately complete the stage (after chain catch up of course)

usmanmani1122 · 2025-02-14T11:03:02Z

From integrating against a3p, I remember we needed to increase the trust period for state sync. Also I suspect that to support invoking the runner without an agoric install in the root, we'll need the patch I wrote to lookup for binaries in SDK_SRC. Both commits are in my standalone-runner branch

I would also love to see an end-to-end CI run with an updated Agoric/agoric-sdk#10816 (unless there is another agoric-sdk PR now?)

Done
I have triggered a new build with the changes, will add the link here once it passes
A build before the latest changes can be found here:
https://github.com/Agoric/agoric-sdk/actions/runs/13134138797/job/36645647192

usmanmani1122 · 2025-02-14T13:14:34Z

Successful build link:
https://github.com/Agoric/agoric-sdk/actions/runs/13329957603/job/37231493129

turadg · 2025-02-14T14:56:16Z

runner/lib/main.js

+    const cliHelpers = await import(srcHelpers)
+      .catch(() => import(libHelpers))
+      .catch((e) =>
+        process.env.SDK_SRC
+          ? import(
+              `${process.env.SDK_SRC}/packages/agoric-cli/${helpersSource}`
+            )
+          : Promise.reject(e),
+      );


because SDK_SRC is option someone has to enable, I would expect it to take precedence.

also I would suggest refactoring to,

Suggested change

const cliHelpers = await import(srcHelpers)

.catch(() => import(libHelpers))

.catch((e) =>

process.env.SDK_SRC

? import(

`${process.env.SDK_SRC}/packages/agoric-cli/${helpersSource}`

)

: Promise.reject(e),

);

const cliHelpers = await getCliHelpers();

It's a shame we have to deep import instead of having real exports of that package, but even if we added good exports we'd still need backwards compatibility so it would only add another case to this code.

In the past I've thought of adding a command to the agoric-cli to return these path. However I don't think it would help in this case since agoric would not be on the PATH. I'm open to suggestion on how to find entrypoint paths in agoric-sdk

because SDK_SRC is option someone has to enable, I would expect it to take precedence.

Yeah you're right. I originally implemented it as a fallback, but it should take precedence.

mhofman

Some nits about curly brace style, and the unexplained removal of change to the cometbft config to restore the log_level to default.

Besides that looks good for this side.

mhofman · 2025-02-17T04:49:41Z

runner/lib/main.js

+        if (regex && !regex.test(fileContent))
+          console.warn('Ignoring unsupported file content: ', fileContent);
+        else return fileContent;


Nit: curly braces.

mhofman · 2025-02-17T04:52:39Z

runner/lib/tasks/testnet.js

-        configP2p.persistent_peers = peers.join(',');
-        configP2p.seeds = seeds.join(',');
-        configP2p.addr_book_strict = false;
-        delete config.log_level;


Why was this line removed?

mhofman · 2025-02-17T05:27:01Z

runner/lib/tasks/types.d.ts

@@ -83,3 +83,123 @@ export interface OrchestratorTasks {
  runClient(options: TaskSwingSetOptions): Promise<RunClientResult>;
  runLoadgen(options: TaskBaseOptions): Promise<RunLoadgenResult>;
 }
+
+/* eslint-disable camelcase */
+export type CometBFTConfig = {


Out of curiosity, where is this coming from?

mhofman · 2025-02-17T05:38:22Z

Shoot this was on automerge and I didn't see it.

usmanmani1122 · 2025-02-17T08:04:23Z

Shoot this was on automerge and I didn't see it.

Yeah I also forgot to turn it off. Will address the remaining comments in a separate PR.

usmanmani1122 self-assigned this Jan 8, 2025

Muneeb147 reviewed Jan 21, 2025

View reviewed changes

usmanmani1122 closed this Feb 3, 2025

usmanmani1122 force-pushed the usman/monitoring-follower branch from f50cdf7 to 3952279 Compare February 3, 2025 09:29

acceptance task

a5503c7

usmanmani1122 reopened this Feb 3, 2025

usmanmani1122 added 9 commits February 4, 2025 12:20

agoric package

36ed0e2

downgrade version

ddb212c

fix stage

88b2801

bump ses + endo

29dc46c

fix exit code

0c93a58

nit

f4e712f

fix stages

9bdb677

revert

c1a9b1d

testing

64ec404

usmanmani1122 added 2 commits February 8, 2025 21:46

update function doc

b31ea4a

go version bump

e456542

usmanmani1122 requested review from mhofman and Muneeb147 February 8, 2025 22:14

usmanmani1122 marked this pull request as ready for review February 8, 2025 22:14

usmanmani1122 enabled auto-merge (squash) February 12, 2025 05:53

Muneeb147 reviewed Feb 13, 2025

View reviewed changes

turadg mentioned this pull request Feb 13, 2025

Yarn 4 compatibility #122

Merged

mhofman reviewed Feb 14, 2025

View reviewed changes

Your Name added 2 commits February 14, 2025 07:49

address matheiu comments

e7b2505

lint fixes

3b39c5f

usmanmani1122 requested a review from mhofman February 14, 2025 11:03

turadg reviewed Feb 14, 2025

View reviewed changes

mhofman approved these changes Feb 17, 2025

View reviewed changes

usmanmani1122 merged commit cbae159 into main Feb 17, 2025
3 checks passed

usmanmani1122 deleted the usman/monitoring-follower branch February 17, 2025 05:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Monitoring follower #119

feat: Monitoring follower #119

usmanmani1122 commented Jan 8, 2025 •

edited

Loading

Muneeb147 left a comment

Muneeb147 left a comment

usmanmani1122 commented Jan 21, 2025

usmanmani1122 commented Jan 21, 2025

muhammadahmadasifbhatti commented Jan 21, 2025

usmanmani1122 commented Jan 22, 2025

Muneeb147 commented Feb 6, 2025

usmanmani1122 commented Feb 8, 2025

Muneeb147 Feb 13, 2025

Muneeb147 Feb 13, 2025

mhofman Feb 14, 2025

mhofman Feb 14, 2025

mhofman left a comment

mhofman Feb 14, 2025

usmanmani1122 Feb 14, 2025

mhofman Feb 14, 2025

usmanmani1122 Feb 14, 2025

mhofman Feb 14, 2025

mhofman Feb 14, 2025

mhofman Feb 14, 2025

usmanmani1122 Feb 14, 2025

mhofman Feb 14, 2025

usmanmani1122 Feb 14, 2025

usmanmani1122 commented Feb 14, 2025 •

edited

Loading

usmanmani1122 commented Feb 14, 2025 •

edited

Loading

turadg Feb 14, 2025

mhofman Feb 17, 2025

mhofman Feb 17, 2025

mhofman left a comment

mhofman Feb 17, 2025

mhofman Feb 17, 2025

mhofman Feb 17, 2025

mhofman commented Feb 17, 2025

usmanmani1122 commented Feb 17, 2025

	* This task will always run in three stages
	* This task should always run in three stages

feat: Monitoring follower #119

feat: Monitoring follower #119

Conversation

usmanmani1122 commented Jan 8, 2025 • edited Loading

Description

Muneeb147 left a comment

Choose a reason for hiding this comment

Muneeb147 left a comment

Choose a reason for hiding this comment

usmanmani1122 commented Jan 21, 2025

usmanmani1122 commented Jan 21, 2025

muhammadahmadasifbhatti commented Jan 21, 2025

usmanmani1122 commented Jan 22, 2025

Muneeb147 commented Feb 6, 2025

usmanmani1122 commented Feb 8, 2025

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

mhofman 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

usmanmani1122 commented Feb 14, 2025 • edited Loading

usmanmani1122 commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman 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

mhofman commented Feb 17, 2025

usmanmani1122 commented Feb 17, 2025

usmanmani1122 commented Jan 8, 2025 •

edited

Loading

usmanmani1122 commented Feb 14, 2025 •

edited

Loading

usmanmani1122 commented Feb 14, 2025 •

edited

Loading