-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add support for plugin config migration #9141
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
Conversation
|
Skipping CI for Draft Pull Request. |
| // Version History | ||
| // 1: Deprecated and removed in containerd 2.0 | ||
| // 2: Uses fully qualified plugin names | ||
| // 3: Added support for migration and warning on unknown fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v3 just seems to update implementation details.
Can we just retain v2 as the format version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you suggest we add another version here? We did that with database to differentiate "schema version" from the data version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just keep it a single "version: 2" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some way to know a migration has been completed, otherwise deprecated fields will always be looked for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add Annotation map[string]string to store the migration state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its best we keep the versioning like this, the other options aren't nearly as clean or easy to handle the migration per version. Realistically these versions won't continue bump very fast since we don't introduce new plugins very often. At most this once per minor release. Keeping the versioning like this allows us to move forward without breaking older configurations, support explicit migration, or allow users to switch between older version of containerd without migrating.
I'm also looking into whether we can just un-deprecate version 1 and use a similar migration. This would be cleaner to users than having an error. I could include this here 5fd2bed (still needs docs updates)
eb7f2ac to
9c068eb
Compare
Allows plugins to migrate from older configurations Signed-off-by: Derek McGowan <derek@mcg.dev>
Add reset registrations function to plugin package Signed-off-by: Derek McGowan <derek@mcg.dev>
Allows applying migration to existing configurations Signed-off-by: Derek McGowan <derek@mcg.dev>
9c068eb to
62f273d
Compare
Signed-off-by: Derek McGowan <derek@mcg.dev>
fuweid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
AkihiroSuda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think it has to be documented what will happen when config v3 is provided for containerd v1.6 LTS and v1.7
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
As we split apart plugins or rename plugins, the configuration options move. Without migration, the move appears as a breaking change to users since previous values may be ignored without an updated config. This change allows plugins to migrate the values from the old plugin or name without breaking users. In order to accomplish this, we will need to bump the config version in every containerd major/minor release which contains a new config migration. We should also be able to avoid completely deprecating older config versions after this change (version 1 will remain deprecated as we have no migration for it).