From edfd6798720e7e3091ba8a476424070559033b32 Mon Sep 17 00:00:00 2001 From: Golan Levy Date: Tue, 19 Sep 2023 13:15:50 +0300 Subject: [PATCH] better error handling Signed-off-by: Golan Levy --- model-serving-puller/puller/puller.go | 18 +++++++++++++----- model-serving-puller/puller/puller_test.go | 10 ++++++---- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/model-serving-puller/puller/puller.go b/model-serving-puller/puller/puller.go index cac18769..6229f1dc 100644 --- a/model-serving-puller/puller/puller.go +++ b/model-serving-puller/puller/puller.go @@ -209,7 +209,7 @@ func (s *Puller) ProcessLoadModelRequest(ctx context.Context, req *mmesh.LoadMod } // update the model key to add the disk size - if size, err1 := getModelDiskSize(modelFullPath); err1 != nil { + if size, err1 := s.getModelDiskSize(modelFullPath); err1 != nil { s.Log.Error(err1, "Model disk size will not be included in the LoadModelRequest due to error", "model_key", modelKey) } else { s.Log.Info("Calculated disk size", "modelFullPath", modelFullPath, "disk_size", size) @@ -231,7 +231,7 @@ func (s *Puller) ProcessLoadModelRequest(ctx context.Context, req *mmesh.LoadMod return req, nil } -func getModelDiskSize(modelPath string) (int64, error) { +func (s *Puller) getModelDiskSize(modelPath string) (int64, error) { // This walks the local filesystem and accumulates the size of the model // It would be more efficient to accumulate the size as the files are downloaded, // but this would require refactoring because the s3 download iterator does not return a size. @@ -244,10 +244,18 @@ func getModelDiskSize(modelPath string) (int64, error) { // Calculating the size of the resolved path (for pvc) instead of the symlink itself. // We are not expecting to have infinite recursion since otherwise the serving runtime would not be able to load the model if info.Mode()&os.ModeSymlink != 0 { - if resolvedPath, evalErr := os.Readlink(path); evalErr == nil { - if symlinkSize, err1 := getModelDiskSize(resolvedPath); err1 == nil { - size += symlinkSize + resolvedPath, resolveErr := os.Readlink(path) + + if resolveErr != nil { + s.Log.Error(resolveErr, "Failed to resolve symlink path", "path", path) + } else { + symlinkTargetSize, symlinkTargetSizeErr := s.getModelDiskSize(resolvedPath) + + if symlinkTargetSizeErr != nil { + return symlinkTargetSizeErr } + + size += symlinkTargetSize } } else if !info.IsDir() { size += info.Size() diff --git a/model-serving-puller/puller/puller_test.go b/model-serving-puller/puller/puller_test.go index cf8c174b..0db7586f 100644 --- a/model-serving-puller/puller/puller_test.go +++ b/model-serving-puller/puller/puller_test.go @@ -607,6 +607,8 @@ func Test_getModelDiskSize(t *testing.T) { {"testModelSize/2", 39375276}, } + p, _ := newPullerWithMock(t) + // creating symlinks on runtime since git is not managing symlinks by default symlinkName := "symlink" symlinkPath := filepath.Join(RootModelDir, symlinkName) @@ -614,13 +616,13 @@ func Test_getModelDiskSize(t *testing.T) { for _, tt := range diskSizeTests { t.Run("", func(t *testing.T) { fullPath := filepath.Join(RootModelDir, tt.modelPath) - validatePathDiskSize(t, fullPath, tt.expectedSize) + validatePathDiskSize(t, p, fullPath, tt.expectedSize) // testing symlink to the same path err := os.Symlink(fullPath, symlinkPath) assert.NoError(t, err) - validatePathDiskSize(t, symlinkPath, tt.expectedSize) + validatePathDiskSize(t, p, symlinkPath, tt.expectedSize) err = os.Remove(symlinkPath) assert.NoError(t, err) @@ -628,8 +630,8 @@ func Test_getModelDiskSize(t *testing.T) { } } -func validatePathDiskSize(t *testing.T, path string, expectedSize int64) { - diskSize, err := getModelDiskSize(path) +func validatePathDiskSize(t *testing.T, p *Puller, path string, expectedSize int64) { + diskSize, err := p.getModelDiskSize(path) assert.NoError(t, err) assert.EqualValues(t, expectedSize, diskSize) }