`agd export` option for diagnosis #10344

usmanmani1122 · 2024-10-28T13:26:13Z

Description

This PR adds a new optional flag --swing-store-export-mode to the agd export command which can be used to specify the nature of swing-store export

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

An optional flag --swing-store-export-mode option is now supported for agd export to specify the kind of swing store export required:

operational (default) option will retain the existing behavior and export the swing-store artifacts using the latest height
skip option will not export any swing-store artifact
debug option will export all swing-store artifacts, starting from height 0

Testing Considerations

Tested manually

Upgrade Considerations

None

cloudflare-workers-and-pages · 2024-10-28T13:31:15Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`cda65f9`
Status:	✅ Deploy successful!
Preview URL:	https://54559351.agoric-sdk.pages.dev
Branch Preview URL:	https://usman-8420.agoric-sdk.pages.dev

View logs

usmanmani1122 · 2024-10-31T11:54:03Z

golang/cosmos/daemon/cmd/root.go

+	}
+
+	// We don't have to launch VM in case the swing store export is not required
+	if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip || OnExportHook == nil) {


@mhofman since OnExportHook currently points to launchVM, is it okay to bypass this hook in case swing-store export is not needed? Or do you see us adding cosmic side future hook changes?

I think that's fine, but I'd like @michaelfig to confirm, especially with his upcoming changed for split brains. Basically export only needs to launch the VM if it will export the swing-store, which is decided by a command line config. Since OnExportHook is unconditionally set or not based on the entrypoint used, the only option we have is to ignore it here.

Btw, my brain still struggles with distributing negations.

Suggested change

if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip || OnExportHook == nil) {

if swingStoreExportMode != swingsetkeeper.SwingStoreArtifactModeSkip && OnExportHook != nil {

Muneeb147 · 2024-10-31T14:46:29Z

golang/cosmos/x/swingset/genesis.go

+				artifactsEnded = true
+				if eventHandler.exportMode == keeper.SwingStoreArtifactModeDebug {
+					err = nil
+					exportDataReader, err := getExportDataReader()


Do we need to close this reader in defer?
defer exportDataReader.Close()

I thought EncodeKVEntryReaderToJsonl function will close the reader passed to it at the end but it seems like it doesn't

Right, EncodeKVEntryReaderToJsonl doesn't close the reader. I think that was because I wanted the reader to be closed even if a read error occurred, and leave the caller in charge of handling potentially double errors. I'm not sure if that's the idiomatic golang thing to do, and would be open to reconsider if it's not.

Muneeb147 · 2024-10-31T15:26:24Z

golang/cosmos/x/swingset/genesis.go

+							artifact = types.SwingStoreArtifact{
+								Data: encodedExportData.Bytes(),
+								Name: keeper.UntrustedExportDataArtifactName,
+							}
+						}
+					}
+				}
+			}


So in-case exportMode is "debug", we are sending an extra artifact at the end which is a full export of swingStore (Kvstore)? Right? Assuming that caller iterates over ReadNextArtifact until gets io.EOF.

mhofman

Thanks for digging through this. The main issue is that we don't currently write out the "export data" as "untrusted copy" in debug mode, but instead we-write the cosmos DB copy.

golang/cosmos/app/app.go

golang/cosmos/daemon/cmd/root.go

mhofman · 2024-11-08T17:49:05Z

golang/cosmos/daemon/cmd/root.go

