SPEC: add wording for GC #981

squeed · 2023-03-20T16:12:13Z

This is an initial draft of the wording for the GC verb.

TODO:

Need to declare that concurrent operations are not allowed (e.g. no ADD and GC at the same time)
Describe the execution flow (akin to deletion)
Final proofreading

coveralls · 2023-03-20T16:15:36Z

coverage: 72.685%. remained the same when pulling 3072cfe on squeed:spec-gc into 3bbe370 on containernetworking:main.

MikeZappa87 · 2023-03-27T15:04:06Z

@henry118 give this a look!

SPEC.md

henry118 · 2023-03-27T23:57:55Z

SPEC.md

+
+The runtime must provide a JSON-serialized plugin configuration object (defined below) on standard in. It contains an additional key;
+
+- `cni.dev/attachments` (array of objects): The list of **still valid** attachments to this network:


I know we've been mentioning "attachment" everywhere, wonder shall we formally define the term now? because it's the first time that we'll pass around it as an object.

Good point; attachments are implied by the spec but it would be good to make them clear.

Funny that you mentioned this -- #903 has been lying around :-)

henry118 · 2023-03-28T00:02:18Z

SPEC.md

+#### `GC`: Clean up any stale resources
+
+The GC comand provides a way for runtimes to specify the expected set of attachments to a network.
+The network plugin may then remove any resources related to attachments that do not exist in this set.


What about attachments existing in this set but not found by plugins? Do we need to define the behavior? e.g. error out vs. silently ignored?

Hmm, interesting idea. We could certainly consider returning those as "potentially failed attachments". However, I think CHECK already covers this use case reasonably well, and I don't want to push too much of a burden on individual plugins.

henry118 · 2023-03-28T00:07:58Z

SPEC.md

+- IPAM reservations
+- Firewall rules
+
+A plugin SHOULD remove as many stale resources as possible. For example, a plugin should remove any IPAM reservations associated with attachments not in the provided list. The plugin MAY assume that the isolation domain (e.g. network namespace) has been deleted, and thus any resources (e.g. network interfaces) therein have been removed.


What should be the return value of GC? Can the runtime assume GC always succeeds? Otherwise how does runtime know which attachments are removed and which are not?

