Repository: distribution/distribution
Pull request #3948 (2023-06-26 10:21:39)

registry/handlers/app: log healthcheck error before return
this error doesn't show up anywhere, so when the healthcheck does error the cause is often very mysterious.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/distribution/distribution/pull/3948?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Patch coverage has no change and project coverage change: **`-0.02`** :warning:
> Comparison is base [(`87b2807`)](https://app.codecov.io/gh/distribution/distribution/commit/87b280718d3898c8c78ddf8f358c58f6e92e3c9e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 55.90% compared to head [(`f6e1ab2`)](https://app.codecov.io/gh/distribution/distribution/pull/3948?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 55.88%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please [install the Github App Integration](https://github.com/apps/codecov) for your organization. [Read more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #3948      +/-   ##
==========================================
- Coverage   55.90%   55.88%   -0.02%     
==========================================
  Files         108      108              
  Lines       10946    10949       +3     
==========================================
  Hits         6119     6119              
- Misses       4142     4145       +3     
  Partials      685      685              
```


| [Impacted Files](https://app.codecov.io/gh/distribution/distribution/pull/3948?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [registry/handlers/app.go](https://app.codecov.io/gh/distribution/distribution/pull/3948?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cmVnaXN0cnkvaGFuZGxlcnMvYXBwLmdv) | `47.01% <0.00%> (-0.20%)` | :arrow_down: |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/distribution/distribution/pull/3948?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

flavianmissi: > I've no real problem with this but beware these checks may be frequent to this can generate an influx of error logs - this can become a real issue if you have bugsnag hooks attached to your error logger, etc...

hmm I hadn't thought about it this way. is there something we can do about it? I can't really think of anything [that doesn't add significant work] 🤔 
milosgajdos: Yeah can't think of anything off the top of my head either 🤔 I'll tag in other maintainers so they can weigh in. I'll approve this as it's a small and handy change but let's see what others say.
deleteriousEffect: > these checks may be frequent to this can generate an influx of error logs

If your storage driver can't routinely stat `/`, that's a significant issue, so it might as well be noisy. 

Patch:
diff --git a/docs/configuration.md b/docs/configuration.md
index 0580cec7..35c32656 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -103,8 +103,6 @@ storage:
       clientid: client_id_string
       tenantid: tenant_id_string
       secret: secret_string
-    copy_status_poll_max_retry: 10
-    copy_status_poll_delay: 100ms
   gcs:
     bucket: bucketname
     keyfile: /path/to/keyfile
diff --git a/docs/storage-drivers/azure.md b/docs/storage-drivers/azure.md
index 50f03a2b..01826e61 100644
--- a/docs/storage-drivers/azure.md
+++ b/docs/storage-drivers/azure.md
@@ -8,14 +8,12 @@ An implementation of the `storagedriver.StorageDriver` interface which uses [Mic
 
 ## Parameters
 
-| Parameter                          | Required | Description                                                                                                                                                                                                                                                         |
-|:-----------------------------------|:---------|:--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `accountname`                      | yes      | Name of the Azure Storage Account.                                                                                                                                                                                                                                  |
-| `accountkey`                       | yes      | Primary or Secondary Key for the Storage Account.                                                                                                                                                                                                                   |
-| `container`                        | yes      | Name of the Azure root storage container in which all registry data is stored. Must comply the storage container name [requirements](https://docs.microsoft.com/rest/api/storageservices/fileservices/naming-and-referencing-containers--blobs--and-metadata). For example, if your url is `https://myaccount.blob.core.windows.net/myblob` use the container value of `myblob`.|
-| `realm`                            | no       | Domain name suffix for the Storage Service API endpoint. For example realm for "Azure in China" would be `core.chinacloudapi.cn` and realm for "Azure Government" would be `core.usgovcloudapi.net`. By default, this is `core.windows.net`.                        |
-| `copy_status_poll_max_retry`       | no       | Max retry number for polling of copy operation status. Retries use a simple backoff algorithm where each retry number is multiplied by `copy_status_poll_delay`, and this number is used as the delay. Set to -1 to disable retries and abort if the copy does not complete immediately. Defaults to 5.                |
-| `copy_status_poll_delay`            | no       | Time to wait between retries for polling of copy operation status. This time is multiplied by N on each retry, where N is the retry number. Defaults to 100ms |
+| Parameter     | Required | Description                                                                                                                                                                                                                                                         |
+|:--------------|:---------|:--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `accountname` | yes      | Name of the Azure Storage Account.                                                                                                                                                                                                                                  |
+| `accountkey`  | yes      | Primary or Secondary Key for the Storage Account.                                                                                                                                                                                                                   |
+| `container`   | yes      | Name of the Azure root storage container in which all registry data is stored. Must comply the storage container name [requirements](https://docs.microsoft.com/rest/api/storageservices/fileservices/naming-and-referencing-containers--blobs--and-metadata). For example, if your url is `https://myaccount.blob.core.windows.net/myblob` use the container value of `myblob`.|
+| `realm`       | no       | Domain name suffix for the Storage Service API endpoint. For example realm for "Azure in China" would be `core.chinacloudapi.cn` and realm for "Azure Government" would be `core.usgovcloudapi.net`. By default, this is `core.windows.net`.                        |
 
 
 ## Related information
diff --git a/registry/handlers/app.go b/registry/handlers/app.go
index 95c677f0..cdcb326f 100644
--- a/registry/handlers/app.go
+++ b/registry/handlers/app.go
@@ -377,6 +377,9 @@ func (app *App) RegisterHealthChecks(healthRegistries ...*health.Registry) {
 			if _, ok := err.(storagedriver.PathNotFoundError); ok {
 				err = nil // pass this through, backend is responding, but this path doesn't exist.
 			}
+			if err != nil {
+				dcontext.GetLogger(app).Errorf("storage driver health check: %v", err)
+			}
 			return err
 		}
 
@@ -745,7 +748,6 @@ func (app *App) dispatcher(dispatch dispatchFunc) http.Handler {
 		}
 
 		dispatch(context, r).ServeHTTP(w, r)
-
 	})
 }
 
diff --git a/registry/storage/driver/azure/azure.go b/registry/storage/driver/azure/azure.go
index f2cb1101..88aea274 100644
--- a/registry/storage/driver/azure/azure.go
+++ b/registry/storage/driver/azure/azure.go
@@ -21,17 +21,16 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container"
 )
 
+const driverName = "azure"
+
 const (
-	driverName   = "azure"
 	maxChunkSize = 4 * 1024 * 1024
 )
 
 type driver struct {
-	azClient               *azureClient
-	client                 *container.Client
-	rootDirectory          string
-	copyStatusPollMaxRetry int
-	copyStatusPollDelay    time.Duration
+	azClient      *azureClient
+	client        *container.Client
+	rootDirectory string
 }
 
 type baseEmbed struct{ base.Base }
@@ -60,19 +59,11 @@ func New(params *Parameters) (*Driver, error) {
 	if err != nil {
 		return nil, err
 	}
-
-	copyStatusPollDelay, err := time.ParseDuration(params.CopyStatusPollDelay)
-	if err != nil {
-		return nil, err
-	}
-
 	client := azClient.ContainerClient()
 	d := &driver{
-		azClient:               azClient,
-		client:                 client,
-		rootDirectory:          params.RootDirectory,
-		copyStatusPollMaxRetry: params.CopyStatusPollMaxRetry,
-		copyStatusPollDelay:    copyStatusPollDelay,
+		azClient:      azClient,
+		client:        client,
+		rootDirectory: params.RootDirectory,
 	}
 	return &Driver{baseEmbed: baseEmbed{Base: base.Base{StorageDriver: d}}}, nil
 }
@@ -291,7 +282,7 @@ func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) e
 		return err
 	}
 	destBlobRef := d.client.NewBlockBlobClient(d.blobName(destPath))
-	resp, err := destBlobRef.StartCopyFromURL(ctx, sourceBlobURL, nil)
+	_, err = destBlobRef.CopyFromURL(ctx, sourceBlobURL, nil)
 	if err != nil {
 		if is404(err) {
 			return storagedriver.PathNotFoundError{Path: sourcePath}
@@ -299,39 +290,6 @@ func (d *driver) Move(ctx context.Context, sourcePath string, destPath string) e
 		return err
 	}
 
-	copyStatus := *resp.CopyStatus
-
-	if d.copyStatusPollMaxRetry == -1 && copyStatus == blob.CopyStatusTypePending {
-		destBlobRef.AbortCopyFromURL(ctx, *resp.CopyID, nil)
-		return nil
-	}
-
-	retryCount := 1
-	for copyStatus == blob.CopyStatusTypePending {
-		props, err := destBlobRef.GetProperties(ctx, nil)
-		if err != nil {
-			return err
-		}
-
-		if retryCount >= d.copyStatusPollMaxRetry {
-			destBlobRef.AbortCopyFromURL(ctx, *props.CopyID, nil)
-			return fmt.Errorf("max retries for copy polling reached, aborting copy")
-		}
-
-		copyStatus = *props.CopyStatus
-		if copyStatus == blob.CopyStatusTypeAborted || copyStatus == blob.CopyStatusTypeFailed {
-			if props.CopyStatusDescription != nil {
-				return fmt.Errorf("failed to move blob: %s", *props.CopyStatusDescription)
-			}
-			return fmt.Errorf("failed to move blob with copy id %s", *props.CopyID)
-		}
-
-		if copyStatus == blob.CopyStatusTypePending {
-			time.Sleep(d.copyStatusPollDelay * time.Duration(retryCount))
-		}
-		retryCount++
-	}
-
 	_, err = d.client.NewBlobClient(d.blobName(sourcePath)).Delete(ctx, nil)
 	return err
 }
