Move snapshot event publishing into metadata store by dmcgowan · Pull Request #5674 · containerd/containerd

dmcgowan · 2021-07-01T20:44:27Z

Removes the snapshot event publishing from the snapshot service.

Cleans up metadata plugin registration into separate packages.

Adds an option to metadata db to add a publisher. Adds event
publishing to prepare, commit, and remove snapshot operations.
Adds remove snapshot event to garbage collection.

Adds snapshotter field to snapshot events

This is useful for snapshot caches which attempt to track current snapshots based on creation and deletion events. Most snapshots are not removed through the API, but through garbage collection. The end goal being the removal of the inefficient snapshot syncer in the CRI plugin

theopenlab-ci · 2021-07-01T20:45:57Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-test-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-integration-test-arm64 : NODE_FAILURE in 0s (non-voting)

theopenlab-ci · 2021-07-01T20:55:01Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-test-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-integration-test-arm64 : NODE_FAILURE in 0s (non-voting)

theopenlab-ci · 2021-07-01T21:01:12Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-test-arm64 : NODE_FAILURE in 0s (non-voting)
containerd-integration-test-arm64 : NODE_FAILURE in 0s (non-voting)

ktock

LGTM

kzys · 2021-07-02T22:01:45Z

api/events/snapshot.proto

Did we have 3 and 4 before?

The fields only need to be unique and anything in the 1-15 range encodes to the same size. Since it is common across all the events, I choose a number higher than what any use so that it can be common and leave 1-4 reserved for event-specific fields. I am not a protobuf expert so I could be running afoul of some conventions here.

I see. Sounds good to me. I'm not a protobuf expert though :)

metadata/gc.go

metadata/snapshot.go

mxpv · 2021-07-02T22:54:13Z

Metadata package is growing in complexity, I'd try to avoid adding new responsibilities to it and have it just manage database.
Would it be feasible to make GC reroute clean up calls through services instead of removing them directly from database? This way each service will be able to publish events or whatever logic it does.
For instance, each participating service may implement interface:

type GarbageCollect interface { Cleanup([]gc.Node) error; }

Would this make sense?

fuweid · 2021-07-03T09:29:48Z

services/server/server.go

If possible, we can move the content plugin init out of services package.

Yes, any plugin which isn't dynamically created via configuration should be moved out

fuweid · 2021-07-03T09:36:39Z

/cc

dmcgowan · 2021-07-18T17:50:23Z

For instance, each participating service may implement interface:
type GarbageCollect interface { Cleanup([]gc.Node) error; }
Would this make sense?

The metadata package sits on the bottom of the service stack and provides the implementation used by the services. The services could register GC callbacks to handle events triggered by GC, however, that adds complexity to to the service implementations and relationship between the packages. The event publishing as it exists today is even lower level as it is built directly into the plugin framework. It seems appropriate to do event publishing at the lowest level of the service stack from my perspective so we don't miss out on events when service logic is changed or added.

I understand the concern about a single package growing in complexity compared to the complexity of other packages. My opinion is it is better to hide complexity in fewer packages rather than add complexity to the interfaces between them. We could always refactor the metadata package to separate the individual service interface implementations though. I would ideally like the metadata package not visible as a plugin but rather as those individual implementations. Right now it does contain quite a bit of the service logic, data persistence, and garbage collection. I think it would be quite a bit of effort to cleanly separate those 3 logical layers without performance degradation or increasing complexity even more.

kzys · 2021-10-07T20:41:14Z

Moving to "Needs Contributor Update" due to code conflicts.

kzys · 2021-10-07T20:43:28Z

metadata/db.go

+			case *eventstypes.SnapshotRemove:
+				topic = "/snapshot/remove"
+			default:
+				log.G(ctx).WithField("event", ne.event).Debug("unhandled event type from garbage collection removal")


"garbage collection removal"?

metadata/snapshot.go

theopenlab-ci · 2021-12-01T02:30:17Z

Build succeeded.

containerd-build-arm64 : FAILURE in 3m 23s (non-voting)
containerd-test-arm64 : FAILURE in 3m 40s (non-voting)
containerd-integration-test-arm64 : FAILURE in 6m 52s (non-voting)

theopenlab-ci · 2021-12-09T22:18:03Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 46s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 52s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 57s (non-voting)

theopenlab-ci · 2021-12-15T20:37:19Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 52s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 49s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 11s (non-voting)

dmcgowan · 2021-12-16T17:06:27Z

This has been rebased is ready for review again

fuweid

LGTM

sbuckfelder · 2022-10-12T14:54:32Z

@kzys are you still requesting a change? Can this be rebased?

AkihiroSuda · 2022-12-08T09:43:23Z

Needs rebase

Removes the snapshot event publishing from the snapshot service. Adds an option to metadata db to add a publisher. Adds event publishing to prepare, commit, and remove snapshot operations. Adds remove snapshot event to garbage collection. Signed-off-by: Derek McGowan <[email protected]>

