Centralized cloud provider configuration #471

akutz · 2019-07-29T17:52:34Z

What this PR does / why we need it:
Introduces support for a centralized cloud provider configuration

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60

Special notes for your reviewer:
Please see #314 (comment) for more information.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

* Support for a centralized cloud provider configuration as part of the cluster spec

k8s-ci-robot · 2019-07-29T17:52:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz

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~~ [akutz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This patch introduces support for a dedicated cloud provider configuration.

This patch removes the field tags for the "ini" package as they add confusion for people who haven't read why the "ini" package is not being used to marshal data.

This patch switches from using a Go template to marshal a cloud provider config to using reflection. This ensures the section and property names match those defined by the "gcfg" field tags.

This patch adds support for the "omitempty" tag to gcfg fields.

This patch updates the YAML examples, generation scripts, and documentation to reflect the new cloud provider configuration.

This patch fixes an issue with the way the cloud provider configuration's workspace datastore was being processed.

This patch moves the machine config properties for datastore, resource pool, and folder into the cluster's cloud provider configuration. CAPV cannot deploy clusters that do not use the vSphere cloud provider anyway due to the way ready state is determined by the node ref controller.

akutz · 2019-08-07T22:43:28Z

/test pull-cluster-api-provider-vsphere-e2e

akutz · 2019-08-08T17:23:21Z

Hi @frapposelli / @figo / @sidharthsurana / @andrewsykim,

Please lgtm this if there are no objections. FWIW, this includes the fix for the CP credentials.

figo · 2019-08-08T17:57:34Z

@akutz do we need to make sure the resource management part of cloud provider configure is consistent with VM's resource management configure? (same resource pool etc)

cc @andrewsykim

akutz · 2019-08-08T17:58:54Z

Hi @figo,

@akutz do we need to make sure the resource management part of cloud provider configure is consistent with VM's resource management configure? (same resource pool etc)

cc @andrewsykim

I spoke with Andrew, and the decision for the time being was to ensure that the VMs are located in the same workspace used by the cloud provider. Which is already implemented in this PR. The only location attribute available for machines is now the datacenter. Otherwise the location is specified only in the cloud provider configuration.

figo · 2019-08-08T18:04:24Z

cmd/clusterctl/examples/vsphere/machineset.yaml.template

        value:
          apiVersion: vsphere.cluster.k8s.io/v1alpha1
          kind: VsphereMachineProviderSpec
          datacenter: "${VSPHERE_DATACENTER}"


@akutz it make sense to me now, thx for the explanation, could you help to clarify why datacenter kept in both machine spec and cloud provider configure?

Hi @figo,

Because you can configure multiple datacenter via multiple vCenter definitions in the cloud provider configuration. So technically you could have a K8s cluster with each of its control plane nodes in different datacenters as long as each datacenter had symmetrical pool/datastore/folder structures.

figo · 2019-08-08T18:12:02Z

config/crds/vsphere_v1alpha1_vspheremachineproviderspec.yaml

            devices:
              description: Devices is the list of network devices used by the virtual
-                machine.
+                machine. TODO(akutz) Make sure at least one network matches the             ClusterSpec.CloudProviderConfiguration.Network.Name


remove the extra space?

That can be addressed in a future PR/patch. Thanks!

figo · 2019-08-08T18:24:06Z

pkg/apis/vsphere/v1alpha1/cloud/types.go

+package cloud
+
+// Config is the vSphere cloud provider's configuration.
+type Config struct {


this probably already been discussed: what is the plan to keep in-sync with cloud provider types?
is vendor-in types from cloud provider repo in the long term plan? thanks

That's correct. On both counts. Eventually these types will move into the CCM codebase.

Can you open an issue in the CCM repo for this?

figo · 2019-08-08T18:32:12Z

/lgtm
/hold
hold to give others chance to review, but feel free to cancel the hold

frapposelli · 2019-08-08T21:24:21Z

/lgtm
/hold cancel

andrewsykim · 2019-08-09T23:03:46Z

Sorry I'm a bit late to the party, but this LGTM!

…to-maintainer Add lubronzhan to the maintainer list

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 29, 2019

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2019

k8s-ci-robot requested review from andrewsykim and sflxn July 29, 2019 17:52

akutz mentioned this pull request Jul 29, 2019

Aligning CAPV with vSphere Cloud Provider #314

Closed

akutz force-pushed the feature/cloud-provider-config branch from 5a40e4e to cba12c2 Compare July 29, 2019 19:21

akutz added this to the 0.4.1 milestone Jul 29, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2019

akutz mentioned this pull request Aug 7, 2019

[WIP] add support support for VMC on AWS #491

Closed

2 tasks

akutz added 7 commits August 7, 2019 17:23

Support for dedicated cloud provider config

8088602

This patch introduces support for a dedicated cloud provider configuration.

Remove field tags for "ini" package

6c61c0e

This patch removes the field tags for the "ini" package as they add confusion for people who haven't read why the "ini" package is not being used to marshal data.

Use reflection to marshal cloud provider config

6fea97f

This patch switches from using a Go template to marshal a cloud provider config to using reflection. This ensures the section and property names match those defined by the "gcfg" field tags.

Implement gcfg omitempty

526b477

This patch adds support for the "omitempty" tag to gcfg fields.

Update YAML tpls, docs for new cloud provider conf

f6106cf

This patch updates the YAML examples, generation scripts, and documentation to reflect the new cloud provider configuration.

Fix default-datastore bug in cloud conf

a4c97f8

This patch fixes an issue with the way the cloud provider configuration's workspace datastore was being processed.

akutz force-pushed the feature/cloud-provider-config branch from cba12c2 to 62fa84c Compare August 7, 2019 22:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2019

figo reviewed Aug 8, 2019

View reviewed changes

k8s-ci-robot assigned figo Aug 8, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 8, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2019

k8s-ci-robot assigned frapposelli Aug 8, 2019

k8s-ci-robot merged commit 64c4c4d into kubernetes-sigs:master Aug 8, 2019

akutz deleted the feature/cloud-provider-config branch August 9, 2019 14:30

yastij mentioned this pull request Aug 12, 2019

update manifest generation to match centralized cloud provider configuration #497

Closed

akutz modified the milestones: 0.4.1, 0.4.2 Aug 16, 2019

silvery1622 pushed a commit to silvery1622/cluster-api-provider-vsphere that referenced this pull request Jun 20, 2025

Merge pull request kubernetes-sigs#471 from nicolehanjing/add-lubron-…

363e8ef

…to-maintainer Add lubronzhan to the maintainer list

Centralized cloud provider configuration #471

Centralized cloud provider configuration #471

Uh oh!

Conversation

akutz commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 29, 2019

Uh oh!

akutz commented Aug 7, 2019

Uh oh!

akutz commented Aug 8, 2019

Uh oh!

figo commented Aug 8, 2019

Uh oh!

akutz commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

figo Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

akutz Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

figo Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

akutz Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

figo Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

akutz Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

frapposelli Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

figo commented Aug 8, 2019

Uh oh!

frapposelli commented Aug 8, 2019

Uh oh!

andrewsykim commented Aug 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akutz commented Jul 29, 2019 •

edited

Loading

akutz commented Aug 8, 2019 •

edited

Loading