fix migrated cri image config when using registry#12617
fix migrated cri image config when using registry#12617dmcgowan merged 1 commit intocontainerd:mainfrom
Conversation
| configPath, configPathExists := regMap["config_path"] | ||
| if _, ok := regMap["mirrors"]; ok { | ||
| // This is needed because if the config_path value is set along with mirrors, it should be made | ||
| // empty | ||
| if configPathExists { | ||
| regMap["config_path"] = "" | ||
| } | ||
| } else if configPathExists && configPath == "" { |
There was a problem hiding this comment.
Making this change here does not actually work, because further later after the configMigration is done, the default values of config_path is used, which gets added when LoadPlugins is called
containerd/cmd/containerd/server/server.go
Line 494 in 6cbc285
|
/cc @dmcgowan |
akhilerm
force-pushed
the
fix-mirror-migration
branch
from
752cce9 to
b00ed38
Compare
| // if mirrors are present, prefer using the mirrors and set the | ||
| // config_path to empty. | ||
| if len(config.Registry.Mirrors) > 0 { | ||
| config.Registry.ConfigPath = "" |
There was a problem hiding this comment.
In previous releases, this would cause an error, right? I can understand some users may have migrated to a breaking config, but erroring is more consistent than silently clearing out a field. Is there another case this is trying to cover?
There was a problem hiding this comment.
In previous releases, this would cause an error, right?
In 2.2 or 1.x series?
I can understand some users may have migrated to a breaking config
But that would have happened only after defaulting the config_path values right?
if we error out here, it will be same as validation, i.e cannot have both config path and mirrors set at the same time and error out.
akhilerm
force-pushed
the
fix-mirror-migration
branch
2 times, most recently
from
c8d977c to
8a84437
Compare
| // config path are empty. | ||
| // This also makes sure that if the loaded config already contains both config path and mirrors | ||
| // the validation will fail. | ||
| if config.Registry.ConfigPath == "" && len(config.Registry.Mirrors) == 0 { |
There was a problem hiding this comment.
@dmcgowan This should ideally solve the issue. also not clearing any values that the users might have already set.
There was a problem hiding this comment.
I think the biggest downside of this is that containerd config default will no longer have the config_path value. But it's unclear to me how much anyone might depend on that, and the deleted code already did something similar so...I think this is probably fine for now.
There was a problem hiding this comment.
Tools like nvidia-container-toolkit uses the containerd config default command to create the config that is used as the base for the drop in config. AFAIK, they modify only the runtime key and not the images key. But will verify if config_path is used when some options are enabled.
akhilerm
force-pushed
the
fix-mirror-migration
branch
from
8a84437 to
db6e225
Compare
akhilerm
force-pushed
the
fix-mirror-migration
branch
from
db6e225 to
78a41d8
Compare
chrishenzie
left a comment
chrishenzie
left a comment
There was a problem hiding this comment.
Thanks for working on this! We're currently hitting this bug internally, and it's blocking our adoption of containerd 2.2. Is there any pending feedback or anything we can do to help move this along?
samuelkarp
force-pushed
the
fix-mirror-migration
branch
from
78a41d8 to
d58df07
Compare
github-project-automation
Bot
moved this from Needs Reviewers
to Review In Progress
in Pull Request Review
github-project-automation
Bot
moved this from Review In Progress
to Needs Update
in Pull Request Review
move setting the config_path from default image config to plugin init. only set the default value when both mirrors and config_path are empty, in all other cases retain the existing behaviour Co-authored-by: Samuel Karp <[email protected]> Signed-off-by: Akhil Mohan <[email protected]> Signed-off-by: Samuel Karp <[email protected]>
samuelkarp
force-pushed
the
fix-mirror-migration
branch
from
d58df07 to
1d77b68
Compare
samuelkarp
dismissed
their stale review
I've amended the commit to fix the problem.
dmcgowan
added
the
cherry-pick/2.2.x
github-project-automation
Bot
moved this from Review In Progress
to Done
in Pull Request Review
|
/cherrypick release/2.2 |
|
@samuelkarp: new pull request created: #12987 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
chrishenzie
added
cherry-picked/2.2.x
6 participants
when config has registry.mirrors from config version 2, after migration, config_path should be set to empty and not be the default value
move setting the config_path from default image config to plugin init.
This makes sure that when both mirrors and config_path are set, mirrors
are given preference and does not cause an error with CRI plugin
Fixes: #12612