[WIP] add mounts to nodes #302
Conversation
|
[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 |
| @@ -54,6 +56,9 @@ type Node struct { | |||
| // KubeadmConfigPatchesJSON6902 are applied to the generated kubeadm config | |||
| // as patchesJson6902 to `kustomize build` | |||
| KubeadmConfigPatchesJSON6902 []kustomize.PatchJSON6902 | |||
| // Mounts describes additional mount points for the node container | |||
| // These may be used to EG bind a hostpath | |||
| Mounts []cri.Mount `json:"mounts,omitempty"` | |||
There was a problem hiding this comment.
AFAIK there is a API "rule" somewhere in k8s (i don't know where it is), which says that you cannot embed an API in another API and graduate the second API to beta if the embedded API stays at alpha.
that's one of the reasons we had the component configs in kubeadm (e.g. KubeProxyConfiguration) as separate structures in the YAML before we moved to v1beta1.
this forces a structure in the lines of:
kind: Config
version: kind.sigs.k8s.io/v1beta1 # eventually
---
kind: HostMountConfig
version: kind.sigs.k8s.io/v1alphaX # has to stay alpha
There was a problem hiding this comment.
yeah, I'm debating if we should have just copied the types instead since we need our own shim to docker as well. looking around for that doc. 👍
There was a problem hiding this comment.
and
There was a problem hiding this comment.
technically having an alpha level field seems to be preferred now, though is expected to be feature gated.
Also: we're not actually embedding an kubernetes API per-se, Mount has no type meta, it is only a substructure that is also in another GRPC API used internally.
There was a problem hiding this comment.
technically having an alpha level field seems to be preferred now, though is expected to be feature gated.
yes, although we were still blocked graduating to beta if we were embedding kubeproxy - i cannot find an exact "clause for that" in the above docs.
possibly this is the corner case where kubeproxy was not gated because it's an essential addon.
others from the SIG have more context here.
but i think this should be a pointer to a slice if omitifempty is used on the field.
marshaling omitempty has weird side effects:
kubernetes/kubeadm#1358 (comment)
Also: we're not actually embedding an kubernetes API per-se, Mount has no type meta, it is only a substructure that is also in another GRPC API used internally.
indeed, it does not have metadata but the structure is tagged as alpha and is a subject to change. so it might be a requirement for kind to gate the field. if we want to follow the rules that is.
There was a problem hiding this comment.
The type is actually:
type Mount struct {
ContainerPath string `protobuf:"bytes,1,opt,name=container_path,json=containerPath,proto3" json:"container_path,omitempty"`
HostPath string `protobuf:"bytes,2,opt,name=host_path,json=hostPath,proto3" json:"host_path,omitempty"`
Readonly bool `protobuf:"varint,3,opt,name=readonly,proto3" json:"readonly,omitempty"`
SelinuxRelabel bool `protobuf:"varint,4,opt,name=selinux_relabel,json=selinuxRelabel,proto3" json:"selinux_relabel,omitempty"`
Propagation MountPropagation `protobuf:"varint,5,opt,name=propagation,proto3,enum=runtime.v1alpha2.MountPropagation" json:"propagation,omitempty"`
}But I'm switching to using this type within the Node:
// Mount describes a container mount
type Mount struct {
cri.Mount `json:",inline"`
}
That way we can always 100% inline the fields and break with CRI if we have to (besides the fact that we should upgrade CRI carefully).
Realistically it sounds like CRI is more or less beta. There haven't been any breaking changes from our POV at all since it was bumped to v1alpha2 a year ago (and not many changes in general)
https://github.com/kubernetes/kubernetes/commits/master/pkg/kubelet/apis/cri/runtime/v1alpha2
There was a problem hiding this comment.
indeed, it does not have metadata but the structure is tagged as alpha and is a subject to change. so it might be a requirement for kind to gate the field. if we want to follow the rules that is.
We should follow the rules. It's not clear to me that they apply to a vendored type w/o typemeta etc.
If it does apply then we might need to featuregate. The field is optional so we have that part.
From a marshalling POV we can avoid breakage by wrapping the type like I mentioned above. This won't work in memory though. 🤔
There was a problem hiding this comment.
But I'm switching to using this type within the Node:
well but this doesn't really makes the alpha type opague.
it should be:
type Mount struct {
mount cri.Mount
...
// add private state vars if needed.
}
and use composition with public getters and setters
but i think we should consult API machinery here too.
¯\_(ツ)_/¯
|
Also FWIW I sent out some feelers about moving CRI to a staging library, it seems like this would be acceptable, it just isn't a priority right now. If we start using it more than just this one type, then perhaps we can pitch in to move it to staging :-) |
|
/hold |
|
#313 implements an alternate route. |
|
going with the other PR instead. |
|
@BenTheElder: Closed this PR. In 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/test-infra repository. |
* 1st part reviewed * file deleted * files renamed and deleted * Doc changes * CODEOWNERS file * Fix eks reference --------- Co-authored-by: iamjanr <iamjanr007@gmail.com> Co-authored-by: Francisco Augusto <faugusto@stratio.com>
fixes #62
TODO: actually add the mounts 😉
I restarted this after #299.
/cc @neolit123