dmcgowan · 2022-12-20T19:47:12Z

Updated, this PR is much simpler and smaller in scope now that the other changes have been split off and already merged.

… main Updates ADO containerd fork-external/main with upstream containerd main @commit hash: f9f8455 containerd@f9f8455 Additional changes in the PR: - update linux container image version - remove copying LICENSE file in shared-build-stages.yml - additions to .gdnsuppress Related work items: containerd#5674, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7879, containerd#7880, containerd#7881, containerd#7882, containerd#7883, containerd#7886, containerd#7887, containerd#7888, containerd#7891, containerd#7892, containerd#7893, containerd#7894, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7960, containerd#7963, containerd#7969, containerd#7970, containerd#7973

…/main Update fork-external/main with upstream containerd/containerd/main at commit hash 3d32da8 Related work items: containerd#5674, containerd#7129, containerd#7393, containerd#7661, containerd#7685, containerd#7810, containerd#7850, containerd#7861, containerd#7882, containerd#7883, containerd#7886, containerd#7891, containerd#7892, containerd#7893, containerd#7903, containerd#7904, containerd#7905, containerd#7906, containerd#7907, containerd#7908, containerd#7911, containerd#7913, containerd#7914, containerd#7917, containerd#7925, containerd#7927, containerd#7928, containerd#7929, containerd#7932, containerd#7935, containerd#7943, containerd#7946, containerd#7948, containerd#7957, containerd#7958, containerd#7959, containerd#7960, containerd#7963, containerd#7968, containerd#7969, containerd#7970, containerd#7973, containerd#7985, containerd#7987, containerd#7994, containerd#8005

dmcgowan force-pushed the metadata-snapshot-publish branch from cf72139 to 0ebe085 Compare July 1, 2021 20:53

dmcgowan force-pushed the metadata-snapshot-publish branch from 0ebe085 to 83ca7a7 Compare July 1, 2021 20:59

ktock approved these changes Jul 2, 2021

View reviewed changes

kzys requested changes Jul 2, 2021

View reviewed changes

fuweid reviewed Jul 3, 2021

View reviewed changes

k8s-ci-robot requested a review from fuweid July 3, 2021 09:36

dmcgowan force-pushed the metadata-snapshot-publish branch 2 times, most recently from b91a011 to ff0c675 Compare July 27, 2021 01:08

dmcgowan added this to the 1.6 milestone Oct 6, 2021

kzys requested changes Oct 7, 2021

View reviewed changes

dmcgowan force-pushed the metadata-snapshot-publish branch from ff0c675 to 97e9a16 Compare December 1, 2021 02:21

dmcgowan force-pushed the metadata-snapshot-publish branch from 97e9a16 to 37cd85b Compare December 9, 2021 21:46

dmcgowan force-pushed the metadata-snapshot-publish branch from 37cd85b to a0d862e Compare December 15, 2021 20:06

fuweid approved these changes Jan 7, 2022

View reviewed changes

dmcgowan modified the milestones: 1.6, 1.7 Jan 18, 2022

This was referenced Jun 21, 2022

Add snapshotter key to snapshot events #7084

Merged

Move metadata plugin registration to seperate package #7096

Merged

dmcgowan force-pushed the metadata-snapshot-publish branch from a0d862e to 2c573de Compare December 20, 2022 01:07

dmcgowan requested a review from kzys December 20, 2022 19:48

kzys approved these changes Dec 22, 2022

View reviewed changes

fuweid approved these changes Jan 3, 2023

View reviewed changes

fuweid merged commit 9a7c264 into containerd:main Jan 3, 2023

dmcgowan deleted the metadata-snapshot-publish branch April 20, 2024 00:46

Conversation

dmcgowan commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Jul 1, 2021

Uh oh!

theopenlab-ci bot commented Jul 1, 2021

Uh oh!

theopenlab-ci bot commented Jul 1, 2021

Uh oh!

ktock left a comment

Choose a reason for hiding this comment

Uh oh!

kzys Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

kzys Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mxpv commented Jul 2, 2021

Uh oh!

fuweid Jul 3, 2021

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

fuweid commented Jul 3, 2021

Uh oh!

dmcgowan commented Jul 18, 2021

Uh oh!

kzys commented Oct 7, 2021

Uh oh!

kzys Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theopenlab-ci bot commented Dec 1, 2021

Uh oh!

theopenlab-ci bot commented Dec 9, 2021

Uh oh!

theopenlab-ci bot commented Dec 15, 2021

Uh oh!

dmcgowan commented Dec 16, 2021

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

sbuckfelder commented Oct 12, 2022

Uh oh!

AkihiroSuda commented Dec 8, 2022

Uh oh!

dmcgowan commented Dec 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dmcgowan commented Jul 1, 2021 •

edited

Loading