Skip to content
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

Support containerd 1.7 #297

Merged
merged 2 commits into from
May 12, 2023

Conversation

Mik4sa
Copy link
Contributor

@Mik4sa Mik4sa commented Apr 21, 2023

Reason for PR:

The kube-flannel and kube-proxy pods were failing/crashing with containerd 1.7.0. This was because of the usage of bind volume instead of symlink volume (see here).
This problem was first mentioned in #289 and here: #277 (comment)

For details check this comment: #277 (comment)

Issue Fixed:

Fixes #289

Requirements

  • Sqaush commits
  • Documentation
  • Tests

Notes:
I decided to move the mounts to /mounts/... instead of directly mounting them to the root because of two reasons:

  • This didn't worked for kube-proxy since the Dockerfile already writes into/uses /kube-proxy
  • /mounts/... maybe better communicates that these are actual mounts which was really hard for me to understand due to the behavior of hostprocess containers.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 21, 2023
@github-actions github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 21, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 21, 2023
@jsturtevant
Copy link
Contributor

I decided to move the mounts to /mounts/... instead of directly mounting them to the root because of two reasons:

I don't think the volumeMount:mountPath bind directly to the c:/folder on host. I would expect them to show up in c:/hpc/. @marosset can help clarify.

It feels like something else is going on here? does the C:\etc\kube-flannel\ folder exist on host?

@marosset
Copy link
Contributor

I decided to move the mounts to /mounts/... instead of directly mounting them to the root because of two reasons:

I don't think the volumeMount:mountPath bind directly to the c:/folder on host. I would expect them to show up in c:/hpc/. @marosset can help clarify.

It feels like something else is going on here? does the C:\etc\kube-flannel\ folder exist on host?

It might be that the working directory changed between containerd v1.6 and v1.7 too.
IIRC in v1.6 the working directory was set to $CONTAINER_SANDBOX_MOUNT_POINT and in v1.7 it is set to c:\hpc\

@jsturtevant
Copy link
Contributor

That was my expectation, was the Working dir changed. I am surprised that change the folder name is making a difference here.

@marosset
Copy link
Contributor

marosset commented May 1, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marosset, Mik4sa

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2023
@Mik4sa
Copy link
Contributor Author

Mik4sa commented May 5, 2023

@marosset What is missing here so this gets the lgtm label/gets merged?

ls $env:CONTAINER_SANDBOX_MOUNT_POINT/etc/kube-flannel/
cp -force $env:CONTAINER_SANDBOX_MOUNT_POINT/etc/kube-flannel/net-conf.json C:\etc\kube-flannel\net-conf.json
ls $env:CONTAINER_SANDBOX_MOUNT_POINT/mounts/kube-flannel/
cp -force $env:CONTAINER_SANDBOX_MOUNT_POINT/mounts/kube-flannel/net-conf.json C:\etc\kube-flannel\net-conf.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the value of CONTAINER_SANDBOX_MOUNT_POINT in this script with 1.7? I am guessing we are getting the error cp : Access to the path 'C:\etc\kube-flannel\net-conf.json' is denied because we are trying to copy from the wrong place. If CONTAINER_SANDBOX_MOUNT_POINT is not set then we would be trying to copy from c:/etc/kube-flannel/net-conf.json which isn't created yet (we are trying to do that now...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik 'C:/hpc' but I can't verify before wednesday/thursday

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the value of CONTAINER_SANDBOX_MOUNT_POINT in this script with 1.7?

It's C:\hpc\. Just verified.

I am guessing we are getting the error cp : Access to the path 'C:\etc\kube-flannel\net-conf.json' is denied because we are trying to copy from the wrong place.

I manually entered into the flannel pod and executed the commands one by one and made some other testing.
The source doesn't seem to be problematic. I'm able to copy it to a different folder. This for example works (after creating the target directory):
cp -force $env:CONTAINER_SANDBOX_MOUNT_POINT/etc/kube-flannel/net-conf.json C:\test\

But when I for example try to delete the target file with this command:
Remove-Item C:\etc\kube-flannel\net-conf.json
I get the following error:

Remove-Item : Cannot remove item C:\etc\kube-flannel\net-conf.json: Access to the path 'C:\etc\kube-flannel\net-conf.json' is denied.
At line:1 char:1
+ Remove-Item C:\etc\kube-flannel\net-conf.json
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : PermissionDenied: (C:\etc\kube-flannel\net-conf.json:FileInfo) [Remove-Item], UnauthorizedAccessException
    + FullyQualifiedErrorId : RemoveFileSystemItemUnAuthorizedAccess,Microsoft.PowerShell.Commands.RemoveItemCommand

This is because the configmap kube-flannel-cfg is mounted into this place (readonly). I verified that by editing the configmap first and then checking the content inside of the container:

PS C:\> cat C:\etc\kube-flannel\net-conf.json
{
  "Network": "10.244.0.0/16",
  "Backend": {
    "Type": "vxlan"
  }
  "Some falsy value..."
}

And this is the target directory structure:

PS C:\> ls C:\etc\kube-flannel\


    Directory: C:\etc\kube-flannel


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----         5/12/2023   2:54 PM                ..2023_05_12_14_54_31.2595385805
d----l         5/12/2023   2:54 PM                ..data
-a---l         5/12/2023   2:54 PM              0 cni-conf.json
-a---l         5/12/2023   2:54 PM              0 net-conf.json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I yes this has to do with the behavioral change with mounts in containerd 1.7 (which I think you already said. Thanks for bearing with me! 😄).

from https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/1981-windows-privileged-container-support#container-mounts

Additional volume mounts specified for hostProcess containers will be mounted at their requested location and can be access the same way as volume mounts in Linux or regular Windows Server containers.
ex: a volume with a mountPath of /var/run/secrets/token for containers will be mounted at c:\var\run\secrets\token for containers.

@jsturtevant
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2023
@k8s-ci-robot k8s-ci-robot merged commit 98215f4 into kubernetes-sigs:master May 12, 2023
@Mik4sa Mik4sa deleted the containerd-1.7-support branch May 12, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PermissionDenied: (C:\hpc\etc\kube-flannel\net-conf.json:FileInfo) [Copy-Item], UnauthorizedAccessException
4 participants