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 HostProcess support for Calico for Windows #5864

Merged
merged 56 commits into from Apr 25, 2022

Conversation

lmm
Copy link
Contributor

@lmm lmm commented Apr 8, 2022

Description

This PR adds support for installing Calico for Windows using HostProcess containers.

Preview: https://deploy-preview-5864--calico-master.netlify.app/getting-started/windows-calico/quickstart

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Add tech-preview support for Calico for Windows using HostProcess containers

@lmm lmm requested a review from a team as a code owner April 8, 2022 00:14
@marvin-tigera marvin-tigera added this to the Calico v3.24.0 milestone Apr 8, 2022
@marvin-tigera marvin-tigera added docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small) labels Apr 8, 2022
@lmm lmm requested a review from song-jiang April 8, 2022 00:15
node/calico-windows.yaml Outdated Show resolved Hide resolved
node/calico-windows.yaml Outdated Show resolved Hide resolved
Copy link
Member

@song-jiang song-jiang left a comment

Choose a reason for hiding this comment

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

Added some comments

calico/scripts/install-calico-windows.ps1 Outdated Show resolved Hide resolved
SetConfigParameters -OldString '<your datastore type>' -NewString $Datastore
SetConfigParameters -OldString '<your etcd endpoints>' -NewString "$EtcdEndpoints"
Set-ConfigParameters -var 'CALICO_DATASTORE_TYPE' -value $Datastore
Set-ConfigParameters -var 'ETCD_ENDPOINTS' -value $EtcdEndpoints
Copy link
Member

Choose a reason for hiding this comment

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

Using a proper lib function is a nice improvement.

calico/scripts/install-calico-windows.ps1 Outdated Show resolved Hide resolved

Write-Host "Done, the Calico services are running:"
Get-Service | where Name -Like 'Calico*'
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, maybe there is a better way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

- name: install
image: laurenceman/calico:latest
args:
- "$env:CONTAINER_SANDBOX_MOUNT_POINT\\install-calico-windows.ps1"
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above, I'm thinking we could construct a new script and keep install-calico-windows.ps1 with limited changes.

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 added a entrypoint script to attempt uninstalling Calico for Windows, then starts the installation.

exec:
command:
- c:\\CalicoWindows\\calico-node.exe
- -felix-live
Copy link
Member

Choose a reason for hiding this comment

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

Have we tested the command on Windows? Is it working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is working

@@ -13,7 +13,8 @@
# limitations under the License.

# This script is run from the main Calico folder.
. .\config.ps1
# TODO: env vars will be set in the daemonset template spec.
#. .\config.ps1
Copy link
Member

Choose a reason for hiding this comment

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

Will it break manual installation after config.ps1 being removed?

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've reverted this

@@ -19,7 +19,7 @@ Param(

$baseDir = "$PSScriptRoot\.."
. $baseDir\config.ps1
ipmo $baseDir\libs\calico\calico.psm1
ipmo $baseDir\libs\calico\calico.psm1 -Force
Copy link
Member

Choose a reason for hiding this comment

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

What is impact of --Force ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use -force elsewhere when importing the calico module. If the module is already loaded, force will load it again. When we uninstall calico we do not currently remove the module but that's fixed with 6e73547

@marvin-tigera marvin-tigera added docs-pr-required Change is not yet documented release-note-required Change has user-facing impact (no matter how small) labels Apr 19, 2022
@lmm lmm force-pushed the lmm-win-hostprocess branch 2 times, most recently from a58019a to 8176616 Compare April 19, 2022 20:29
Copy link
Member

@song-jiang song-jiang left a comment

Choose a reason for hiding this comment

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

Looking good. A few comments. I still need some time to review install kubelet part.

calico/getting-started/windows-calico/quickstart.md Outdated Show resolved Hide resolved
calico/_includes/content/calico-windows-install.md Outdated Show resolved Hide resolved
lmm added 3 commits April 21, 2022 14:34
This sets the logging parameters for the kubelet service to be like
what we currently use in our manual installation method.
curl {{ "/manifests/windows-kube-proxy.yaml" | absolute_url }} -o windows-kube-proxy.yaml
```
- Edit the downloaded manifest
- Replace `VERSION` with your Windows nodes' server version.
Copy link
Member

Choose a reason for hiding this comment

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

e.g. 1809.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and a few other small issues

@song-jiang
Copy link
Member

Just a minor comment but LGTM.

@lmm lmm added docs-completed Docs PR has been merged for this change and removed docs-pr-required Change is not yet documented labels Apr 25, 2022
@lmm lmm merged commit 35b0c49 into projectcalico:master Apr 25, 2022
lmm added a commit to lmm/calico that referenced this pull request Apr 25, 2022
Add tech-preview of Calico for Windows installed using hostprocess containers
@lmm lmm deleted the lmm-win-hostprocess branch April 25, 2022 20:06
lmm added a commit that referenced this pull request Apr 26, 2022
Add tech-preview of Calico for Windows installed using hostprocess containers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-completed Docs PR has been merged for this change release-note-required Change has user-facing impact (no matter how small)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants