Skip to content
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

fix: allow azure blobs to not exist when deleting #11070

Conversation

roelarents
Copy link
Contributor

@roelarents roelarents commented May 11, 2023

Motivation

The docs say about artifact garbage collection:

Artifact already having been deleted which is not considered a failure

However it is considered a failure at the moment. (At least for azure blobs.)

Modifications

Changed the azure implementation of the ArtifactDriver to swallow StorageErrorCodeBlobNotFound errors from the azure client when Deleteing an artifact.

Verification

I didn't test it yet.

@roelarents roelarents marked this pull request as ready for review May 11, 2023 12:36
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Looks good. Could you test it and verify the changes?

blobClient, err := containerClient.NewBlobClient(blobName)
if err != nil {
return fmt.Errorf("unable to create Azure Blob client for %s: %s", blobName, err)
}

_, err = blobClient.Delete(context.TODO(), nil)
if err != nil {
return fmt.Errorf("unable to delete Azure Blob %s: %s", blobName, err)
if allowNonExistent && IsAzureError(err, azblob.StorageErrorCodeBlobNotFound) {
log.Debugf("blob to delete '%s' does not exist (anymore): %s", blobName, err)
Copy link
Member

Choose a reason for hiding this comment

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

We can remove "anymore"

Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
@roelarents roelarents force-pushed the allow-azure-artifact-delete-nonexistent branch from 2467884 to 958c6ff Compare May 12, 2023 09:46
Dirc and others added 2 commits May 15, 2023 15:51
Signed-off-by: Dirc <e.e.cornet@gmail.com>
fix fix: allow azure blobs to not exist when deleting
@Dirc
Copy link
Contributor

Dirc commented May 15, 2023

Hi @terrytangyuan, I have tested the change and added a small fix. This fixes the bug for @roelarents and me.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thank you!

@terrytangyuan terrytangyuan enabled auto-merge (squash) May 15, 2023 16:10
@terrytangyuan terrytangyuan merged commit 901549f into argoproj:master May 17, 2023
23 checks passed
terrytangyuan added a commit that referenced this pull request May 25, 2023
Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
Signed-off-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Yuan Tang <terrytangyuan@gmail.com>
@roelarents roelarents deleted the allow-azure-artifact-delete-nonexistent branch June 1, 2023 11:45
JPZ13 pushed a commit to pipekit/argo-workflows that referenced this pull request Jul 4, 2023
Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
Signed-off-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Yuan Tang <terrytangyuan@gmail.com>
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
Signed-off-by: Roel Arents <roel.arents@kadaster.nl>
Signed-off-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Dirc <e.e.cornet@gmail.com>
Co-authored-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
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.

None yet

3 participants