diff --git a/registry/storage/driver/azure/parser.go b/registry/storage/driver/azure/parser.go
index c463ac32..825c78d6 100644
--- a/registry/storage/driver/azure/parser.go
+++ b/registry/storage/driver/azure/parser.go
@@ -8,9 +8,7 @@ import (
 )
 
 const (
-	defaultRealm                  = "core.windows.net"
-	defaultCopyStatusPollMaxRetry = 5
-	defaultCopyStatusPollDelay    = "100ms"
+	defaultRealm = "core.windows.net"
 )
 
 type Credentials struct {
@@ -21,16 +19,14 @@ type Credentials struct {
 }
 
 type Parameters struct {
-	Container              string      `mapstructure:"container"`
-	AccountName            string      `mapstructure:"accountname"`
-	AccountKey             string      `mapstructure:"accountkey"`
-	Credentials            Credentials `mapstructure:"credentials"`
-	ConnectionString       string      `mapstructure:"connectionstring"`
-	Realm                  string      `mapstructure:"realm"`
-	RootDirectory          string      `mapstructure:"rootdirectory"`
-	ServiceURL             string      `mapstructure:"serviceurl"`
-	CopyStatusPollMaxRetry int         `mapstructure:"copy_status_poll_max_retry"`
-	CopyStatusPollDelay    string      `mapstructure:"copy_status_poll_delay"`
+	Container        string      `mapstructure:"container"`
+	AccountName      string      `mapstructure:"accountname"`
+	AccountKey       string      `mapstructure:"accountkey"`
+	Credentials      Credentials `mapstructure:"credentials"`
+	ConnectionString string      `mapstructure:"connectionstring"`
+	Realm            string      `mapstructure:"realm"`
+	RootDirectory    string      `mapstructure:"rootdirectory"`
+	ServiceURL       string      `mapstructure:"serviceurl"`
 }
 
 func NewParameters(parameters map[string]interface{}) (*Parameters, error) {
@@ -49,11 +45,5 @@ func NewParameters(parameters map[string]interface{}) (*Parameters, error) {
 	if params.ServiceURL == "" {
 		params.ServiceURL = fmt.Sprintf("https://%s.blob.%s", params.AccountName, params.Realm)
 	}
-	if params.CopyStatusPollMaxRetry == 0 {
-		params.CopyStatusPollMaxRetry = defaultCopyStatusPollMaxRetry
-	}
-	if params.CopyStatusPollDelay == "" {
-		params.CopyStatusPollDelay = defaultCopyStatusPollDelay
-	}
 	return &params, nil
 }


Test patch:
diff --git a/registry/storage/driver/azure/azure_test.go b/registry/storage/driver/azure/azure_test.go
index 5fbd01ad..42a8a9b4 100644
--- a/registry/storage/driver/azure/azure_test.go
+++ b/registry/storage/driver/azure/azure_test.go
@@ -1,9 +1,7 @@
 package azure
 
 import (
-	"context"
 	"fmt"
-	"math/rand"
 	"os"
 	"strings"
 	"testing"
@@ -21,8 +19,6 @@ const (
 	envRootDirectory = "AZURE_ROOT_DIRECTORY"
 )
 
-var azureDriverConstructor func() (storagedriver.StorageDriver, error)
-
 // Hook up gocheck into the "go test" runner.
 func Test(t *testing.T) { check.TestingT(t) }
 
@@ -40,10 +36,10 @@ func init() {
 		value     *string
 		missingOk bool
 	}{
-		{envAccountName, &accountName, true},
-		{envAccountKey, &accountKey, true},
-		{envContainer, &container, true},
-		{envRealm, &realm, true},
+		{envAccountName, &accountName, false},
+		{envAccountKey, &accountKey, false},
+		{envContainer, &container, false},
+		{envRealm, &realm, false},
 		{envRootDirectory, &rootDirectory, true},
 	}
 
@@ -55,7 +51,7 @@ func init() {
 		}
 	}
 
-	azureDriverConstructor = func() (storagedriver.StorageDriver, error) {
+	azureDriverConstructor := func() (storagedriver.StorageDriver, error) {
 		parameters := map[string]interface{}{
 			"container":     container,
 			"accountname":   accountName,
@@ -81,66 +77,6 @@ func init() {
 	testsuites.RegisterSuite(azureDriverConstructor, skipCheck)
 }
 
-var letterRunes = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
-
-func randStringRunes(n int) string {
-	b := make([]rune, n)
-	for i := range b {
-		b[i] = letterRunes[rand.Intn(len(letterRunes))]
-	}
-	return string(b)
-}
-
-func TestCommitAfterMove(t *testing.T) {
-	driver, err := azureDriverConstructor()
-	if err != nil {
-		t.Fatalf("unexpected error creating azure driver: %v", err)
-	}
-
-	contents := randStringRunes(4 * 1024 * 1024)
-	sourcePath := "/source/file"
-	destPath := "/dest/file"
-	ctx := context.Background()
-
-	defer driver.Delete(ctx, sourcePath)
-	defer driver.Delete(ctx, destPath)
-
-	writer, err := driver.Writer(ctx, sourcePath, false)
-	if err != nil {
-		t.Fatalf("unexpected error from driver.Writer: %v", err)
-	}
-
-	_, err = writer.Write([]byte(contents))
-	if err != nil {
-		t.Fatalf("writer.Write: unexpected error: %v", err)
-	}
-
-	err = writer.Commit()
-	if err != nil {
-		t.Fatalf("writer.Commit: unexpected error: %v", err)
-	}
-
-	err = writer.Close()
-	if err != nil {
-		t.Fatalf("writer.Close: unexpected error: %v", err)
-	}
-
-	_, err = driver.GetContent(ctx, sourcePath)
-	if err != nil {
-		t.Fatalf("driver.GetContent(sourcePath): unexpected error: %v", err)
-	}
-
-	err = driver.Move(ctx, sourcePath, destPath)
-	if err != nil {
-		t.Fatalf("driver.Move: unexpected error: %v", err)
-	}
-
-	_, err = driver.GetContent(ctx, destPath)
-	if err != nil {
-		t.Fatalf("GetContent(destPath): unexpected error: %v", err)
-	}
-}
-
 func TestParamParsing(t *testing.T) {
 	expectErrors := []map[string]interface{}{
 		{},