Send Progrock updates to Cloud #5297

vito · 2023-06-07T17:33:06Z

This is an incremental step that maintains compatibility with the Dagger Cloud API:

Progrock Groups are mapped to their equivalent pipeline paths.
Vertex <-> Group membership is handled client-side; an Op will be emitted for ~~every~~ the first group membership as soon as both pieces of information are known.

In the long run we might want to store groups as first-class entities in Cloud to aid with querying and knowing the actual start/end of a pipeline, but that's a bigger lift.

It also:

Removes all of the pre-Progrock progress infra, so we're no longer injecting pipeline paths into Buildkit ProgressGroups
Updates our one-off tools (cmd/{upload-journal,dagger-graph,otel-collector}) - though I fully admit I haven't tested these
Adds the cloud URL and engine name to the TUI

Fixes DAG-1087

vito

Ready for review! Did a review of my own and left some comments below, hope it helps.

vito · 2023-06-09T20:28:45Z

core/container.go

-		Name: pipelineName,
-	})
-	ctx, subRecorder := progrock.WithGroup(ctx, pipelineName)
+	ctx, subRecorder := progrock.WithGroup(ctx, fmt.Sprintf("from %s", addr), progrock.Weak())


Marking a pipeline as "weak" allows us to differentiate these implicitly-added pipelines from explicit user-configured ones.

vito · 2023-06-09T20:29:51Z

core/container.go

