New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kublet-wrapper won't start after upgrade to 1353.2.0 with /var/logs already mounted #1892

Closed
dadux opened this Issue Mar 30, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@dadux

dadux commented Mar 30, 2017

Bug

Container Linux Version

1353.2.0

Expected Behavior

A upgrade of kublet-wrapper shouldn't prevent it to start.

Actual Behavior

We use RKT_OPTIONS to mount /var/log as the kublet symlinks the docker logs with the pod information. It's usefull for fluentd.

The latest kubelet-wrapper seems to have added the /var/log mount, resulting in having it twice on the rkt command line.
Rkt errors with the following error message :
wrapper[10308]: run: can't evaluate mounts: missing mount for volume "var-log"

Reproduction Steps

  1. Start the kubelet-wrapper with RKT_OPTIONS

@dadux dadux changed the title from Kublet-wrapper won't start after upgrade to 1353.2.0 to Kublet-wrapper won't start after upgrade to 1353.2.0 with /var/logs already mounted Mar 30, 2017

@edevil

This comment has been minimized.

Show comment
Hide comment
@edevil

edevil Apr 7, 2017

Eeck, I guess I'll have this problem as well if reaches Stable.

edevil commented Apr 7, 2017

Eeck, I guess I'll have this problem as well if reaches Stable.

@euank

This comment has been minimized.

Show comment
Hide comment
@euank

euank Apr 7, 2017

Contributor

We won't let it reach stable.

If we don't have a better solution, we can replace the hardcoded volume name in kublet-wrapper with a UUID to ensure we don't conflict with user's names, but I wanted to try and find a better solution first.

Contributor

euank commented Apr 7, 2017

We won't let it reach stable.

If we don't have a better solution, we can replace the hardcoded volume name in kublet-wrapper with a UUID to ensure we don't conflict with user's names, but I wanted to try and find a better solution first.

@euank euank self-assigned this Apr 24, 2017

@euank

This comment has been minimized.

Show comment
Hide comment
@euank

euank Apr 24, 2017

Contributor

It turns out this is far more complicated than I initially thought. I shouldn't have put off looking into this for so long!

If two mounts have the same target, stage1-fly actually "dedupes" them.

Basically, the bug is:

$ sudo rkt run --stage1-name=coreos.com/rkt/stage1-fly --insecure-options=image,ondisk --volume=name1,kind=host,source=/tmp --mount volume=name1,target=/tmp --volume=name2,kind=host,source=/tmp --mount volume=name2,target=/tmp docker://busybox -- -c 'ls /tmp'
run: can't evaluate mounts: missing mount for volume "name2"

However, if the target differs they don't get merged and things work, or if the names are identical then they merge identically and work.

It seems like the actual fix is going to be a patch to rkt fly to make it's mount logic closer to the other stage1's mount logic.

Contributor

euank commented Apr 24, 2017

It turns out this is far more complicated than I initially thought. I shouldn't have put off looking into this for so long!

If two mounts have the same target, stage1-fly actually "dedupes" them.

Basically, the bug is:

$ sudo rkt run --stage1-name=coreos.com/rkt/stage1-fly --insecure-options=image,ondisk --volume=name1,kind=host,source=/tmp --mount volume=name1,target=/tmp --volume=name2,kind=host,source=/tmp --mount volume=name2,target=/tmp docker://busybox -- -c 'ls /tmp'
run: can't evaluate mounts: missing mount for volume "name2"

However, if the target differs they don't get merged and things work, or if the names are identical then they merge identically and work.

It seems like the actual fix is going to be a patch to rkt fly to make it's mount logic closer to the other stage1's mount logic.

@euank

This comment has been minimized.

Show comment
Hide comment
@euank

euank May 9, 2017

Contributor

The upstream rkt bug has been fixed, this is now pending release of rkt v1.26. It won't make this alpha, but will make the next one.
As soon as rkt is updated, changing the kubelet-wrapper's mounts to include a unique prefix should fix this.

Contributor

euank commented May 9, 2017

