Use deployment for CCM in example yaml #1252

lzhecheng · 2022-03-15T03:35:07Z

Signed-off-by: Zhecheng Li [email protected]

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Use deployment for CCM in example yaml

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

coveralls · 2022-03-15T03:39:27Z

Coverage remained the same at 80.095% when pulling c2a84b2 on lzhecheng:ccm-yaml-in-deployment into 6d49847 on kubernetes-sigs:master.

MartinForReal · 2022-03-15T03:57:27Z

examples/out-of-tree/cloud-controller-manager.yaml

        - name: etc-ssl
-          mountPath: /etc/ssl
-          readOnly: true
+          hostPath:


These files can not be found..

I didn't change this part. Do you mean there's some problem with the original yaml file about these files?

These are ca certs used by apiserver and other component. These files are located on master.

I think toleration has been set for master?
https://github.com/kubernetes-sigs/cloud-provider-azure/pull/1252/files#diff-5f33bdfdadf384ba7fdac1e2fa8c83ad6d497f3c5f49fda1774def3b8c9f4115R153-R155

Let's add a nodeSelector for master

nilo19 · 2022-03-15T04:16:20Z

examples/out-of-tree/cloud-controller-manager.yaml

+            - "--configure-cloud-routes=true" # "false" for Azure CNI and "true" for other network plugins
+            - "--leader-elect=true"
+            - "--route-reconciliation-period=10s"
+            - "--v=2"


can we make v=4?

v=5 by default.

I think 4 is enough for debugging.

Let's use 4 for now and update in the future if needed.

examples/out-of-tree/cloud-controller-manager.yaml

feiskyer · 2022-03-15T05:35:50Z

examples/out-of-tree/cloud-controller-manager.yaml

        - name: etc-kubernetes
-          mountPath: /etc/kubernetes
+          hostPath:
+            path: /etc/kubernetes


could we add a secret with azure.json and mount the secret for authz?

Do you mean we are going to put key-value pairs from azure.json directly into this yaml file of a Secret object? Like:

apiVersion: v1 kind: Secret metadata: name: azure-cloud-config type: Opaque stringData: azure.json: |- tenantId: <tenant-id> subscriptionId: <subscription-id>

Discussed offline. Updated.

feiskyer · 2022-03-15T05:37:14Z

site/content/en/install/azure-ccm.md

@@ -39,6 +39,8 @@ Do not set flag `--cloud-provider`.

 ### azure-cloud-controller-manager

+azure-cloud-controller-manager should be run as Deployment.


run as Deployment with multiple replicas or Kubelet static Pods on each master node.

feiskyer · 2022-03-15T05:47:04Z

could you update https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/tests/k8s-azure/manifest/cluster-api/vmss-multi-nodepool.yaml#L457 to deployment as well?

feiskyer · 2022-03-16T08:54:29Z

tests/k8s-azure/manifest/cluster-api/vmss-multi-nodepool.yaml

+              path: /var/lib/waagent/ManagedIdentity-Settings
+    ---
+    apiVersion: v1
+    kind: Secret


I think capz should already create a secret, so we don't need to add it here? @nilo19 do you know the secret name, or host path should be used instead just for capz?

We don't need a secret here in this template, and host path either. The capz will create a default one and the volume is defined in L533.

Discussed offline. Since this yaml file is for capz, then keep it as it was.

Signed-off-by: Zhecheng Li <[email protected]>

jackfrancis · 2022-03-16T16:33:23Z

examples/out-of-tree/cloud-controller-manager.yaml

-apiVersion: v1
-kind: Pod
+apiVersion: apps/v1
+kind: Deployment
 metadata:
  name: cloud-controller-manager
  namespace: kube-system
  labels:


Do we need to keep the Deployment labels? I think defining pod labels (via spec.template.metadata.labels) should be sufficient.

Yes, it is not a must but I think maybe we can keep it. It does no harm, right?

Correct, keeping the deployment labels are not harmful

lzhecheng · 2022-03-18T07:05:50Z

/retest

feiskyer

/lgtm
/approve

k8s-ci-robot · 2022-03-18T07:17:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, lzhecheng

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~~ [feiskyer,lzhecheng]

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 · 2022-03-18T12:26:13Z