-	Pipeline pipeline.Path
-}
-
-func (r PipelineMetaResolver) ResolveImageConfig(ctx context.Context, ref string, opt llb.ResolveImageConfigOpt) (digest.Digest, []byte, error) {


This is handled by (core.GatewayClient).ResolveImageConfig now.

vito · 2023-06-09T20:31:26Z

core/container.go

@@ -1110,7 +1079,7 @@ func (container *Container) WithExec(ctx context.Context, gw bkgw.Client, progSo
 	// create /dagger mount point for the shim to write to
 	runOpts = append(runOpts,
 		llb.AddMount(metaMountDestPath,
-			llb.Scratch().File(meta, pipeline.CustomName{Name: "creating dagger metadata", Internal: true}.LLBOpt(), container.Pipeline.LLBOpt()),
+			llb.Scratch().File(meta, llb.WithCustomName("[internal] creating dagger metadata")),


This is the one spot where we want to mark a Buildkit vertex as "internal." We don't need to shove the whole pipeline path in there anymore, so I swapped it out for a [internal] prefix, which gets stripped off and turned into internal: true during the Buildkit => Progrock conversion.

vito · 2023-06-09T20:33:07Z

core/directory.go

@@ -256,18 +256,15 @@ func (dir *Directory) WithNewFile(ctx context.Context, dest string, content []by

 	parent, _ := path.Split(dest)
 	if parent != "" {
-		st = st.File(llb.Mkdir(parent, 0755, llb.WithParents(true)), dir.Pipeline.LLBOpt())


Don't need to pass pipeline info to Buildkit anymore; group <-> vertex membership is handled by core.GatewayClient.

vito · 2023-06-09T20:34:40Z

core/pipeline/label.go

@@ -64,9 +64,6 @@ func loadRootLabels(workdir, engineName string) []Label {
 	return labels
 }

-// ServiceHostnameLabel is annotated on all services started by Dagger.
-const ServiceHostnameLabel = "dagger.io/service.hostname"


This originally existed so we can detect service vertices and treat them differently, which shouldn't be needed once we convert it to the gateway. I don't think this ended up being load-bearing, so just removing it along the way.

vito · 2023-06-09T21:01:29Z

engine/engine.go

@@ -383,58 +334,6 @@ func eventsMultiReader(ch chan *bkclient.SolveStatus, readers ...chan *bkclient.
 	}
 }

-func uploadTelemetry(ch chan *bkclient.SolveStatus) error {


This has moved to telemetry.Writer.

This is an incremental step that maintains compatibility with the Dagger Cloud API. Groups are mapped to pipeline paths. (TODO: pipeline descriptions are currently droped.) Vertex <-> Group membership is handled client-side; an Op will be emitted for every group membership. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

this was copy-pasta from Bass, not intended to be preserved in Dagger Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

this is used for engine logs - need to dedupe these, or maybe use one in the other Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

'weak' pipelines are ones that Dagger automatically adds, or a Buildkit frontend adds via a ProgressGroup. They are treated differently from user-specified pipelines. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Too difficult to figure out many-to-many vertex <-> groups in the frontend. This is much simpler for now. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

* remove JournalWriter (unused) * remove internal/engine/journal/ (outdated) Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

* add telemetry.Pipeliner for tying vertices to pipeline.Paths Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

trivial now that there's telemetry.Writer Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

We're no longer putting this information into Buildkit, so the console output no longer shows pipelines and such. To be clear, this format was always a bit janky, often resulting in things like blank vertex names. These tests can be brought back once Progrock has a proper console output format. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

this was a temporary hack to show the Dagger Cloud URL in the TUI. will figure this out in a separate PR. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

no need to tie this 100% to Buildkit events, in case we want to create any other vertexes Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

* --silent no longer configures console output format, which was frankly confusing. --progress has been added instead. * removed buildkit console format; we are now 100% Progrock! * show Cloud URL and engine name in TUI Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

the Progrock socket should always be available, so this is pretty pointless anyway Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

sipsma

LGTM, super minor comments/questions

sipsma · 2023-06-15T22:41:46Z

telemetry/telemetry.go

-	t.mu.Lock()
-	defer t.mu.Unlock()
+	if t.token != "" {
+		// no token; don't send telemetry


is this comment supposed to be outside this if?

Fixed! Not sure what happened here, probably inverted the conditional or something.

sipsma · 2023-06-15T23:02:30Z

cmd/dagger/engine.go

+	if !silent {
+		if progress == "auto" && autoTTY || progress == "tty" {
+			if interactive {
+				return interactiveTUI(ctx, engineConf, fn)


Do we not set the EngineName and CloudURL callbacks for the interactive TUI? If so, is that just an oversight or is it because we'll need more updates to display those in the interactive tui? Not a big deal either way, just checking

Needs more updates; will probably do it in another PR as this one's already grown quite a bit.

sipsma · 2023-06-15T23:49:32Z

telemetry/pipeliner.go

+	if group.Parent != nil {
+		parentPath, found := t.pipelinePaths[group.GetParent()]
+		if found {
+			path = append(path, parentPath...)


extreme nit, probably separate issue if one at all: is there harm in changing pipeline.Path to be type []*Pipeline? It just looks like we are using quadratic memory here by re-copying each parent path, which I guess could plausibly matter in extreme scenarios with many long pipelines.

Done! I don't think this matters too much since it only happens when a group starts or finishes, and the largest chunk of data (Labels) is already by-reference, but it's a tiny tweak and every bit helps.

pretty tiny change that should reduce some memory pressure, since each of these values never change. (this likely has very little actual effect, just seems sensible.) Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

vito force-pushed the cloud-progrock branch 2 times, most recently from c007389 to 00aa067 Compare June 9, 2023 20:01

vito marked this pull request as ready for review June 9, 2023 20:08

vito force-pushed the cloud-progrock branch from 00aa067 to e61f34b Compare June 9, 2023 20:51

vito commented Jun 9, 2023

View reviewed changes

vito requested review from sipsma and marcosnils June 9, 2023 21:02

vito force-pushed the cloud-progrock branch 2 times, most recently from 22caf86 to 3498067 Compare June 9, 2023 21:07

This was referenced Jun 15, 2023

Host export support for project commands #5157

Merged

Breaking API change: run services via gateway, revise API #5322

Closed

vito added 19 commits June 15, 2023 16:39

preserve pipeline descriptions through progrock

e68be8d

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

remove erroneous [hide] handling

8376bd3

this was copy-pasta from Bass, not intended to be preserved in Dagger Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

clean up pre-Progrock progress support

d5b351a

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

bump progrock for minor UI fixes

c3d40b6

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

bring back telemetry.Telemetry

c09cbe0

this is used for engine logs - need to dedupe these, or maybe use one in the other Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

support 'weak' pipelines

d3768bf

'weak' pipelines are ones that Dagger automatically adds, or a Buildkit frontend adds via a ProgressGroup. They are treated differently from user-specified pipelines. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

limit vertices to one group for now

0707db1

Too difficult to figure out many-to-many vertex <-> groups in the frontend. This is much simpler for now. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

JournalFile writes progrock updates

846561d

* remove JournalWriter (unused) * remove internal/engine/journal/ (outdated) Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

convert otel-collector to progrock

7c50ae2

* add telemetry.Pipeliner for tying vertices to pipeline.Paths Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

convert upload-journal to progrock

2b1c0d1

trivial now that there's telemetry.Writer Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

convert dagger-graph to progrock

2449ab2

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

use telemetry.Pipeliner as part of Writer

a159ad2

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

telemetry: consolidate Telemetry and Writer

d87250d

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

get rid of temporary 'meta' vertex

cc92759

this was a temporary hack to show the Dagger Cloud URL in the TUI. will figure this out in a separate PR. Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

promote recorder completion to engine.Start

545182b

no need to tie this 100% to Buildkit events, in case we want to create any other vertexes Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

shim: don't use LogOutput during nesting

dda00ce

the Progrock socket should always be available, so this is pretty pointless anyway Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

vito added 2 commits June 15, 2023 16:39

appease linter

a871f29

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

remove pre-progrock code from Host.File

9dfe622

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

vito force-pushed the cloud-progrock branch from 89e310c to 9dfe622 Compare June 15, 2023 20:40

log console output from 'dagger session'

613d0fa

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

sipsma approved these changes Jun 15, 2023

View reviewed changes

vito added 2 commits June 16, 2023 09:38

pipeline.Path: change to []*Pipeline

f3f65ca

pretty tiny change that should reduce some memory pressure, since each of these values never change. (this likely has very little actual effect, just seems sensible.) Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

fix comment

4fe76bb

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>

vito merged commit 7d1c04a into dagger:main Jun 16, 2023
21 checks passed

vito deleted the cloud-progrock branch June 16, 2023 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send Progrock updates to Cloud #5297

Send Progrock updates to Cloud #5297

vito commented Jun 7, 2023 •

edited

vito left a comment

vito Jun 9, 2023

vito Jun 9, 2023

vito Jun 9, 2023

vito Jun 9, 2023

vito Jun 9, 2023

vito Jun 9, 2023

sipsma left a comment

sipsma Jun 15, 2023

vito Jun 16, 2023

sipsma Jun 15, 2023

vito Jun 16, 2023

sipsma Jun 15, 2023

vito Jun 16, 2023

Send Progrock updates to Cloud #5297

Send Progrock updates to Cloud #5297

Conversation

vito commented Jun 7, 2023 • edited

vito 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

sipsma 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

vito commented Jun 7, 2023 •

edited