refactor container packages and support mounts #313

BenTheElder · 2019-02-17T04:58:17Z

move pkg/docker to pkg/container/docker, long term we can better break up docker specific vs general container functionality and support other container runtimes.
- tracking issue: support multiple container engines on the host #154
- see also: Attempting to make kind a little less Docker Centric #151, this is work towards a cleaner alternative
- see also: https://kind.sigs.k8s.io/docs/design/principles/#target-cri-functionality
create a CRI package, with types (Mount related only for now) copied from upstream
- this allows us to avoid stability concerns, if necessary we can provide our own conversion
- I added better JSON / yaml encoding for Mount's propagation field
  - I've tested that in the future we could instead use type Mount upstreamcri.Mount and still support this
add support for CRI Mounts in docker.Run, refactor docker.Run to take functional options
support mounts in the node config, update control-plane and worker node creation to support this

This is an alternative to #302 where we vendor CRI

cc @neolit123

k8s-ci-robot · 2019-02-17T04:58:21Z

[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:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/container/docker/run.go

BenTheElder · 2019-02-17T07:27:58Z

usage:

kind: Config
apiVersion: kind.sigs.k8s.io/v1alpha2
nodes:
- role: control-plane
  mounts:
  - container_path: /kind-source
    host_path: /Users/bentheelder/go/src/sigs.k8s.io/kind
    readonly: true

Things to consider for the future (but I think I'd rather not do in the first PR to keep the diff down):

swap out the snakeCase json fields for camelCase fields to match the rest of our config?
resolve environment variables in host_path or at least resolve relative paths with respect to the kind working directory. Docker requires absolute paths for bind mounts but that's not super great UX for our case I think ...

neolit123

this seems good @BenTheElder
i just need to do some quick testing for Mounts *[]cri.Mount

neolit123 · 2019-02-17T15:05:09Z

pkg/cluster/config/types.go

@@ -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 is a weird side effect when marshaling slices with omitempty, that is already a problem in k/k, kubeadm, cluster-api. i think the safe route was to use Mounts *[]cri.Mount json:"mounts,omitempty"` but i need to check this.

package main import ( "fmt" "log" "sigs.k8s.io/yaml" ) type A struct { Mounts []string `json:"mounts,omitempty"` } func main() { a := A{Mounts: []string{}} // <--------- [*] fmt.Println(a.Mounts, a.Mounts == nil) b, err := yaml.Marshal(a) if err != nil { log.Fatal(err) } fmt.Println(string(b)) c := A{} err = yaml.Unmarshal(b, &c) if err != nil { log.Fatal(err) } fmt.Println(c.Mounts, c.Mounts == nil) }

[] false {} [] true

[*]
so this problem is relevant only if we assign a meaning to the "empty" value of Mounts and what is the "empty" value (is it a empty slice or nil) and if the user is marhaling the kind Config.

if there is no default value this problem is not relevant, but just referencing this here so that we don't have this problem for the future kind config:
kubernetes/kubeadm#1358

~~What was it? https://play.golang.org/p/EOfDnWThpdj~~
Your comment hadn't loaded

I think we can treat empty and nil the same, the node already has mounts (tmps and one docker volume) we're just adding additional mounts here (we should clarify that in the docs)

hm, yes. i know the existing mounts are essential for the setup to work.

so in a way we already have default mounts and these mounts are extra mounts 🤔
would it be clearer naming this ExtraMounts and including some more notes in the field comment?

Yeah I think that might make sense.

renamed it 👍

neolit123 · 2019-02-17T15:09:29Z

pkg/container/cri/json.go

+	}{
+		Alias: (*Alias)(m),
+	}
+	if err := json.Unmarshal(data, &aux); err != nil {


ideally everything "unmarshall" should be using DisallowUnknownFields
but i leave this to you.

Yeah, trying to figure out if we should just do that inline or if there's a way to plumb through the option 🙄

I'm going to tackle this in a follow up, but before the next release, I'm not sure how this will interact with the rest of the decoding logic.

i'm still hopeful for getting support for that upstream in API machinery.

neolit123 · 2019-02-17T15:32:36Z

pkg/container/cri/types.go

+	// If set, the mount needs SELinux relabeling.
+	SelinuxRelabel bool `protobuf:"varint,4,opt,name=selinux_relabel,json=selinuxRelabel,proto3" json:"selinux_relabel,omitempty"`
+	// Requested propagation mode.
+	Propagation MountPropagation `protobuf:"varint,5,opt,name=propagation,proto3,enum=runtime.v1alpha2.MountPropagation" json:"propagation,omitempty"`


looks like these variable names need "adjustments" upstream.

Yeah I'm not sure if that's actually an option. I think it would probably be fine for us to match the types closely but use a different on-disk format.

we still have the option to shim with getters and setters. this means some extra boiler plate.
this fixes both the field names and hides the upstream config.

create a CRI package, with types (Mount related only for now) copied from upstream

this allows us to avoid stability concerns, if necessary we can provide our own conversion

i need to know the roadmap for CRI in terms of do they have plans to remove or rename fields, because if there are upstream API changes our local type can end up invalid and we will have to break kind users because these fields are public in the kind config.

i will leave this to you at this point if you want us to proceed, but comments from others would be appreciated too.

CRI is pretty stable at this point, this type hasn't actually changed in years.

But our local type can be a shim just closely matching their type, if they diverge we will translate. As long as we are not using the client this isn't an issue.

Also for the types we expose here for on disk purposes, what they are capable of is unlikely to change. They cannot remove support for these fields without breaking Kubernetes majorly.

It is nice for now that they are literally the same definition, but if it diverges slightly (EG they add a field or break the type up) we can keep our changes compatible and translate.

The things we will need to expose in config like Mounts, DNS, ... those seem unlikely to change significantly.

More complex types like entire containers, those seem stable too but we don't need to expose those on disk anyhow.

I adjusted our names, strangely the proto json tags were already camelCase but not the actual json tags 🤷‍♂️

Where possible these match user facing API fields in Kubernetes APIs

neolit123 · 2019-02-17T15:33:20Z

pkg/container/cri/types.go

+	MountPropagation_PROPAGATION_HOST_TO_CONTAINER MountPropagation = 1
+	// Mounts get propagated from the host to the container and from the
+	// container to the host ("rshared" in Linux).
+	MountPropagation_PROPAGATION_BIDIRECTIONAL MountPropagation = 2


i've missed if we are adding a linter exception for this file/package.

We did, it's a separate commit.

ah, saw it.

neolit123 · 2019-02-17T17:57:32Z

/assign @munnerz
hi, do you have comments on the new CRI mount support?

BenTheElder · 2019-02-18T21:18:09Z

per discussion in the meeting I renamed to camel case and switched to the core/v1 VolumeMount strings for the mount enum. I think we can remove the lint exception now as well.

BenTheElder · 2019-02-19T02:41:35Z

Re-enabled golint and fixed remaining lints.

neolit123

as per the discussions today during the kind meeting LGTM.
/lgtm
/hold

BenTheElder · 2019-02-19T03:28:53Z

pkg/container/cri/types.go

+	// If set, the mount is read-only.
+	Readonly bool `protobuf:"varint,3,opt,name=readonly,proto3,json=readOnly,proto3" json:"readOnly,omitempty"`
+	// If set, the mount needs SELinux relabeling.
+	SelinuxRelabel bool `protobuf:"varint,4,opt,name=selinux_relabel,json=selinuxRelabel,proto3" json:"selinuxRelabel,omitempty"`


this one is not actually exposed directly in kubernetes volumes AFAICT, but from previous test-infra containerization work I remember control over this being necessary on some systems.

we should consider if we want to expose this or not, imho we can just document it well and expect the user to do the right thing, support for this shouldn't go anywhere in CRI

BenTheElder · 2019-02-19T03:31:50Z

/hold cancel

Will follow up with considerations on EG relative paths / other UX improvements.

In the meantime this config is now possible:

kind: Config
apiVersion: kind.sigs.k8s.io/v1alpha2
nodes:
- role: control-plane
  extraMounts:
  - containerPath: /kind-source
    hostPath: /Users/bentheelder/go/src/sigs.k8s.io/kind
    readOnly: true

* Print build version * Fix version * Bump golang image * Fix version * Fix version

move docker package to pkg/container/docker

26e4f9c

k8s-ci-robot requested review from amwat and neolit123 February 17, 2019 04:58

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2019

BenTheElder mentioned this pull request Feb 17, 2019

[WIP] add mounts to nodes #302

Closed

BenTheElder commented Feb 17, 2019

View reviewed changes

pkg/container/docker/run.go Show resolved Hide resolved

BenTheElder force-pushed the mountalias branch from dd61969 to 3e723af Compare February 17, 2019 07:17

BenTheElder added 6 commits February 16, 2019 23:21

add CRI package

9809d03

refactor docker run and support cri Mounts

4073e5c

ignore cri in linter

1bf5bec

add mounts to config nodes

d0280e4

hack/update-generated.sh

7cfeaee

add mounts to created nodes

0e68020

BenTheElder force-pushed the mountalias branch from 3e723af to 0e68020 Compare February 17, 2019 07:21

neolit123 reviewed Feb 17, 2019

View reviewed changes

k8s-ci-robot assigned munnerz Feb 17, 2019

BenTheElder added 2 commits February 17, 2019 18:24

rename mounts to extraMounts

9231ba3

improve mount type serialization and propogation enum

b484635

re-enable golint for cri package

045244b

BenTheElder force-pushed the mountalias branch from b863d2a to 045244b Compare February 19, 2019 02:33

neolit123 approved these changes Feb 19, 2019

View reviewed changes

k8s-ci-robot assigned neolit123 Feb 19, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2019

BenTheElder commented Feb 19, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2019

k8s-ci-robot merged commit 37389ea into kubernetes-sigs:master Feb 19, 2019

BenTheElder mentioned this pull request Feb 19, 2019

Allow mounting host host directories to kind nodes #62

Closed

BenTheElder deleted the mountalias branch February 19, 2019 05:45

sdudoladov mentioned this pull request May 3, 2019

Implement runner for e2e tests zalando/postgres-operator#548

Merged

stg-0 pushed a commit to stg-0/kind that referenced this pull request Oct 2, 2023

[CLOUD-52] Imprimir build de la version (kubernetes-sigs#313)

a076e19

* Print build version * Fix version * Bump golang image * Fix version * Fix version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor container packages and support mounts #313

refactor container packages and support mounts #313

BenTheElder commented Feb 17, 2019 •

edited

Loading

k8s-ci-robot commented Feb 17, 2019

BenTheElder commented Feb 17, 2019

neolit123 left a comment

neolit123 Feb 17, 2019

neolit123 Feb 17, 2019

BenTheElder Feb 17, 2019 •

edited

Loading

BenTheElder Feb 17, 2019

neolit123 Feb 17, 2019 •

edited

Loading

BenTheElder Feb 17, 2019

BenTheElder Feb 18, 2019

neolit123 Feb 17, 2019

BenTheElder Feb 17, 2019

BenTheElder Feb 19, 2019

neolit123 Feb 17, 2019

BenTheElder Feb 17, 2019

neolit123 Feb 17, 2019 •

edited

Loading

BenTheElder Feb 17, 2019 •

edited

Loading

neolit123 Feb 17, 2019

BenTheElder Feb 17, 2019

BenTheElder Feb 19, 2019

neolit123 Feb 17, 2019

BenTheElder Feb 17, 2019

neolit123 Feb 17, 2019

neolit123 commented Feb 17, 2019

BenTheElder commented Feb 18, 2019

BenTheElder commented Feb 19, 2019

neolit123 left a comment

BenTheElder Feb 19, 2019

BenTheElder commented Feb 19, 2019

refactor container packages and support mounts #313

refactor container packages and support mounts #313

Conversation

BenTheElder commented Feb 17, 2019 • edited Loading

k8s-ci-robot commented Feb 17, 2019

BenTheElder commented Feb 17, 2019

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

BenTheElder Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Feb 17, 2019

BenTheElder commented Feb 18, 2019

BenTheElder commented Feb 19, 2019

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Feb 19, 2019

BenTheElder commented Feb 17, 2019 •

edited

Loading

BenTheElder Feb 17, 2019 •

edited

Loading

neolit123 Feb 17, 2019 •

edited

Loading

neolit123 Feb 17, 2019 •

edited

Loading

BenTheElder Feb 17, 2019 •

edited

Loading