+	}
+
+	// We don't have to launch VM in case the swing store export is not required
+	if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip || OnExportHook == nil) {


I think that's fine, but I'd like @michaelfig to confirm, especially with his upcoming changed for split brains. Basically export only needs to launch the VM if it will export the swing-store, which is decided by a command line config. Since OnExportHook is unconditionally set or not based on the entrypoint used, the only option we have is to ignore it here.

mhofman · 2024-11-08T17:50:12Z

golang/cosmos/daemon/cmd/root.go

+	}
+
+	// We don't have to launch VM in case the swing store export is not required
+	if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip || OnExportHook == nil) {


Btw, my brain still struggles with distributing negations.

Suggested change

if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip || OnExportHook == nil) {

if swingStoreExportMode != swingsetkeeper.SwingStoreArtifactModeSkip && OnExportHook != nil {

golang/cosmos/x/swingset/genesis.go

mhofman · 2024-11-08T18:15:52Z

golang/cosmos/x/swingset/genesis.go

+					err = agoric.EncodeKVEntryReaderToJsonl(
+						exportDataReader,
+						&encodedExportData,
+					)
+					if err == nil {
+						artifact = types.SwingStoreArtifact{
+							Data: encodedExportData.Bytes(),
+							Name: keeper.UntrustedExportDataArtifactName,
+						}
+					}


It really is a bit unfortunate we have to read, decode, re-encode it all in memory before finally writing it out, but streamlining this is definitely out of scope.

golang/cosmos/x/swingset/keeper/swing_store_exports_handler.go

golang/cosmos/x/swingset/genesis.go

mhofman

Assuming my final nit is addressed, feel free to merge this. Thanks for working on this.

mhofman · 2024-11-18T17:19:18Z

golang/cosmos/x/swingset/keeper/swing_store_exports_handler.go

+const (
+	// SwingStoreExportModeSkip indicates swing store data should be
+	// excluded from the export.
+	SwingStoreExportModeSkip = "skip"
+
+	// SwingStoreExportModeOperational (default) indicates export should
+	// have the minimal set of artifacts needed to operate a node.
+	SwingStoreExportModeOperational = "operational"
+
+	// SwingStoreExportModeDebug indicates export should have the maximal
+	// set of artifacts available in the JS swing-store.
+	SwingStoreExportModeDebug = "debug"
+)
+


Let's move these to genesis.go as it's only applicable to that logic.

mhofman · 2024-11-18T17:27:35Z

golang/cosmos/x/swingset/genesis.go

+				artifactsEnded = true
+				if eventHandler.exportMode == keeper.SwingStoreArtifactModeDebug {
+					err = nil
+					exportDataReader, err := getExportDataReader()


Right, EncodeKVEntryReaderToJsonl doesn't close the reader. I think that was because I wanted the reader to be closed even if a read error occurred, and leave the caller in charge of handling potentially double errors. I'm not sure if that's the idiomatic golang thing to do, and would be open to reconsider if it's not.

usmanmani1122 added 6 commits October 7, 2024 12:51

testing

0a4c1f0

Merge branch 'master' into usman/8420

4467bd6

pass flag value ahead to swingset

e5b1b86

Merge branch 'master' into usman/8420

8bd66da

handle export data modes

10cfea3

revert logs

c1d3cdb

usmanmani1122 self-assigned this Oct 28, 2024

usmanmani1122 added 5 commits October 28, 2024 20:15

Merge branch 'master' into usman/8420

36a10d6

nit

8811fc0

Merge branch 'master' into usman/8420

980037a

Merge branch 'master' into usman/8420

6daf159

disable vm launch on skip mode

1ad8be5

usmanmani1122 commented Oct 31, 2024

View reviewed changes

usmanmani1122 requested a review from mhofman October 31, 2024 12:07

usmanmani1122 marked this pull request as ready for review October 31, 2024 12:07

usmanmani1122 requested a review from a team as a code owner October 31, 2024 12:07

Merge branch 'master' into usman/8420

c4be539

Muneeb147 reviewed Oct 31, 2024

View reviewed changes

usmanmani1122 and others added 3 commits November 2, 2024 08:03

Merge branch 'master' into usman/8420

cc6cbd0

close reade

3928c4a

Merge branch 'master' into usman/8420

65e4656

mhofman requested changes Nov 8, 2024

View reviewed changes

usmanmani1122 added 2 commits November 11, 2024 08:26

Merge branch 'master' into usman/8420

d2759e2

addressed Mathieu comments

483c566

usmanmani1122 requested a review from mhofman November 11, 2024 10:09

usmanmani1122 added 2 commits November 12, 2024 07:32

Merge branch 'master' into usman/8420

f3c69b3

use new constants

530a2b5

mhofman approved these changes Nov 18, 2024

View reviewed changes

Merge branch 'master' into usman/8420

f97d3da

move constants to genesis file

cda65f9

usmanmani1122 added the automerge:squash Automatically squash merge label Nov 21, 2024

mergify bot merged commit 6112a8b into master Nov 21, 2024
91 checks passed

mergify bot deleted the usman/8420 branch November 21, 2024 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`agd export` option for diagnosis #10344

`agd export` option for diagnosis #10344

usmanmani1122 commented Oct 28, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 28, 2024 •

edited

Loading

usmanmani1122 Oct 31, 2024

mhofman Nov 8, 2024

mhofman Nov 8, 2024

Muneeb147 Oct 31, 2024

usmanmani1122 Nov 2, 2024

mhofman Nov 18, 2024

Muneeb147 Oct 31, 2024 •

edited

Loading

mhofman left a comment

mhofman Nov 8, 2024

mhofman Nov 8, 2024

mhofman Nov 8, 2024

mhofman left a comment

mhofman Nov 18, 2024

mhofman Nov 18, 2024

	if !(swingStoreExportMode == swingsetkeeper.SwingStoreArtifactModeSkip \|\| OnExportHook == nil) {
	if swingStoreExportMode != swingsetkeeper.SwingStoreArtifactModeSkip && OnExportHook != nil {

agd export option for diagnosis #10344

agd export option for diagnosis #10344

Conversation

usmanmani1122 commented Oct 28, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 28, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

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

Muneeb147 Oct 31, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`agd export` option for diagnosis #10344

`agd export` option for diagnosis #10344

usmanmani1122 commented Oct 28, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 28, 2024 •

edited

Loading

Muneeb147 Oct 31, 2024 •

edited

Loading