chore(modulestore): add public coredump modulestore API

[korniltsev]

Move public coredump modulestore definitions from tools/coredump/main package to a new coredumpstore package.

This allows to use the coredumpstore in multiple packages, not just coredump main. This may be handy for unit tests.

[florianl]

Can you elaborate a bit more, what you prevents from constructing your own modulestore.New() wrapper, so it can be used also in different places?

Would it be sufficient for you to move the constants, like moduleStoreRegion and others, to package modulestore and make them public?

[korniltsev]

Can you elaborate a bit more, what you prevents from constructing your own modulestore.New() wrapper, so it can be used also in different places?

I don't quite get the question. This PR does exactly that.

Would it be sufficient for you to move the constants, like moduleStoreRegion and others, to package modulestore and make them public?

It would work. I still want a convenient wrapper, not just constants. I don't want to bother with aws config and s3 clients.

[korniltsev]

I also want to have a shared cache

[florianl]

From the request to use the coredump package, and its subpackages, in other places I would have expected some interface, that can be used and replaced in other situations.
That is why I did ask if the alternative with exposing the constants would be an option.

Personally, I don't have a good feeling around the findGitRoot() function. And so I'm looking for alternatives.

[korniltsev]

just in case: I've added findGitRoot so that I don't have to specify the correct number of ../../../../../ for each different package that is going to use the new helper.

[korniltsev]

Trying to understand you.

I propose to do this

package foo;

func TestName1(t *testing.T) {
	store, err := coredumpstore.New()
	require.NoError(t, err)

	id, err := modulestore.IDFromString("a621ea443bba20ffbdb5322633c5a1bf439905baa5822973b2cafc4106c64789")
	require.NoError(t, err)
	_ = store.UnpackModule(id, bytes.NewBuffer(nil))
	// ...
}

and you propose to do this

package foo;
// moduleStoreRegion defines the S3 bucket OCI region.
const moduleStoreRegion = "us-sanjose-1"

// moduleStoreObjectNamespace defines the S3 bucket OCI object name space.
const moduleStoreObjectNamespace = "axtwf1hkrwcy"

// modulePublicReadUrl defines the S3 bucket OCI public read only base path.
//
//nolint:lll
const modulePublicReadURL = "sm-wftyyzHJkBghWeexmK1o5ArimNwZC-5eBej5Lx4e46sLVHtO_y7Zf7FZgoIu_/n/axtwf1hkrwcy"

// moduleStoreS3Bucket defines the S3 bucket used for the module store.
const moduleStoreS3Bucket = "ebpf-profiling-coredumps"




func TestName1(t *testing.T) {
	localCachePath := "../../../../../tools/coredump/modulecache"
	publicReadURL := fmt.Sprintf("https://%s.objectstorage.%s.oci.customer-oci.com/p/%s/b/%s/o/",
		moduleStoreObjectNamespace, moduleStoreRegion, modulePublicReadURL, moduleStoreS3Bucket)

	cfg, err := config.LoadDefaultConfig(context.Background())
	require.NoError(t, err)

	s3Client := s3.NewFromConfig(cfg, func(o *s3.Options) {
		baseEndpoint := fmt.Sprintf("https://%s.compat.objectstorage.%s.oraclecloud.com/",
			moduleStoreObjectNamespace, moduleStoreRegion)
		o.Region = moduleStoreRegion
		o.BaseEndpoint = aws.String(baseEndpoint)
		o.UsePathStyle = true
	})
	store, err := modulestore.New(s3Client, publicReadURL, moduleStoreS3Bucket, localCachePath)
	require.NoError(t, err)

	id, err := modulestore.IDFromString("a621ea443bba20ffbdb5322633c5a1bf439905baa5822973b2cafc4106c64789")
	require.NoError(t, err)
	_ = store.UnpackModule(id, bytes.NewBuffer(nil))
	// ...
}

Right?

[korniltsev]

And as there is only a single occurrence in the scope of this public repository, I think this should be fine.

That is what this PR is trying to change.

[florianl]

Sorry for being short in #543 (comment) and not expanding more on the idea.

With #558 I have quickly implemented the idea that I proposed in #543 (comment).

With this, your tests should look like this:

func TestName1(t *testing.T) {
	localCachePath := "../../../../../tools/coredump/modulecache"
        cloudClient, err := cloudstore.Client()
	require.NoError(t, err)
	store, err := modulestore.New(cloudClient,
		cloudstore.PublicReadURL(), cloudstore.ModulestoreS3Bucket(), localCachePath)

Is this something you could work with?

[korniltsev]

Your draft is almost the same as mine, why can't we make cloudstore.Client() just return the modulestore.Store ?

It technically work. It is less convenient. It is verbose and repetitive I still have to copypaste these details loudClient, cloudstore.PublicReadURL(), cloudstore.ModulestoreS3Bucket(), which I don't really care about ( I just need to pull a blob, I dont want to know about s3 clients and urls). It is fine to have them available as publi API for those who need them and needs to swap them. I just don't need them and propose something slightly more convenient.

It is ugly and inconvenitent to calculate the correct number of ../../../ localCachePath in each test.

Would that work for you if I add cloudstore.Client() , cloudstore.ModulestoreS3Bucket(), cloudstore.PublicReadURL() and then never use but still keep the cloudstore.New() with zero parameters?

[florianl]

When thinking about APIs and packages, I try to separate them by their functionality.
To me, the most significant difference between #543 and #558 is, that #543 mixes functionality, while #558 separates them. Additionally, #558 also allows people to have their dedicated cloud module storage.

It is ugly and inconvenitent to calculate the correct number of ../../../ localCachePath in each test.

I agree with that. But I think, you can use and reuse your findGitRoot() from #543 to improve that.

Would that work for you [..] still keep the cloudstore.New() with zero parameters?

As pointed out, before, I think, packages and API should have their clear boundary and should be separated by functionality.

[korniltsev]

Gentle bump.
Not having modulestore API prevents submitting tests using modulestore. I had to decide not to submit a testsuite for #552
cc @florianl @fabled

EDIT: wrong PR link

[felixge]

This LGTM. But somebody who is more hands on in this repo should approve as well.