add limited support for version aware containerd config patching #4042
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold |
k8s-ci-robot
added
size/M
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| Version int `toml:"version,omitempty"` | ||
| } | ||
| v := version{} | ||
| if err := toml.Unmarshal([]byte(configTOML), &v); err != nil { |
There was a problem hiding this comment.
The version field in the config file specifies the config’s version. If no version number is specified inside the config file then it is assumed to be a version 1 config and parsed as such. Please use version = 2 to enable version 2 config as version 1 has been deprecated.
should we default to version 1 instead of error?
There was a problem hiding this comment.
version 1 isn't the default in containerd as far as I know, but the default would depend on containerd version and that seems fragile.
This is the file to be patched, we are always specifying the version ourselves, so if it doesn't have the version or we can't parse it, something is wrong.
since this is in an internal package already, I didn't think it was worth designing for other use cases (hence the rename to Containerd and the assumption about the version key)
There was a problem hiding this comment.
Ah also, maybe this wasn't obvious: We won't error if there is no version, error would mean it fails to parse as toml or it has a version key with a different data type or something like that.
(You can also confirm from the existing test cases without version specified continuing to work, in additon to the updated test cases)
We have version field as omitempty, so it defaults to 0 if not specified
| if err != nil { | ||
| return "", err | ||
| } | ||
| version, err := containerdConfigVersion(toPatch) |
There was a problem hiding this comment.
there is an assumption that we always get a valid version from the base configuration here, we could error if this is not non-zero here, or below we could always match if it is zero similar to how we ignore match for patches with version = 0
There was a problem hiding this comment.
I think failing to read this one should be an error, it should only happen if we changed our base config and didn't update this properly (because version parsing changed in containerd config?) or because someone has forked kind with a custom containerd base config that doesn't set version, which I don't really care to guard against.
There was a problem hiding this comment.
updated to require detecting the version in the config to be patched
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
BenTheElder
force-pushed
the
config-patch-aware
branch
from
d8f3f81 to
0de9b10
Compare
k8s-ci-robot
added
size/L
|
/test pull-kind-conformance-parallel-ipv6 Retesting to see whether the job works here (it fails in #4046). |
|
/test pull-kind-conformance-parallel-ipv6 |
5 participants
see: #3946