Initial implementation for YAML based config #305

davidvonthenen · 2020-03-18T05:15:39Z

What this PR does / why we need it:
Implements a new YAML-based cloud-config which will enable a bunch of new features going forward:

more expressive configuration which YAML provides
is inline with the YAML-based configurations used all over in k8s
allows the possibility for versioning in the cloud-config when YAML-based
fixes a bunch of YAML and INI naming consistency issues in the config file as they relate to the new LoadBalancer feature
the possibilities are endless 😀

Which issue this PR fixes: fixes #275

Special notes for your reviewer:

Tested on a vSphere 6.7u3 configuration with 2 VCs and 3 DCs using zone support.
Verified my existing INI-based cloud-config worked with the code changes for backward compatibility
Created and verified the new YAML-based cloud-config works
Adds a bunch of YAML based go test to verify functionality

Release note:
Since this PR preserves backward compatibility of the INI-based cloud-config (and has been tested), this should have little impact (as of right now) to the end-user.

davidvonthenen · 2020-03-18T19:38:25Z

CC: @andrewsykim @frapposelli

The lint test/check is failing because of the struct names ConfigINI and ConfigYAML, but I would like to keep these names as-is for now because this is only temporary since this also has a future deprecation notice for the INI-based configuration. If it's an issue, I can change the class names to just INI and YAML as recommends.

pkg/common/config/types_ini_legacy.go:119:6: type name will be used as config.ConfigINI by other packages, and that stutters; consider calling this INI
pkg/common/config/types_yaml.go:123:6: type name will be used as config.ConfigYAML by other packages, and that stutters; consider calling this YAML

The integration test is failing for some unknown reason. I think it might be some issue with the timing when cluster coming up in the test or perhaps the verification that it's deployed correctly. See below:

make[2]: Entering directory '/home/prow/go/src/k8s.io/cloud-provider-vsphere/test/integration'
while ! kubectl -n kube-system logs vsphere-cloud-controller-manager >/dev/null 2>&1; do sleep 1; done && \
sleep 3 && \
kubectl -n kube-system logs vsphere-cloud-controller-manager
Error from server: Get https://ccm-integration-test-worker:10250/containerLogs/kube-system/vsphere-cloud-controller-manager/vsphere-cloud-controller-manager: dial tcp: lookup ccm-integration-test-worker on 10.63.240.10:53: no such host

I looking into the integration test failure right now...

docs/book/tutorials/deploying_cpi_and_csi_with_multi_dc_vc_aka_zones.md

pkg/cloudprovider/vsphere/config/config_ini_legacy.go

pkg/cloudprovider/vsphere/config/config_yaml.go

pkg/cloudprovider/vsphere/config/config_yaml_test.go

pkg/cloudprovider/vsphere/loadbalancer/config/config_ini_legacy_test.go

pkg/cloudprovider/vsphere/loadbalancer/config/types_yaml.go

davidvonthenen · 2020-06-15T21:14:55Z

@andrewsykim we just have the one item/flag left... I can change it either way. If we do change it back, it would be preferred if the newer YAML at least matches.

davidvonthenen · 2020-07-01T22:12:40Z

/retest

davidvonthenen · 2020-07-01T23:12:53Z

I took another pass at reviewing this PR and I think it looks good... and all current feedback has been addressed.

andrewsykim

Is the virtualcenter -> vcenter technically a breaking change? I agree on the name change but both field name should be accepted I think?

docs/book/tutorials/deploying_cpi_and_csi_with_multi_dc_vc_aka_zones.md

pkg/cloudprovider/vsphere/cloud.go

pkg/cloudprovider/vsphere/config/config_yaml.go

pkg/cloudprovider/vsphere/loadbalancer/config/config_ini_legacy_test.go

pkg/cloudprovider/vsphere/loadbalancer/config/types_yaml.go

pkg/common/config/config_ini_legacy.go

pkg/common/config/config_yaml.go

davidvonthenen · 2020-07-10T01:06:56Z

There are some changes (port from string to unit) that I want to retest with before giving my "thumbs up" again for additional review and/or merge.

davidvonthenen · 2020-07-10T03:31:12Z

Tested on a 1.17.4 config with my normal config of 2 VCs with 3DCs cut into 3 zones... looks good to me! Good for another pass @andrewsykim

andrewsykim

LGTM, minor comments about using camel case in documentation

docs/book/tutorials/deploying_cpi_and_csi_with_multi_dc_vc_aka_zones.md

pkg/cloudprovider/vsphere/config/config_yaml_test.go

Fix lint and format Fixing go tests Update go tests Add more go tests Change package name Fixed docs Fix go test virtualcenter -> vcenter in docs Fixed YAML camel case Update for lint Fixes based on feedback Fix config YAML

davidvonthenen · 2020-07-21T18:22:36Z

Updated based on latest feedback from @andrewsykim

andrewsykim · 2020-07-21T18:40:48Z

manifests/controller-manager/vsphere-cloud-controller-manager-ds.yaml

@@ -26,7 +30,7 @@ spec:
      nodeSelector:
        node-role.kubernetes.io/master: ""
      securityContext:
-        runAsUser: 0


Are the changes in this file intentional?

yes, it is. I forgot to pull down the YAML I used to test with. There were discrepancies between the vsphere-cloud-controller-manager-ds.yaml and vsphere-cloud-controller-manager-pod.yaml. This resolves it... the ds.yaml should be running as user 1001 which is a non-root account.

Should have been changed for this PR:
#297

andrewsykim

/approve
/lgtm

k8s-ci-robot · 2020-07-21T20:25:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim

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

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 18, 2020

k8s-ci-robot requested review from imkin and SandeepPissay March 18, 2020 05:16

davidvonthenen force-pushed the feature/yaml2 branch from 0eaf212 to 34854f4 Compare March 18, 2020 16:32

davidvonthenen assigned andrewsykim and frapposelli Mar 19, 2020

andrewsykim reviewed Mar 23, 2020

View reviewed changes

davidvonthenen mentioned this pull request Apr 10, 2020

Typo fix for vcpctl provision long description #314

Merged

davidvonthenen force-pushed the feature/yaml2 branch from 34854f4 to f070ca0 Compare June 8, 2020 21:20

frapposelli added this to the v1.2.0 milestone Jul 1, 2020

davidvonthenen force-pushed the feature/yaml2 branch from 3d9bbe5 to 5654e42 Compare July 1, 2020 21:13

davidvonthenen force-pushed the feature/yaml2 branch from b3a8579 to 0ea6e5f Compare July 1, 2020 22:22

andrewsykim reviewed Jul 9, 2020

View reviewed changes

davidvonthenen force-pushed the feature/yaml2 branch from fbcbf0f to d149dac Compare July 10, 2020 01:03

davidvonthenen force-pushed the feature/yaml2 branch 2 times, most recently from 65a2b29 to 5d3cb31 Compare July 10, 2020 01:30

andrewsykim reviewed Jul 21, 2020

View reviewed changes

docs/book/tutorials/deploying_cpi_and_csi_with_multi_dc_vc_aka_zones.md Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/config/config_yaml_test.go Outdated Show resolved Hide resolved

pkg/cloudprovider/vsphere/config/config_yaml_test.go Outdated Show resolved Hide resolved

Initial implementation for YAML based config

ab57b53

Fix lint and format Fixing go tests Update go tests Add more go tests Change package name Fixed docs Fix go test virtualcenter -> vcenter in docs Fixed YAML camel case Update for lint Fixes based on feedback Fix config YAML

davidvonthenen force-pushed the feature/yaml2 branch from 4372a8f to ab57b53 Compare July 21, 2020 18:18

andrewsykim reviewed Jul 21, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2020

k8s-ci-robot merged commit 3a383cd into kubernetes:master Jul 21, 2020

davidvonthenen deleted the feature/yaml2 branch July 22, 2020 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation for YAML based config #305

Initial implementation for YAML based config #305

davidvonthenen commented Mar 18, 2020 •

edited

Loading

davidvonthenen commented Mar 18, 2020 •

edited

Loading

davidvonthenen commented Jun 15, 2020

davidvonthenen commented Jul 1, 2020

davidvonthenen commented Jul 1, 2020

andrewsykim left a comment

davidvonthenen commented Jul 10, 2020

davidvonthenen commented Jul 10, 2020 •

edited

Loading

andrewsykim left a comment

davidvonthenen commented Jul 21, 2020

andrewsykim Jul 21, 2020

davidvonthenen Jul 21, 2020

davidvonthenen Jul 21, 2020

andrewsykim left a comment

k8s-ci-robot commented Jul 21, 2020

Initial implementation for YAML based config #305

Initial implementation for YAML based config #305

Conversation

davidvonthenen commented Mar 18, 2020 • edited Loading

davidvonthenen commented Mar 18, 2020 • edited Loading

davidvonthenen commented Jun 15, 2020

davidvonthenen commented Jul 1, 2020

davidvonthenen commented Jul 1, 2020

andrewsykim left a comment

Choose a reason for hiding this comment

davidvonthenen commented Jul 10, 2020

davidvonthenen commented Jul 10, 2020 • edited Loading

andrewsykim left a comment

Choose a reason for hiding this comment

davidvonthenen commented Jul 21, 2020

andrewsykim Jul 21, 2020

Choose a reason for hiding this comment

davidvonthenen Jul 21, 2020

Choose a reason for hiding this comment

davidvonthenen Jul 21, 2020

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 21, 2020

davidvonthenen commented Mar 18, 2020 •

edited

Loading

davidvonthenen commented Mar 18, 2020 •

edited

Loading

davidvonthenen commented Jul 10, 2020 •

edited

Loading