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

Scripts update to make them usable with hostprocess model #248

Closed
wants to merge 4 commits into from

Conversation

a4099181
Copy link
Contributor

@a4099181 a4099181 commented Oct 26, 2022

Reason for PR:
It seems that Install-Container.ps1 and PrepareNode.ps1 may be useful to build a Windows node with HostProcess containers and k8s 1.25.3 at least. But some changes are needed because some code does not cooperate. This commit is a proposal to leave the current code unchanged for backward compatibility and introduce the UseHostProcess parameter to handle code not needed to support HostProcess containers where are engaged.

Requirements

  • Squash commits

Notes:
There are also changes beyond the main reason. When the UseHostProcess flag is present, the script makes asserts on Kubernetes and ContainerD versions to announce incompatibilities. Another improvement is to not have a choice of container runtime when HostProcess is in use.

Additionally, it offers the SuppressHints parameter to have the option to omit on demand some help messages. Omitted code depends on the git tool installed. It may be an optimization for automated node provisioning (eg. vagrant) and saves some time wasted for git install and repository cloning.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 26, 2022
@github-actions github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 26, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 14, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2022
@marosset
Copy link
Contributor

/assign

@marosset
Copy link
Contributor

The behavior of volume mounts changes depending on if you are using containerd v1.6.x vs v1.7.x (more info at https://github.com/kubernetes/enhancements/tree/master/keps/sig-windows/1981-windows-privileged-container-support#container-mounts)

Can we update all the references to default to https://github.com/containerd/containerd/releases/tag/v1.7.0-beta.1 instead of v1.6.8 (until v1.7.0 releases)?

HelpMessage = "Name of network adapter to use when configuring basic nat network",
ParameterSetName = "kubeadm"
)]
[string] $netAdapterName = "Ethernet",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop the $netAdapterName param since this script no longer creates a nat network (removed with #254 b/c it was causing lots of issues)

Copy link
Contributor Author

@a4099181 a4099181 Dec 14, 2022

Choose a reason for hiding this comment

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

Yes, now it is unused parameter. Should we take care of the backward compatibility? If not, then we can do some more cleanups in both scripts. If so, these scripts will not fit kubeadm folder no longer. Maybe better/cleaner version of the scripts should be made in host-process folder. But there is a couple of PR's which makes changes to them right now. It is not a good time, I think.

Currently, after #254, UseHostProcess parameter is only for containerd version assert purpose. It may not be necessary at all.

I am not convinced to update referenced containerd version in this PR.

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.

Left a couple of comments

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: a4099181
Once this PR has been reviewed and has the lgtm label, please ask for approval from marosset by writing /assign @marosset in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

It seems that Install-Container.ps1 and PrepareNode.ps1 may be useful to
build Windows node with HostProcess containers and k8s 1.25.3 at least.
But some changes are needed, becasue some code does not cooperate.
This commit is a propose to leave current code unchanged for backward compatibility
and introduce UseHostProcess parameter to handle code not needed
to support HostProcess containers where are engaged.

Additionally it offers SuppressHints parameter to have an option
to omit on demand some help messages. Omitted code depends on git tool installed.
It may be an optimization for automated node provisioning (eg. vagrant)
and saves some time wasted for git install and repository cloning.
@jsturtevant
Copy link
Contributor

Thanks for your updates here. We discussed at sig-windows community meeting a couple weeks ago and decided it would be easier to maintain long term just the HostProcess parts in the scripts going forward (#262).

I am going to close this PR since the changes were made in #262. Please open issue/pr if those changes aren't working with Hostprocess. Thanks again for your interest here.

/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

None yet

4 participants