dashboard/dashapi: run syz-ci without a dashboard

[glpesk]

syz-ci calls several Dashboard methods, and the case of a nil Dashboard pointer receiver was not handled. This change allows syz-ci to operate with a nil dashboard. (In terms of the syzbot setup docs, we wanted to deploy a test instance of syz-ci without deploying a dashboard.)

[a-nogikh]

I think it would be better to patch it it in the syz-ci code rather than in dashapi.

[glpesk]

Thanks for the review - I'll get back to this PR soon. Before I make changes, I just wanted to explain my reasoning and double-check: based on https://go.dev/tour/methods/12, it sounds like interface methods with a pointer receiver often implement their own handling for the case of a nil receiver. I will go with whatever you prefer, though.

[a-nogikh]

We already divert the logic in case of dash == nil in syz-ci:

syzkaller/syz-ci/manager.go

Lines 535 to 539 in 5643e0e

	if mgr.dash == nil {
	log.Logf(0, "%v: image testing failed: %v\n\n%s\n\n%s",
	mgr.name, rep.Title, rep.Report, rep.Output)
	return nil
	}

syzkaller/syz-ci/manager.go

Lines 639 to 646 in 5643e0e

	if mgr.dash == nil {
	// Dashboard identifies builds by unique tags that are combined
	// from kernel tag, compiler tag and config tag.
	// This combined tag is meaningless without dashboard,
	// so we use kenrel tag (commit tag) because it communicates
	// at least some useful information.
	return info.KernelCommit, nil
	}

Over time we probably just broke it since we don't use this mode anywhere and there are almost no syz-ci tests.

dash == nil disables significant chunks of syz-ci functionality, I think it's better to not even try to run those chunks of code in the first place rather that let them run with a dummy Dashapi implementation.

[glpesk]

We already divert the logic in case of dash == nil in syz-ci

Just a note on this -- it was originally surprising to me that these dash == nil checks didn't result in early returns in the case of a nil dashboard. The explanation turned out to be in that golang reference above (https://go.dev/tour/methods/12): "Note that an interface value that holds a nil concrete value is itself non-nil". Since dash is an interface value, dash != nil even when the dashboard pointer is nil.

[a-nogikh]

Indeed!

I think the bug was introduced recently with the ManagerDashapi interface -- before that it was only operating with *dashapi.Dashboard objects.

[glpesk]

Hi! Hoping I can catch you today for a question:

Would you like DashboardAddr, DashboardClient, and DashboardKey to continue to be optional fields of syz-ci's Config as noted here?

syzkaller/syz-ci/syz-ci.go

Line 94 in 96eb609

DashboardAddr string `json:"dashboard_addr"` // Optional.

If so, how should their absence be handled (maybe add a no-op Dashapi implementation and instantiate that if the fields are missing)?

It is nice to be able to start syz-ci (for testing / during development) without deploying a separate dashboard app, so I'd vote to make that possible somehow.

[a-nogikh]

I think we should repair the non-dashboard mode and let those fields stay optional.

How exactly -- it depends on:

1. Whether there are cases when an if dash == nil check could have prevented unnecessary work we would still be doing with a dummy dashboard implementation. E.g. a syz-ci may upload build assets to GCS and push download links to the dashboard, which makes no sense at all if there's no dashboard to send the link to. Though in this particular case you probably wouldn't configure the asset storage either, so nothing will be uploaded anyway.
2. The size of the resulting code diff.

If the approaches are equivalent on both these factors, I don't have a strong preference.

[glpesk]

Thank you! That makes sense. I'll spend some time reworking this today and upload a new version.