Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: External cloud provider support for Azure Stack Cloud #4635

Merged
merged 8 commits into from Sep 3, 2021

Conversation

haofan-ms
Copy link
Contributor

Reason for Change:

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

Notes:

parts/k8s/kuberneteswindowssetup.ps1 Show resolved Hide resolved
mountPath: /var/lib/kubelet
readOnly: true
volumes:
- name: etc-kubernetes
Copy link
Member

Choose a reason for hiding this comment

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

when possible, I would just mount the file instead of the entire directory:

volumes:
- name: etc-kubernetes
  hostPath:
    path: /etc/kubernetes/azurestackcloud.json
    type: FileOrCreate
volumeMounts:
- name: custom-environment
  mountPath: /etc/kubernetes/azurestackcloud.json
  readOnly: true

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 /etc/kubernetes directory also contains azure.json and cert, so I leave this one to folder level access. I updated the /var/lib/kubelet/kubeconfig to file level access.

parts/k8s/kuberneteswindowssetup.ps1 Show resolved Hide resolved
parts/k8s/kuberneteswindowssetup.ps1 Outdated Show resolved Hide resolved
@@ -45,8 +45,8 @@ const (
csiSnapshotControllerImageReference string = "oss/kubernetes-csi/snapshot-controller:v2.0.0"
csiAzureDiskImageReference string = "k8s/csi/azuredisk-csi:v0.7.0"
csiAzureFileImageReference string = "k8s/csi/azurefile-csi:v0.6.0"
azureCloudControllerManagerImageReference string = "oss/kubernetes/azure-cloud-controller-manager:v0.5.1"
azureCloudNodeManagerImageReference string = "oss/kubernetes/azure-cloud-node-manager:v0.5.1"
azureCloudControllerManagerImageReference string = "oss/kubernetes/azure-cloud-controller-manager:v1.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

is it OK to update this for all clouds?

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 change works for Azure and Azure Stack. For other cloud, I think they can continue to use with "useCloudControllerManager" flag set to false, or adopt similar change we did if they want to use external cloud provider as well. Does this make sense?

jadarsie
jadarsie previously approved these changes Sep 2, 2021
@@ -88,18 +88,52 @@ spec:
command:
- cloud-node-manager
- --node-name=$(NODE_NAME)
{{- if IsAzureStackCloud}}
- --use-instance-metadata=false
- --cloud-config=/etc/kubernetes/azure.json
Copy link
Member

Choose a reason for hiding this comment

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

do you know why only ASH needs cloud-config and kubeconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For kubeconfig, if not provided the ecp (external cloud provider) will use default value from environment variable of Kubernetes Service (https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/vendor/k8s.io/client-go/rest/config.go#L488), which doesn't work for Azure Stack as well as Azure Windows node.

For cloud-config, we need that to correct get cloud and tenant information.

Copy link
Member

@jadarsie jadarsie Sep 3, 2021

Choose a reason for hiding this comment

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

We should figure out which KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT values work on ASH and Windows. The same for cloud and tenant info. The least access to the host file system, the better.

parts/k8s/kuberneteswindowssetup.ps1 Show resolved Hide resolved
parts/k8s/kuberneteswindowssetup.ps1 Outdated Show resolved Hide resolved
@@ -148,16 +182,40 @@ spec:
command:
- /cloud-node-manager.exe
- --node-name=$(NODE_NAME)
- --kubeconfig=C:\k\config
Copy link
Member

Choose a reason for hiding this comment

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

Did we want this new --kubeconfig configuration to be outside of the Azure Stack block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my test on Azure, the windows node will also need this config to launch the cloud node manager pod. The default IP in external-cloud-provider is not 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.

Sample error:
E0902 16:06:16.057842 1344 nodemanager.go:183] Error monitoring node status: Get "https://10.0.0.1:443/api/v1/nodes?fieldSelector=metadata.name%3D2356k8s010&resourceVersion=0": dial tcp 10.0.0.1:443: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

- name: AZURE_ENVIRONMENT_FILEPATH
value: /etc/kubernetes/azurestackcloud.json
- name: AZURE_GO_SDK_LOG_LEVEL
value: DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep this DEBUG verbosity for customers?

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 will update, thanks for catching this

jackfrancis
jackfrancis previously approved these changes Sep 2, 2021
Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@jackfrancis
Copy link
Member

@haofan-ms make generate and then commit generated code to pass UT

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@jackfrancis jackfrancis merged commit d0da828 into Azure:master Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants