From b22989cfe07ddc764eddcecef8ea8822c828248c Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani <45800463+rchincha@users.noreply.github.com> Date: Fri, 7 Jul 2023 13:51:00 -0700 Subject: [PATCH] fix: refactor monitoring code outside locks (#1596) We use locks to protect OCI layouts. However, our critical sections have too long and cover code instead of data. Fixes issue #1595 Signed-off-by: Ramkumar Chinchani --- pkg/storage/local/local.go | 46 ++++++++++++++++++++++++++------------ pkg/storage/s3/s3.go | 46 ++++++++++++++++++++++++++------------ 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/pkg/storage/local/local.go b/pkg/storage/local/local.go index b01bd770..d382830b 100644 --- a/pkg/storage/local/local.go +++ b/pkg/storage/local/local.go @@ -404,15 +404,23 @@ func (is *ImageStoreLocal) GetImageTags(repo string) ([]string, error) { // GetImageManifest returns the image manifest of an image in the specific repository. func (is *ImageStoreLocal) GetImageManifest(repo, reference string) ([]byte, godigest.Digest, string, error) { - var lockLatency time.Time - dir := path.Join(is.rootDir, repo) if !is.DirExists(dir) { return nil, "", "", zerr.ErrRepoNotFound } + var lockLatency time.Time + + var err error + is.RLock(&lockLatency) - defer is.RUnlock(&lockLatency) + defer func() { + is.RUnlock(&lockLatency) + + if err == nil { + monitoring.IncDownloadCounter(is.metrics, repo) + } + }() index, err := common.GetIndex(is, repo, is.log) if err != nil { @@ -440,8 +448,6 @@ func (is *ImageStoreLocal) GetImageManifest(repo, reference string) ([]byte, god return nil, "", "", err } - monitoring.IncDownloadCounter(is.metrics, repo) - return buf, manifestDesc.Digest, manifestDesc.MediaType, nil } @@ -457,8 +463,17 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, / var lockLatency time.Time + var err error + is.Lock(&lockLatency) - defer is.Unlock(&lockLatency) + defer func() { + is.Unlock(&lockLatency) + + if err == nil { + monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) + monitoring.IncUploadCounter(is.metrics, repo) + } + }() digest, err := common.ValidateManifest(is, repo, reference, mediaType, body, is.log) if err != nil { @@ -584,23 +599,28 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, / } } - monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) - monitoring.IncUploadCounter(is.metrics, repo) - return desc.Digest, subjectDigest, nil } // DeleteImageManifest deletes the image manifest from the repository. func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCollision bool) error { - var lockLatency time.Time - dir := path.Join(is.rootDir, repo) if !is.DirExists(dir) { return zerr.ErrRepoNotFound } + var lockLatency time.Time + + var err error + is.Lock(&lockLatency) - defer is.Unlock(&lockLatency) + defer func() { + is.Unlock(&lockLatency) + + if err == nil { + monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) + } + }() index, err := common.GetIndex(is, repo, is.log) if err != nil { @@ -654,8 +674,6 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCol _ = os.Remove(p) } - monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) - return nil } diff --git a/pkg/storage/s3/s3.go b/pkg/storage/s3/s3.go index 0428d036..f2bbfc93 100644 --- a/pkg/storage/s3/s3.go +++ b/pkg/storage/s3/s3.go @@ -315,15 +315,23 @@ func (is *ObjectStorage) GetImageTags(repo string) ([]string, error) { // GetImageManifest returns the image manifest of an image in the specific repository. func (is *ObjectStorage) GetImageManifest(repo, reference string) ([]byte, godigest.Digest, string, error) { - var lockLatency time.Time - dir := path.Join(is.rootDir, repo) if fi, err := is.store.Stat(context.Background(), dir); err != nil || !fi.IsDir() { return nil, "", "", zerr.ErrRepoNotFound } + var lockLatency time.Time + + var err error + is.RLock(&lockLatency) - defer is.RUnlock(&lockLatency) + defer func() { + is.RUnlock(&lockLatency) + + if err == nil { + monitoring.IncDownloadCounter(is.metrics, repo) + } + }() index, err := common.GetIndex(is, repo, is.log) if err != nil { @@ -351,8 +359,6 @@ func (is *ObjectStorage) GetImageManifest(repo, reference string) ([]byte, godig return nil, "", "", err } - monitoring.IncDownloadCounter(is.metrics, repo) - return buf, manifestDesc.Digest, manifestDesc.MediaType, nil } @@ -368,8 +374,17 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n var lockLatency time.Time + var err error + is.Lock(&lockLatency) - defer is.Unlock(&lockLatency) + defer func() { + is.Unlock(&lockLatency) + + if err == nil { + monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) + monitoring.IncUploadCounter(is.metrics, repo) + } + }() dig, err := common.ValidateManifest(is, repo, reference, mediaType, body, is.log) if err != nil { @@ -486,23 +501,28 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n return "", "", err } - monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) - monitoring.IncUploadCounter(is.metrics, repo) - return desc.Digest, subjectDigest, nil } // DeleteImageManifest deletes the image manifest from the repository. func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectCollisions bool) error { - var lockLatency time.Time - dir := path.Join(is.rootDir, repo) if fi, err := is.store.Stat(context.Background(), dir); err != nil || !fi.IsDir() { return zerr.ErrRepoNotFound } + var lockLatency time.Time + + var err error + is.Lock(&lockLatency) - defer is.Unlock(&lockLatency) + defer func() { + is.Unlock(&lockLatency) + + if err == nil { + monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) + } + }() index, err := common.GetIndex(is, repo, is.log) if err != nil { @@ -555,8 +575,6 @@ func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectColli } } - monitoring.SetStorageUsage(is.metrics, is.rootDir, repo) - return nil }