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

HMS-5244: fix snapshot task cleanup #939

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dominikvagner
Copy link
Contributor

Summary

This PR fixes up the snapshot task's cleanup that happens on context cancellation. This caused errors that were being reported by glitchtip.
(error deleting rpm repository versions: 400 Bad Request: [\"The repository version cannot be deleted because it (or its publications) are currently being used to distribute content. Please update the necessary distributions first.\"])

Testing steps

  1. Create a repo and let is snapshot.
  2. Apply this git patch:
diff --git a/pkg/tasks/snapshot_helper.go b/pkg/tasks/snapshot_helper.go
--- a/pkg/tasks/snapshot_helper.go	(revision 8c9b3c2e8f371d28878423f47acd3f19039f46cb)
+++ b/pkg/tasks/snapshot_helper.go	(date 1736855224959)
@@ -111,7 +111,7 @@
 		return fmt.Errorf("unable to save distribution task href: %w", err)
 	}
 
-	return nil
+	return context.Canceled
 }
  1. Update the created repo and create one more.
  2. Let them snapshot and verify there are no errors in the logs and that the first repo has only one snapshot.
  3. Revert the change made in the 2. step.
  4. Trigger snapshots on the 2 created repos and verify that after the snapshot tasks complete their status is valid.

@jlsherrill

This comment was marked as outdated.

@dominikvagner dominikvagner changed the title HMS-5224: fix snapshot task cleanup HMS-5244: fix snapshot task cleanup Jan 15, 2025
@jlsherrill
Copy link
Member

Comment on lines +105 to +108
err = sh.payload.SaveSnapshotIdent(snap.UUID)
if err != nil {
return fmt.Errorf("unable to save snapshot ident: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little confusing, but the SnapshotIdent is set previously and it's actually not the snapshot UUID, just a random UUID, because the snapshot object has not been created yet.

I see that you're setting it so that the snapshot can be deleted, which I think makes sense. To avoid conflating the two values, it probably makes sense to make a new "SnapshotUUID" field for the purpose of tracking snapshot creation?

Comment on lines +109 to +113
err = sh.payload.SaveDistributionTaskHref(snap.DistributionHref)
if err != nil {
return fmt.Errorf("unable to save distribution task href: %w", err)
}

Copy link
Contributor

@rverdile rverdile Jan 16, 2025

Choose a reason for hiding this comment

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

the distribution task href is saved earlier in the function, so I think it's okay to remove this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants