Skip to content

Use hostprocess for Windows node manager in Helm chart#3283

Merged
k8s-ci-robot merged 1 commit into
kubernetes-sigs:masterfrom
CecileRobertMichon:windows-node-hostprocess
Feb 10, 2023
Merged

Use hostprocess for Windows node manager in Helm chart#3283
k8s-ci-robot merged 1 commit into
kubernetes-sigs:masterfrom
CecileRobertMichon:windows-node-hostprocess

Conversation

@CecileRobertMichon
Copy link
Copy Markdown
Contributor

@CecileRobertMichon CecileRobertMichon commented Feb 3, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, external cloud-provider does not work with Windows nodes when using the Helm chart. The Daemonset for cloud-node-manager-windows is not able to reach the metadata endpoint and therefore windows nodes always have the Taint node.cloudprovider.kubernetes.io/uninitialized=true:NoSchedule.

This PR changes the cloud-node-manager-windows Daemonset to use hostProcess: true and adds a temporary workaround for not being able to use inClusterConfig() (until k8s 1.26 + containerd v1.7).

All credits go to @mweibel and @marosset for discovering the issue and suggesting a fix.

Which issue(s) this PR fixes:

Fixes kubernetes-sigs/cluster-api-provider-azure#2591

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 3, 2023
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 3, 2023

Deploy Preview for kubernetes-sigs-cloud-provide-azure ready!

Name Link
🔨 Latest commit 4a33259
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/63e5a398d9e87c00088556da
😎 Deploy Preview https://deploy-preview-3283--kubernetes-sigs-cloud-provide-azure.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 3, 2023
Comment thread helm/cloud-provider-azure/templates/cloud-provider-azure.yaml
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2023
@CecileRobertMichon CecileRobertMichon changed the title [WIP] Use hostprocess for Windows node manager in Helm chart Use hostprocess for Windows node manager in Helm chart Feb 8, 2023
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 8, 2023
@CecileRobertMichon
Copy link
Copy Markdown
Contributor Author

/kind bugfix

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

@CecileRobertMichon: The label(s) kind/bugfix cannot be applied, because the repository doesn't have them.

Details

In response to this:

/kind bugfix

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.

@CecileRobertMichon
Copy link
Copy Markdown
Contributor Author

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind labels Feb 8, 2023
@CecileRobertMichon
Copy link
Copy Markdown
Contributor Author

/assign @marosset

@feiskyer
Copy link
Copy Markdown
Member

feiskyer commented Feb 9, 2023

the Daemonset for cloud-node-manager-windows is not able to reach the metadata endpoint

Could you elaborate why IMDS is not reachable on capz?

Comment thread helm/cloud-provider-azure/templates/cloud-provider-azure.yaml
@CecileRobertMichon
Copy link
Copy Markdown
Contributor Author

Could you elaborate why IMDS is not reachable on capz?

@marosset can weigh in but I believe it's not a capz limitation, it's a calico overlay networking issue: kubernetes-sigs/cluster-api-provider-azure#2132

Azure CSI driver helm charts also had to do this for this reason: kubernetes-sigs/azuredisk-csi-driver#1201

Copy link
Copy Markdown
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

This should unblock deploying the Windows cloud-node-manager!
We can update the deployment to no longer need the powershell / kube-config after containerd 1.7 releases (but the current changes should work on both containerd 1.6 and 1.7)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023
@marosset
Copy link
Copy Markdown
Contributor

marosset commented Feb 9, 2023

Could you elaborate why IMDS is not reachable on capz?

@marosset can weigh in but I believe it's not a capz limitation, it's a calico overlay networking issue: kubernetes-sigs/cluster-api-provider-azure#2132

I believe it is a limitation on overlay networking on Windows in general (not specific to calico)

@CecileRobertMichon
Copy link
Copy Markdown
Contributor Author

/assign @feiskyer

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023
Copy link
Copy Markdown
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2023
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, feiskyer, marosset

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

The pull request process is described here

Details 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

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windows cluster with cloud-provider: external does not work

4 participants