@lzhecheng: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cloud-provider-azure-e2e-ccm-vmss-basic-lb	`c2a84b2`	link	unknown	`/test pull-cloud-provider-azure-e2e-ccm-vmss-basic-lb`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

MartinForReal · 2022-03-18T12:37:41Z

/retest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 15, 2022

k8s-ci-robot requested review from MartinForReal and nilo19 March 15, 2022 03:35

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 15, 2022

MartinForReal reviewed Mar 15, 2022

View reviewed changes

nilo19 reviewed Mar 15, 2022

View reviewed changes

feiskyer reviewed Mar 15, 2022

View reviewed changes

examples/out-of-tree/cloud-controller-manager.yaml Show resolved Hide resolved

feiskyer reviewed Mar 15, 2022

View reviewed changes

lzhecheng force-pushed the ccm-yaml-in-deployment branch 7 times, most recently from fdf3f7a to a1970a7 Compare March 16, 2022 03:19

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 Mar 16, 2022

lzhecheng force-pushed the ccm-yaml-in-deployment branch 2 times, most recently from 5b46837 to 4436e44 Compare March 16, 2022 03:39

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 16, 2022

lzhecheng requested review from feiskyer, MartinForReal and nilo19 March 16, 2022 07:27

feiskyer reviewed Mar 16, 2022

View reviewed changes

Use deployment for CCM in example yaml

c2a84b2

Signed-off-by: Zhecheng Li <[email protected]>

lzhecheng force-pushed the ccm-yaml-in-deployment branch from 4436e44 to c2a84b2 Compare March 16, 2022 13:21

CecileRobertMichon mentioned this pull request Mar 16, 2022

Templates and E2E tests use external cloud-provider-azure by default kubernetes-sigs/cluster-api-provider-azure#1994

Closed

3 tasks

jackfrancis reviewed Mar 16, 2022

View reviewed changes

lzhecheng requested review from feiskyer and jackfrancis March 18, 2022 07:05

feiskyer reviewed Mar 18, 2022

View reviewed changes

k8s-ci-robot assigned feiskyer Mar 18, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2022

k8s-ci-robot merged commit 92ffde3 into kubernetes-sigs:master Mar 18, 2022

lzhecheng deleted the ccm-yaml-in-deployment branch March 18, 2022 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use deployment for CCM in example yaml #1252

Use deployment for CCM in example yaml #1252

lzhecheng commented Mar 15, 2022

coveralls commented Mar 15, 2022 •

edited

Loading

MartinForReal Mar 15, 2022

lzhecheng Mar 15, 2022

MartinForReal Mar 15, 2022

lzhecheng Mar 15, 2022 •

edited

Loading

lzhecheng Mar 15, 2022

nilo19 Mar 15, 2022

MartinForReal Mar 15, 2022

nilo19 Mar 15, 2022

lzhecheng Mar 15, 2022

feiskyer Mar 15, 2022

lzhecheng Mar 15, 2022

lzhecheng Mar 16, 2022

feiskyer Mar 15, 2022

lzhecheng Mar 15, 2022

feiskyer commented Mar 15, 2022

feiskyer Mar 16, 2022

nilo19 Mar 16, 2022

lzhecheng Mar 16, 2022

jackfrancis Mar 16, 2022

lzhecheng Mar 17, 2022

jackfrancis Mar 18, 2022

lzhecheng commented Mar 18, 2022

feiskyer left a comment

k8s-ci-robot commented Mar 18, 2022

k8s-ci-robot commented Mar 18, 2022

MartinForReal commented Mar 18, 2022

		@@ -39,6 +39,8 @@ Do not set flag `--cloud-provider`.

		### azure-cloud-controller-manager

		azure-cloud-controller-manager should be run as Deployment.

Use deployment for CCM in example yaml #1252

Use deployment for CCM in example yaml #1252

Conversation

lzhecheng commented Mar 15, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

coveralls commented Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Mar 18, 2022

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 18, 2022

k8s-ci-robot commented Mar 18, 2022

MartinForReal commented Mar 18, 2022

coveralls commented Mar 15, 2022 •

edited

Loading

lzhecheng Mar 15, 2022 •

edited

Loading