mount node product_uuid and product_name in pod containers #2321

qinqon · 2021-06-22T09:37:38Z

When pods are running at kind cluster their product_uuid and product_name is the same since the share the kernel vfs, this PR add a new mount to OCI spec to bind mount node's product_uuid and product_name into pod's containers.
This is the result

$ kubectl exec nginx-kind-worker cat /sys/class/dmi/id/product_uuid
053ba73c-3a24-4cfe-b7ca-5a938a4600d7
$ kubectl exec nginx-kind-worker2 cat /sys/class/dmi/id/product_uuid
db9f435b-0316-4f66-92a0-8d3632d6f69c

Closes ##2318

k8s-ci-robot · 2021-06-22T09:37:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qinqon
To complete the pull request process, please assign amwat after the PR has been reviewed.
You can assign the PR to them by writing /assign @amwat in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2021-06-22T09:37:45Z

Hi @qinqon. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

qinqon · 2021-06-22T10:41:06Z

/hold
not ready

BenTheElder

TIL base_runtime_spec. Neat!

images/base/Dockerfile

BenTheElder · 2021-06-22T14:53:15Z

images/base/Dockerfile

@@ -144,6 +145,7 @@ RUN echo "Installing containerd ..." \
    && chmod 755 /usr/local/sbin/runc \
    && containerd --version \
    && runc --version \
+    && ctr oci spec |jq '.mounts[.mounts | length] |= . + {"destination": "/sys/class/dmi/id/product_uuid", "source": "/proc/sys/kernel/random/uuid", "options": ["bind"]}'> /etc/containerd/cri-base.json \


Mount should be read only?
Why wouldn't we mount the file from the kind node instead of a random file per container?

Do we have to mimic https://github.com/kubernetes-sigs/kind/blob/main/images/base/files/usr/local/bin/entrypoint#L280-L289 too ?

BenTheElder · 2021-06-22T14:54:47Z

images/base/files/etc/containerd/config.toml

+  # base_runtime_spec is a file path to a JSON file with the OCI spec that will be used as the base spec that all
+  # container's are created from.
+  # Use containerd's `ctr oci spec > /etc/containerd/cri-base.json` to output initial spec file.
+  # Spec files are loaded at launch, so containerd daemon must be restarted on any changes to refresh default specs.


We only need to justify why were are setting this, we can leave the rest to the upstream docs without repeating all of it here. We can also see what we're doing in the referenced file in the other sources.

qinqon · 2021-06-23T11:41:03Z

@BenTheElder I have move to the entrypoint the modification of OCI spec so we can do like other product_uuid https://github.com/kubernetes-sigs/kind/compare/b31b3842d57c6e712eb2332baafeb58e7e23ca3f..e196b8abd76be173bdb277665034edaccfb20ebc

But I was not able to do it for the virtual one, since /sys/device is mounted read only.

Jun 23 13:54:29 kind-control-plane containerd[238]: time="2021-06-23T13:54:29.230084841Z" level=error msg="StartContainer for \"262a111c5a5efe56fd2b57a1b07f737b646852ffa7074836021d94fc18612f8b\" failed" error="failed to create containerd task: failed to create shim: OCI runtime create failed: container_linux.go:380: starting container process caused: process_linux.go:545: container init caused: rootfs_linux.go:76: mounting \"/kind/product_uuid\" to rootfs at \"/sys/device/virtual/dmi/id/product_uuid\" caused: mkdir /run/containerd/io.containerd.runtime.v2.task/k8s.io/262a111c5a5efe56fd2b57a1b07f737b646852ffa7074836021d94fc18612f8b/rootfs/sys/device: read-only file system: unknown"

BenTheElder · 2021-06-25T06:08:08Z

images/base/files/usr/local/bin/entrypoint

  fi
  if [[ -f /sys/devices/virtual/dmi/id/product_uuid ]]; then
    echo 'INFO: faking /sys/devices/virtual/dmi/id/product_uuid as well' >&2
    mount -o ro,bind /kind/product_uuid /sys/devices/virtual/dmi/id/product_uuid
+    #cat <<< "$(jq '.mounts[.mounts | length] |= . + {"destination": "/sys/device/virtual/dmi/id/product_uuid", "source": "/kind/product_uuid", "options": ["ro", "bind"]}' < /etc/containerd/cri-base.json)" > /etc/containerd/cri-base.json


remove commented code?
edit: or just fix the typo 😅 , see #2321 (comment)

Ups.... fixed, thanks!

BenTheElder · 2021-06-25T06:20:59Z

sorry, will get back to this tomorrow, I'm probably still the best person to review this but I've been a bit behind.

But I was not able to do it for the virtual one, since /sys/device is mounted read only.

I think the problem is actually just that it should be /sys/devices (plural) not /sys/device. Besides the existing reference in the entrypoint script, see the docs at https://man7.org/linux/man-pages/man5/sysfs.5.html 😅

qinqon · 2021-06-25T07:44:15Z

The k8s conformance tests does not check product_uuid at pods right ?

BenTheElder · 2021-06-25T07:55:03Z

The k8s conformance tests does not check product_uuid at pods right ?

not as far as I know, but some things try to read product_name as well (kubelet does, not the tests), we might also want to mount that while we're at it.

qinqon · 2021-06-25T07:58:23Z

The k8s conformance tests does not check product_uuid at pods right ?

not as far as I know, but some things try to read product_name as well (kubelet does, not the tests), we might also want to mount that while we're at it.

I was thinking about adding some tests for this, but maybe it's not worth it, I can put in place an extra commit with product_name

I see product_name is "hacked" too

kind/images/base/files/usr/local/bin/entrypoint

Lines 262 to 270 in 9fe9644

    
           fix_product_name() { 
        
             # this is a small fix to hide the underlying hardware and fix issue #426 
        
             # https://github.com/kubernetes-sigs/kind/issues/426 
        
             if [[ -f /sys/class/dmi/id/product_name ]]; then 
        
               echo 'INFO: faking /sys/class/dmi/id/product_name to be "kind"' >&2 
        
               echo 'kind' > /kind/product_name 
        
               mount -o ro,bind /kind/product_name /sys/class/dmi/id/product_name 
        
             fi 
        
           }

UPDATE: Confirmed that product_name has the same issue

$ for pod in $(kubectl get pod -n kube-system -o wide  |grep kube-proxy |awk '{print $1}'); do kubectl exec -n kube-system $pod cat /sys/class/dmi/id/product_name /sys/devices/virtual/dmi/id/product_name ;done
20HGS22D0C
20HGS22D0C
20HGS22D0C
20HGS22D0C
20HGS22D0C
20HGS22D0C

20HGS22D0C is my laptop's product_name

UPDATE2:

The product_name at "nodes"

[ellorent@localhost images]$ docker exec kind-control-plane cat /sys/devices/virtual/dmi/id/product_name /sys/devices/virtual/dmi/id/product_name
kind
kind
[ellorent@localhost images]$ docker exec kind-worker  cat /sys/devices/virtual/dmi/id/product_name /sys/devices/virtual/dmi/id/product_name
kind
kind
[ellorent@localhost images]$ docker exec kind-worker2  cat /sys/devices/virtual/dmi/id/product_name /sys/devices/virtual/dmi/id/product_name
kind
kind

qinqon · 2021-06-25T08:02:16Z

/hold cancel

working fine now:

$ for pod in $(kubectl get pod -n kube-system -o wide  |grep kube-proxy |awk '{print $1}'); do kubectl exec -n kube-system $pod cat /sys/class/dmi/id/product_uuid /sys/devices/virtual/dmi/id/product_uuid ;done
5fec4d4d-826c-4c5d-9ab4-eb7e811d2f47
5fec4d4d-826c-4c5d-9ab4-eb7e811d2f47
da43c828-f1e0-4ce4-a842-40b3d5a0d828
da43c828-f1e0-4ce4-a842-40b3d5a0d828
bddd0385-2f21-4335-9875-530e9a2deaae
bddd0385-2f21-4335-9875-530e9a2deaae

BenTheElder · 2021-08-12T17:28:18Z

This should be better implemented in containerd rather than modifying OCI bundles

@AkihiroSuda what would you propose the abstraction be like?

OCI gets us close with "insert some additional standard mounts in the base config" but it seems we have contention?/performance issues if we don't do a file per container. I'm not sure what would be reasonable for suggesting containerd do this.

AkihiroSuda · 2021-08-26T09:35:08Z

what would you propose the abstraction be like?

I was thinking that we could just have a new boolean like bind_mount_dmi_product in /etc/containerd/config.toml, but not a strong opinion.

But at least, let's avoid parsing JSON with sed.

qinqon · 2021-09-01T06:04:39Z

what would you propose the abstraction be like?

I was thinking that we could just have a new boolean like bind_mount_dmi_product in /etc/containerd/config.toml, but not a strong opinion.

But at least, let's avoid parsing JSON with sed.

Going back to installing jq at node containers and use it is ok ?

AkihiroSuda · 2021-09-01T07:33:44Z

Going back to installing jq at node containers and use it is ok ?

Yes, SGTM (non-binding)

qinqon · 2021-09-01T09:16:42Z

Going back to installing jq at node containers and use it is ok ?

Yes, SGTM (non-binding)

Added a commit with it, since jq is quite small It may be not an issue to keep it installed at the node container.

qinqon · 2021-09-06T14:55:34Z

@AkihiroSuda, this is ready now with your suggestion.

qinqon · 2021-09-10T05:16:52Z

@BenTheElder is this PR ok, or do it present any problem?

aojea · 2021-09-17T09:58:13Z

I'm fine with merging and iterating, the issue is legit #2318 and I think that we have explored all the possibles alternatives.
+1
Please squash and remove the commit for the custom images
Thanks

When pods are running at kind cluster their product_uuid and product_name is the same since the share the kernel vfs, this PR add a new mount to OCI spec to bind mount node's product_uuid and product_name into pod's containers. Signed-off-by: Quique Llorente <ellorent@redhat.com>

qinqon · 2021-09-17T10:00:44Z

I'm fine with merging and iterating, the issue is legit #2318 and I think that we have explored all the possibles alternatives.
+1
Please squash and remove the commit for the custom images
Thanks

Done

k8s-ci-robot · 2021-09-17T10:40:57Z

@qinqon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-kind-e2e-kubernetes	`9834db3`	link	`/test pull-kind-e2e-kubernetes`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

qinqon · 2021-09-22T05:38:50Z

Bum image blocked by docker/buildx#772

aojea · 2021-10-08T13:27:51Z

This will go in this PR #2465 , with the base image bump
Thanks

BenTheElder · 2021-10-14T17:57:41Z

rebased version merged in #2465, thank you!

The new KinD k8s-1.22 nodes includes a change [1] [2] that allows us to stop using PodPresets and still support Kubevirt Live Migration. [1] kubernetes-sigs/kind#2465 [2] kubernetes-sigs/kind#2321 Signed-off-by: Or Mergi <ormergi@redhat.com>

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2021

k8s-ci-robot requested review from krzyzacy and munnerz June 22, 2021 09:37

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 22, 2021

qinqon mentioned this pull request Jun 22, 2021

pods running at different nodes should have different product_uuid #2318

Closed

qinqon force-pushed the random-product-uuid branch from d00c823 to fae02be Compare June 22, 2021 10:34

qinqon marked this pull request as ready for review June 22, 2021 10:40

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2021

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

BenTheElder reviewed Jun 22, 2021

View reviewed changes

qinqon force-pushed the random-product-uuid branch from fae02be to b31b384 Compare June 22, 2021 15:04

qinqon requested a review from BenTheElder June 22, 2021 15:06

qinqon force-pushed the random-product-uuid branch from b31b384 to e196b8a Compare June 23, 2021 11:39

BenTheElder reviewed Jun 25, 2021

View reviewed changes

qinqon force-pushed the random-product-uuid branch from e196b8a to 79d9106 Compare June 25, 2021 07:41

qinqon requested a review from BenTheElder June 25, 2021 07:41

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

BenTheElder changed the title ~~Generate a random product_uuid at containerd~~ mount node product_uuid in pod containers Jun 25, 2021

qinqon force-pushed the random-product-uuid branch from 79d9106 to 40ca52e Compare June 25, 2021 08:13

ormergi mentioned this pull request Aug 10, 2021

SRIOV provider: Deploy default CNI kubevirt/kubevirtci#644

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021

qinqon force-pushed the random-product-uuid branch from 36046ef to 15d0be9 Compare September 1, 2021 09:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2021

qinqon requested a review from AkihiroSuda September 1, 2021 09:16

qinqon force-pushed the random-product-uuid branch from 15d0be9 to 9834db3 Compare September 17, 2021 10:00

