Skip to content

feat: Add s3 storage-based registry store in Go feature server #5336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

iamcodingcat
Copy link
Contributor

@iamcodingcat iamcodingcat commented May 7, 2025

What this PR does / why we need it:

This pull request integrates S3 storage as a registry store supported by the Go Feature Server, which is currently in alpha. At the moment, the Go Feature Server only supports a registry store based on the local file system. To make it suitable for production use in a service environment, we developed support for a registry store based on AWS S3 — a widely adopted and popular cloud storage solution.

I developed below things

  • protobuf version upgraded to latest version
    • arm64 platform and MacOS compatibility
    • arm64 platform and debian OS compatibility
  • add feature of s3 based registry store and test-code

*For your reference, the commit username younghun-jo-ilevit-com is also me.

Which issue(s) this PR fixes:

No. it is just for feature enhancement.

Misc

@iamcodingcat iamcodingcat requested a review from a team as a code owner May 7, 2025 08:12
@@ -68,7 +68,7 @@ func NewRegistry(registryConfig *RegistryConfig, repoPath string, project string
func (r *Registry) InitializeRegistry() error {
_, err := r.getRegistryProto()
if err != nil {
if _, ok := r.registryStore.(*FileRegistryStore); ok { // S3에는 굳이 연동할 필요 없어 보임. 오히려 이로 인해 정상적이던 s3 내의 레지스트리 파일이 초기화된 파일로 덮어쓰기 될 수도 있지 않음?
if _, ok := r.registryStore.(*FileRegistryStore); ok {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't apply if statement in case that registry store is S3RegistryStore. because this initialized empty registry store is likely to overwrite normal registry file in s3 storage. Of course, this case is when the err of r.getRegistryProto() method is the network error not abnormal file in s3 storage.

@franciscojavierarceo franciscojavierarceo changed the title feat: s3 storage-based registry store in Go feature server feat: Add s3 storage-based registry store in Go feature server May 7, 2025
Copy link
Member

@franciscojavierarceo franciscojavierarceo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind adding a test for this please?

@iamcodingcat
Copy link
Contributor Author

@franciscojavierarceo thanks for review. But I think about how to write test-code for s3 registry store. As you konw, for testing s3 registry, any s3 bucket needs to exist. Of course, in my localhost test, my own s3 bucket exists but in the test of this public repository, I thins that a single s3 bucket will be public.. it is right? Or, how to make mock s3 bucket? Can you advise to me about how to add test-code for s3 registry store?

@shuchu
Copy link
Collaborator

shuchu commented May 8, 2025

@franciscojavierarceo thanks for review. But I think about how to write test-code for s3 registry store. As you konw, for testing s3 registry, any s3 bucket needs to exist. Of course, in my localhost test, my own s3 bucket exists but in the test of this public repository, I thins that a single s3 bucket will be public.. it is right? Or, how to make mock s3 bucket? Can you advise to me about how to add test-code for s3 registry store?

Hi @iamcodingcat , thank you for this feature. I know add unit test/integration test here is difficult. by any chance, can you post any results/logs from your local run here as a reference for us?

So far, the only concern from my side is the authentication and the credential handling for accessing the S3.

@franciscojavierarceo
Copy link
Member

import (
	"bytes"
	"context"
	"errors"
	"io"
	"testing"

	"github.com/aws/aws-sdk-go-v2/service/s3"
	"github.com/aws/aws-sdk-go-v2/service/s3/types"
	"github.com/feast-dev/feast/go/internal/feast/registry"
	"github.com/stretchr/testify/assert"
)

// Define a mock S3 client
type MockS3Client struct {
	GetObjectFn    func(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error)
	DeleteObjectFn func(ctx context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error)
}

func (m *MockS3Client) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) {
	if m.GetObjectFn != nil {
		return m.GetObjectFn(ctx, input)
	}
	return nil, errors.New("not implemented")
}

func (m *MockS3Client) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) {
	if m.DeleteObjectFn != nil {
		return m.DeleteObjectFn(ctx, input)
	}
	return nil, errors.New("not implemented")
}
func NewMockS3RegistryStore(mockClient *MockS3Client, filePath string) *registry.S3RegistryStore {
	return &registry.S3RegistryStore{
		filePath: filePath,
		s3Client: mockClient,
	}
}

@iamcodingcat
Copy link
Contributor Author

iamcodingcat commented May 9, 2025

Hi @iamcodingcat , thank you for this feature. I know add unit test/integration test here is difficult. by any chance, can you post any results/logs from your local run here as a reference for us?
So far, the only concern from my side is the authentication and the credential handling for accessing the S3.

Hi. @shuchu thanks for review. I attached my console std output after running ./feast --type=http command connected to my s3 registry store.
When comparing with console using local file registry store, the console output seems to the same. Is this useful for adding test-code?
(In the attachment, I masked my redis cluster hostname. please understand me.)

스크린샷 2025-05-09 오전 11 46 15

@iamcodingcat
Copy link
Contributor Author

@franciscojavierarceo thanks for your guide. I try to add test-code and if I have a question, I will ask for it.

@iamcodingcat
Copy link
Contributor Author

iamcodingcat commented May 9, 2025

@shuchu @franciscojavierarceo
I am working on covering linux-based container environment. So I intiallly test it(default setting), the aws related credential error occurs. the logger message is below.

{"level":"error","error":"operation error S3: GetObject, resolve auth scheme: resolve endpoint: endpoint rule error, Invalid region: region was not a valid DNS name.","time":"2025-05-09T05:01:28Z","message":"Registry refresh Failed"}

At present, when this error occurs, the server is still running not panic. Of course, when that error occurs, it seems that the health check endpoint (/health) also does not return a normal response. So even if the Go feature server is deployed in a Kubernetes environment as a Deployment, the readinessProbe would fail, and traffic likely wouldn’t be routed to the pods with the error during a rolling update. Still, would it be better to explicitly panic in order to clearly indicate the error?

Comment on lines +658 to +660
ifeq ($(PB_ARCH), aarch64)
PB_ARCH=aarch_64
endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In linux based arm64 architecture platform, output of uname -m command is aarch64. If this if-statement doesn't exist, PB_ARCH value is aarch64 and this cause error when downloading protobuf release download. Because arm architecture name of protobuf release file name is aarch_64 not aarch64. please refer below link

Copy link
Contributor Author

@iamcodingcat iamcodingcat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuchu @franciscojavierarceo Hi! I add test-code for s3 registry store with mocking s3 client. I ask for you to review it.

@@ -55,7 +61,7 @@ func (r *S3RegistryStore) GetRegistryProto() (*core.Registry, error) {
Key: aws.String(key),
})
if err != nil {
return nil, err
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't apply panic, a failed test case seems to normal.

@@ -88,7 +94,7 @@ func (r *S3RegistryStore) Teardown() error {
Key: aws.String(key),
})
if err != nil {
return err
panic(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't apply panic, a failed test case seems to normal.

@iamcodingcat
Copy link
Contributor Author

iamcodingcat commented May 10, 2025

@shuchu @franciscojavierarceo In the go feature server, I encountered synchronization issue using redis as online-store. pleease check this issue

This is automatically resolved.. I am confused at... anyway it is normal. you ignore this comment!

@franciscojavierarceo
Copy link
Member

@iamcodingcat can you fix the DCO? you just have to click button in UI.

@iamcodingcat iamcodingcat force-pushed the dev/go/feature-server/registry/s3 branch from 9e54a97 to 4862622 Compare May 13, 2025 04:40
@iamcodingcat
Copy link
Contributor Author

iamcodingcat commented May 13, 2025

@franciscojavierarceo thnaks for review approval. I fixed DCO. I request your review again :)

@franciscojavierarceo franciscojavierarceo merged commit abe18df into feast-dev:master May 13, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants