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

add repo and tag deletion event #2648

Merged
merged 5 commits into from
Aug 7, 2018

Conversation

manishtomar
Copy link
Contributor

@manishtomar manishtomar commented Jul 18, 2018

Whenever a repo or tag is deleted an event is sent out via the regular notification channels. When repo is deleted the event will only contain repo name but not tag or digest. When tag is deleted the event it will have repo and tag but not digest. There is new interface RepositoryRemover containing method to delete the repo. Its implemented by internal registry instance. To capture the event the registry instance is also stored as RepositoryRemover in the application context.

whenever a tag is deleted an event is sent out via the regular
notification channels

Signed-off-by: Manish Tomar <[email protected]>
@codecov
Copy link

codecov bot commented Jul 18, 2018

Codecov Report

Merging #2648 into master will decrease coverage by 9.72%.
The diff coverage is 70.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2648      +/-   ##
==========================================
- Coverage   61.03%   51.31%   -9.73%     
==========================================
  Files         132      129       -3     
  Lines       12122    11916     -206     
==========================================
- Hits         7399     6115    -1284     
- Misses       3803     5037    +1234     
+ Partials      920      764     -156
Impacted Files Coverage Δ
registry.go 40% <ø> (ø) ⬆️
registry/storage/catalog.go 78.08% <0%> (-7%) ⬇️
notifications/bridge.go 75.42% <100%> (+2.02%) ⬆️
registry/storage/registry.go 90.2% <100%> (-0.34%) ⬇️
registry/handlers/app.go 48.16% <100%> (+0.45%) ⬆️
notifications/listener.go 61.06% <68.18%> (+0.42%) ⬆️
registry/storage/driver/gcs/gcs.go 0.39% <0%> (-68.67%) ⬇️
registry/storage/driver/oss/oss.go 0.56% <0%> (-56.91%) ⬇️
registry/storage/driver/s3-goamz/s3.go 0.5% <0%> (-51.64%) ⬇️
registry/storage/driver/s3-aws/s3.go 4.15% <0%> (-50.98%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dae095...8c05756. Read the comment docs.

@manishtomar
Copy link
Contributor Author

manishtomar commented Jul 18, 2018

@dmcgowan Can you please review this? This is required for Hub. I also need repo deletion event which is not possible cleanly because it doesn't go through Repository interface. I've it written locally by storing eventBridge in context and calling that in vacuum.RemoveRepository: https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/registry/storage/vacuum.go#L89. Let me know if you can think of a better way.

@dmcgowan
Copy link
Collaborator

LGTM

This approach looks fine to me, maybe @stevvooe or @aaronlehmann can give it a quick look to see if the event looks consistent to them.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "tag-deleted-event" [email protected]:manishtomar/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354429072
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

by having another interface RepositoryRemover that is implemented by
registry instance and is injected in app context for event tracking

Signed-off-by: Manish Tomar <[email protected]>
forgot to commit this earlier

Signed-off-by: Manish Tomar <[email protected]>
Copy link
Contributor

@davidswu davidswu left a comment

Choose a reason for hiding this comment

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

LGTM. This is consistent with the other events.

Signed-off-by: Manish Tomar <[email protected]>
@dmp42
Copy link
Contributor

dmp42 commented Aug 6, 2018

LGTM.

@manishtomar why is Circle failing?

Signed-off-by: Manish Tomar <[email protected]>
@manishtomar
Copy link
Contributor Author

@dmp42 I fixed the circle failure but I don't know why Jenkins is failing with integration tests. The logs are not very clear. If you don't mind can you have a quick glance and let me know whats wrong? If not, I can try digging into it.

@manishtomar manishtomar changed the title add tag deletion event add repo and tag deletion event Aug 6, 2018
@dmcgowan
Copy link
Collaborator

dmcgowan commented Aug 7, 2018

LGTM

@dmcgowan dmcgowan merged commit 003aa05 into distribution:master Aug 7, 2018
@manishtomar manishtomar deleted the tag-deleted-event branch August 7, 2018 20:19
manishtomar added a commit to manishtomar/distribution that referenced this pull request Aug 14, 2018
context.App.repoRemover is single registry instance stored throughout
app run. It was wrapped in another remover when processing each request.
This remover happened to be remover got from previous request. This way
every remover created was stored in infinite linked list causing memory
leak. Fixing it by storing the wrapped remover inside the request context
which will get gced when request context is gced. This was introduced in
PR distribution#2648.
manishtomar added a commit to manishtomar/distribution that referenced this pull request Aug 14, 2018
context.App.repoRemover is single registry instance stored throughout
app run. It was wrapped in another remover when processing each request.
This remover happened to be remover got from previous request. This way
every remover created was stored in infinite linked list causing memory
leak. Fixing it by storing the wrapped remover inside the request context
which will get gced when request context is gced. This was introduced in
PR distribution#2648.

Signed-off-by: Manish Tomar <[email protected]>
dmp42 added a commit that referenced this pull request Aug 24, 2018
cquon pushed a commit to cquon/distribution that referenced this pull request Aug 31, 2018
context.App.repoRemover is single registry instance stored throughout
app run. It was wrapped in another remover when processing each request.
This remover happened to be remover got from previous request. This way
every remover created was stored in infinite linked list causing memory
leak. Fixing it by storing the wrapped remover inside the request context
which will get gced when request context is gced. This was introduced in
PR distribution#2648.

Signed-off-by: Manish Tomar <[email protected]>
andrew-leung pushed a commit to andrew-leung/distribution that referenced this pull request Sep 3, 2018
context.App.repoRemover is single registry instance stored throughout
app run. It was wrapped in another remover when processing each request.
This remover happened to be remover got from previous request. This way
every remover created was stored in infinite linked list causing memory
leak. Fixing it by storing the wrapped remover inside the request context
which will get gced when request context is gced. This was introduced in
PR distribution#2648.

Signed-off-by: Manish Tomar <[email protected]>
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.

5 participants