Skip to content

Conversation

@dmcgowan
Copy link
Member

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).

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan dmcgowan marked this pull request as ready for review September 26, 2023 03:10
// 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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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" ?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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)

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>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@AkihiroSuda AkihiroSuda left a 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

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