Good question. What sort of information would be useful for the runtime? For example, we could have the plugin return whether or not any resources were cleaned up. If we wanted to indicate potentially-misconfigured attachments, we could return them here (though I'm not yet convinced this is useful).

As for returning error, the semantics are the same as with DEL: don't return an error if resources are missing, however, if an error condition prevents successful GC, expose that fact to the runtime.

Yeah I am not sure how much the runtime can do in cases of GC failures, apart from logging an error message. But I do think it would be useful for the runtimes to be aware of GC failures. So if someone were to analyze resource leaks, the log message could be a good indicator.

So we might want to define "what is a GC failure" for CNI plugin. e.g.

Plugin's GC procedure cannot be started; or

An attachment failed to be deleted; or

The resources of an attachment are missing (this case is probably safe to ignore)

When CNI finishes invoking GC on all "leaked" attachments, it should return an error to runtime if "any" of the attachment fails GC.

OK, we worked on wording for this, PTAL.

henry118

LGTM in general. Just some nits, PTAL

henry118 · 2023-04-21T16:55:11Z

SPEC.md

+### Garbage-collecting a network
+The runtime may also ask every plugin in a network to clean up any stale resources.
+
+Garbage collection is similar to add with two exceptions:


you mean similar to "del"?

SPEC.md

henry118 · 2023-04-21T16:58:52Z

SPEC.md

+#### `GC`: Clean up any stale resources
+
+The GC comand provides a way for runtimes to specify the expected set of attachments to a network.
+The network plugin may then remove any resources related to attachments that do not exist in this set.


henry118 · 2023-04-21T17:00:11Z

SPEC.md

+- IPAM reservations
+- Firewall rules
+
+A plugin SHOULD remove as many stale resources as possible. For example, a plugin should remove any IPAM reservations associated with attachments not in the provided list. The plugin MAY assume that the isolation domain (e.g. network namespace) has been deleted, and thus any resources (e.g. network interfaces) therein have been removed.


SPEC.md

MikeZappa87 · 2023-06-05T15:40:32Z

@henry118 we made some updates

SPEC.md

maiqueb · 2023-07-25T09:44:49Z

/cc

SPEC.md

GC, or garbage collection, is intended as a way for runtimes to tell plugins about valid attachments, enabling them to delete any leaked or stale resources. Signed-off-by: Casey Callendrello <[email protected]>

dcbw · 2023-08-07T15:56:15Z

LGTM

henry118 reviewed Mar 28, 2023

View reviewed changes

squeed force-pushed the spec-gc branch 2 times, most recently from d3acf5b to 8cecebf Compare April 20, 2023 11:12

henry118 reviewed Apr 21, 2023

View reviewed changes

tamilmani1989 reviewed May 1, 2023

View reviewed changes

SPEC.md Show resolved Hide resolved

SPEC.md Show resolved Hide resolved

SPEC.md Show resolved Hide resolved

squeed force-pushed the spec-gc branch from 8cecebf to f86c232 Compare June 5, 2023 15:40

squeed force-pushed the spec-gc branch from f86c232 to e3e5390 Compare June 5, 2023 15:46

squeed commented Jun 5, 2023

View reviewed changes

SPEC.md Show resolved Hide resolved

squeed added this to the CNI v1.1 milestone Jul 4, 2023

squeed force-pushed the spec-gc branch from e3e5390 to 152a806 Compare July 24, 2023 15:00

squeed assigned MikeZappa87 Jul 24, 2023

MikeZappa87 reviewed Jul 27, 2023

View reviewed changes

SPEC.md Show resolved Hide resolved

MikeZappa87 reviewed Jul 27, 2023

View reviewed changes

SPEC.md Show resolved Hide resolved

MikeZappa87 reviewed Jul 27, 2023

View reviewed changes

SPEC.md Show resolved Hide resolved

SPEC: add wording for GC verb

3072cfe

GC, or garbage collection, is intended as a way for runtimes to tell plugins about valid attachments, enabling them to delete any leaked or stale resources. Signed-off-by: Casey Callendrello <[email protected]>

squeed force-pushed the spec-gc branch from 152a806 to 3072cfe Compare August 7, 2023 15:50

dcbw merged commit f6506e2 into containernetworking:main Aug 7, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPEC: add wording for GC #981

SPEC: add wording for GC #981

squeed commented Mar 20, 2023 •

edited

Loading

coveralls commented Mar 20, 2023 •

edited

Loading

MikeZappa87 commented Mar 27, 2023

henry118 Mar 27, 2023

squeed Mar 30, 2023

squeed Apr 17, 2023

henry118 Mar 28, 2023

squeed Mar 30, 2023

henry118 Apr 21, 2023

henry118 Mar 28, 2023

squeed Mar 30, 2023

henry118 Mar 31, 2023

squeed Apr 20, 2023

henry118 Apr 21, 2023

henry118 left a comment

henry118 Apr 21, 2023

henry118 Apr 21, 2023

henry118 Apr 21, 2023

MikeZappa87 commented Jun 5, 2023

maiqueb commented Jul 25, 2023

dcbw commented Aug 7, 2023


		The runtime must provide a JSON-serialized plugin configuration object (defined below) on standard in. It contains an additional key;

		- `cni.dev/attachments` (array of objects): The list of still valid attachments to this network:

SPEC: add wording for GC #981

SPEC: add wording for GC #981

Conversation

squeed commented Mar 20, 2023 • edited Loading

coveralls commented Mar 20, 2023 • edited Loading

MikeZappa87 commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henry118 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeZappa87 commented Jun 5, 2023

maiqueb commented Jul 25, 2023

dcbw commented Aug 7, 2023

squeed commented Mar 20, 2023 •

edited

Loading

coveralls commented Mar 20, 2023 •

edited

Loading