aojea mentioned this pull request Sep 18, 2021

Bump base image #2465

Merged

aojea added this to the v0.12.0 milestone Oct 8, 2021

BenTheElder closed this Oct 14, 2021

ormergi mentioned this pull request Oct 20, 2021

Upgrade SR-IOV provider nodes image to k8s-1.22 kubevirt/kubevirtci#694

Merged

unixlab mentioned this pull request Nov 11, 2021

limit of file descriptors inside a container always is 1024 #2532

Closed

BenTheElder mentioned this pull request Nov 22, 2021

Pods with non-standard PATH can fail to start #2551

Closed

qinqon mentioned this pull request Sep 30, 2022

REQUEST: New membership for qinqon kubernetes/org#3740

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mount node product_uuid and product_name in pod containers #2321

mount node product_uuid and product_name in pod containers #2321

qinqon commented Jun 22, 2021 •

edited

Loading

k8s-ci-robot commented Jun 22, 2021

k8s-ci-robot commented Jun 22, 2021

qinqon commented Jun 22, 2021

BenTheElder left a comment

BenTheElder Jun 22, 2021

qinqon Jun 22, 2021

BenTheElder Jun 22, 2021

qinqon Jun 22, 2021

qinqon commented Jun 23, 2021 •

edited

Loading

BenTheElder Jun 25, 2021 •

edited

Loading

qinqon Jun 25, 2021

BenTheElder commented Jun 25, 2021

qinqon commented Jun 25, 2021 •

edited

Loading

BenTheElder commented Jun 25, 2021

qinqon commented Jun 25, 2021 •

edited

Loading

qinqon commented Jun 25, 2021

BenTheElder commented Aug 12, 2021

AkihiroSuda commented Aug 26, 2021

qinqon commented Sep 1, 2021

AkihiroSuda commented Sep 1, 2021

qinqon commented Sep 1, 2021

qinqon commented Sep 6, 2021 •

edited

Loading

qinqon commented Sep 10, 2021

aojea commented Sep 17, 2021

qinqon commented Sep 17, 2021

k8s-ci-robot commented Sep 17, 2021

qinqon commented Sep 22, 2021

aojea commented Oct 8, 2021

BenTheElder commented Oct 14, 2021

mount node product_uuid and product_name in pod containers #2321

mount node product_uuid and product_name in pod containers #2321

Conversation

qinqon commented Jun 22, 2021 • edited Loading

k8s-ci-robot commented Jun 22, 2021

k8s-ci-robot commented Jun 22, 2021

qinqon commented Jun 22, 2021

BenTheElder left a comment

Choose a reason for hiding this comment

BenTheElder Jun 22, 2021

Choose a reason for hiding this comment

qinqon Jun 22, 2021

Choose a reason for hiding this comment

BenTheElder Jun 22, 2021

Choose a reason for hiding this comment

qinqon Jun 22, 2021

Choose a reason for hiding this comment

qinqon commented Jun 23, 2021 • edited Loading

BenTheElder Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

qinqon Jun 25, 2021

Choose a reason for hiding this comment

BenTheElder commented Jun 25, 2021

qinqon commented Jun 25, 2021 • edited Loading

BenTheElder commented Jun 25, 2021

qinqon commented Jun 25, 2021 • edited Loading

qinqon commented Jun 25, 2021

BenTheElder commented Aug 12, 2021

AkihiroSuda commented Aug 26, 2021

qinqon commented Sep 1, 2021

AkihiroSuda commented Sep 1, 2021

qinqon commented Sep 1, 2021

qinqon commented Sep 6, 2021 • edited Loading

qinqon commented Sep 10, 2021

aojea commented Sep 17, 2021

qinqon commented Sep 17, 2021

k8s-ci-robot commented Sep 17, 2021

qinqon commented Sep 22, 2021

aojea commented Oct 8, 2021

BenTheElder commented Oct 14, 2021

qinqon commented Jun 22, 2021 •

edited

Loading

qinqon commented Jun 23, 2021 •

edited

Loading

BenTheElder Jun 25, 2021 •

edited

Loading

qinqon commented Jun 25, 2021 •

edited

Loading

qinqon commented Jun 25, 2021 •

edited

Loading

qinqon commented Sep 6, 2021 •

edited

Loading