The upstream rkt bug has been fixed, this is now pending release of rkt v1.26. It won't make this alpha, but will make the next one.
As soon as rkt is updated, changing the kubelet-wrapper's mounts to include a unique prefix should fix this.

@edevil

This comment has been minimized.

Show comment
Hide comment
@edevil

edevil May 10, 2017

So, if I understand your comments correctly, the current problem is when you have 2 mounts with a different name, but with the same target? Is this in stable?

edevil commented May 10, 2017

So, if I understand your comments correctly, the current problem is when you have 2 mounts with a different name, but with the same target? Is this in stable?

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab May 10, 2017

Member

@edevil for reference, these are the discussions on rkt side: rkt/rkt#3663 and rkt/rkt#3666.

Root cause is a mix of: 1) not checking for multiple volumes 2) non-uniform deduplication of mounts 3) wrong logic in fly which didn't allow for unused volumes.

Member

lucab commented May 10, 2017

@edevil for reference, these are the discussions on rkt side: rkt/rkt#3663 and rkt/rkt#3666.

Root cause is a mix of: 1) not checking for multiple volumes 2) non-uniform deduplication of mounts 3) wrong logic in fly which didn't allow for unused volumes.

@edevil

This comment has been minimized.

Show comment
Hide comment
@edevil

edevil May 10, 2017

Ok, I just wanted to be sure that in my case upgrade will not break my kubernetes clusters. This is the value that I use for RKT_RUN_ARGS:
RKT_RUN_ARGS=--uuid-file-save=/var/run/kubelet-pod.uuid --volume var-cni,kind=host,source=/var/lib/cni --mount volume=var-cni,target=/var/lib/cni --volume var-log,kind=host,source=/var/log --mount volume=var-log,target=/var/log --volume lib-modules,kind=host,source=/lib/modules --mount volume=lib-modules,target=/lib/modules --volume etc-resolv,kind=host,source=/etc/resolv.conf --mount volume=etc-resolv,target=/etc/resolv.conf

Of these, only /var/log seems to be duplicated. And since it has the same name as in here, this will not be a problem, right?

edevil commented May 10, 2017

Ok, I just wanted to be sure that in my case upgrade will not break my kubernetes clusters. This is the value that I use for RKT_RUN_ARGS:
RKT_RUN_ARGS=--uuid-file-save=/var/run/kubelet-pod.uuid --volume var-cni,kind=host,source=/var/lib/cni --mount volume=var-cni,target=/var/lib/cni --volume var-log,kind=host,source=/var/log --mount volume=var-log,target=/var/log --volume lib-modules,kind=host,source=/lib/modules --mount volume=lib-modules,target=/lib/modules --volume etc-resolv,kind=host,source=/etc/resolv.conf --mount volume=etc-resolv,target=/etc/resolv.conf

Of these, only /var/log seems to be duplicated. And since it has the same name as in here, this will not be a problem, right?

@euank

This comment has been minimized.

Show comment
Hide comment
@euank

euank May 10, 2017

Contributor

@edevil that we be fine I think.

Regardless we're not updating any mounts in kubelet-wrapper on stable until rkt's updated so we can do so without fear of breaking things any further than they are

Contributor

euank commented May 10, 2017

@edevil that we be fine I think.

Regardless we're not updating any mounts in kubelet-wrapper on stable until rkt's updated so we can do so without fear of breaking things any further than they are

@lucab

This comment has been minimized.

Show comment
Hide comment
@lucab

lucab May 11, 2017

Member

@euank @bgilbert I think we'd still need to unique-prefix volume names to avoid clashes/dups with user-provided ones.

Member

lucab commented May 11, 2017

@euank @bgilbert I think we'd still need to unique-prefix volume names to avoid clashes/dups with user-provided ones.

stickycode referenced this issue in coreos/coreos-overlay Nov 21, 2017

app-admin/etcd-wrapper: vendor-prefix rkt volume names
Since rkt 1.26.0, duplicate volume names are invalid.  This avoids
clashing with common user volume names like var-log.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment