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

Add Host process examples #161

Merged
merged 12 commits into from
Oct 21, 2021
Merged

Conversation

jsturtevant
Copy link
Contributor

Reason for PR:

Start providing some examples of how to use hostprocess

Issue Fixed:

Issue #

Requirements

  • Sqaush commits
  • Documentation
  • Tests

Notes:

This is meant to be an example to start testing things out. Not ready for production.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 19, 2021
@github-actions github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 19, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 19, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021
securityContext:
windowsOptions:
hostProcess: true
runAsUserName: "NT AUTHORITY\\system"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use NT AUTHORITY\\NetworkService here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is on my list to try this. local system did not work because calls to HNS apis failed silently

Copy link
Contributor

Choose a reason for hiding this comment

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

The network service doesn't work either, I think there were some folder permissions or something, I can't quite remember but I was hoping to use network service as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give this a try and see if I get an error message. @dcantah @kevpar any thoughts on what specific permissions are needed?

Copy link
Member

Choose a reason for hiding this comment

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

No not off the top of my head, I can ask some hns folks

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 3, 2021
@jsturtevant jsturtevant force-pushed the hostprocess branch 4 times, most recently from d47fa61 to 20c89d1 Compare September 4, 2021 00:59
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 4, 2021
# and CNI network config file on each node.
- name: install-cni
image: sigwindowstools/calico-install:v3.20.0-hostprocess
args: ["$env:CONTAINER_SANDBOX_MOUNT_POINT/calico/install.ps1"]
Copy link

Choose a reason for hiding this comment

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

what interprets this envvar? normally, envvars would be specified in args as $(CONTAINER_SANDBOX_MOUNT_POINT)/calico/install.ps1 (https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#using-environment-variables-inside-of-your-config)

- name: calico-node-startup
image: sigwindowstools/calico-node:v3.20.0-hostprocess
args: ["$env:CONTAINER_SANDBOX_MOUNT_POINT/calico/node-service.ps1"]
workingDir: "$env:CONTAINER_SANDBOX_MOUNT_POINT/calico/"
Copy link

Choose a reason for hiding this comment

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

same question about what component evaluates/expands this envvar... are envvars normally usable inside workingDir?

it looks like the kubelet only expands envvars in command/args, and workingdir is passed as-is to the runtime:

https://github.com/kubernetes/kubernetes/blob/95390e6476fca15b0f55cde2c142ec18ca57aee0/pkg/kubelet/kuberuntime/kuberuntime_container.go#L313-L329

Copy link
Contributor

Choose a reason for hiding this comment

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

These variables are set for processes created inside the container by hcsshim (for both normal and hostProcess containers). Windows will expand and environment variables found in the command line when it tries to runt he process / entry point for the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The runtime sets and evaluates these ENV variables as part of its path processing. We can't use the approach in https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/#using-environment-variables-inside-of-your-config) since it is not known until runtime since that value is set by hcsschim.

Using the vars, technically shouldn't be necessary for hostprocess but we found some bugs in hcsshim around arg and Working directory path processing. With the approach that we are evaluating now this env variable won't be necessary any longer as the container will be given its own filesystem (thought it still has access to the host filesystem unlike on linux).

For reference these are the fixes that are in being worked on until the new approach is validated:
microsoft/hcsshim#1137
microsoft/hcsshim#1117

cc: @dcantah

hostprocess/build.sh Outdated Show resolved Hide resolved
@marosset
Copy link
Contributor

Can we update the 1.22.1 references to 1.22.2?

@marosset
Copy link
Contributor

Can we update the 1.22.1 references to 1.22.2?

Other than that I think this is ready to merge (and we can iterate later if needed)

Co-authored-by: Mark Rossetti <marosset@microsoft.com>
Copy link
Contributor

@marosset marosset left a comment

Choose a reason for hiding this comment

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

/lgtm

I think this is ready to merge. We can iterate if needed.
Thanks for all this work @jsturtevant

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [jsturtevant,marosset]

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 merged commit e73cf36 into kubernetes-sigs:master Oct 21, 2021
@vitaliy-leschenko
Copy link
Contributor

Please make sigwindowstools/flannel-tools:v0.14.0-hostprocess as public. Currently we don't have access to the image.

@jsturtevant
Copy link
Contributor Author

@vitaliy-leschenko It should be
sigwindowstools/flannel:v0.14.0-hostprocess. Will update that typo. Please note these samples and let us know how it works!

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/XXL Denotes a PR that changes 1000+ 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.

7 participants