search

kubernetes-sigs / vsphere-csi-driver

vSphere storage Container Storage Interface (CSI) plugin
https://docs.vmware.com/en/VMware-vSphere-Container-Storage-Plug-in/index.html
Apache License 2.0
295 stars 181 forks source link

Bump govmomi to support using MOID and moref on specs #3097

Closed rikatz closed 2 weeks ago

rikatz commented 3 weeks ago

What this PR does / why we need it: This change allows setting a datacenter MOID instead of inventory path on configuration

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

Testing done: With a CAPV cluster, and modified versions of CPI and CSI, testing workflows like:

Special notes for your reviewer:

Release note:

Support using MOID on Datacenter field
divyenpatel commented 3 weeks ago

/ok-to-test

divyenpatel commented 3 weeks ago

I see we are populating datacenter name in the config secret on supervisor cluster.

datacenters = "datacenter-3"

Can you ensure we do not cause regression in the wcp supervisor due to this change?

rikatz commented 3 weeks ago

I see we are populating datacenter name in the config secret on supervisor cluster.

datacenters = "datacenter-3"

Can you ensure we do not cause regression in the wcp supervisor due to this change?

Are we populating the datacenter this way on supervisor? I don't have an environment right now to check it, may ask some borrowed :)

rikatz commented 2 weeks ago

/hold

I want to wait for https://github.com/vmware/govmomi/pull/3609 to be merged and a new release made, so instead we can bump govmomi here and rely on the new behavior of being able to search by name or moid directly from the library

divyenpatel commented 2 weeks ago

@rikatz can we have dataceter name in the format for moref? if yes, we need to handle that case as well.

rikatz commented 2 weeks ago

@rikatz can we have dataceter name in the format for moref? if yes, we need to handle that case as well.

yes, new govmomi version on main supports moref now as well. I am waiting them to cut a release to change here, and instead of doing my changes, just a govmomi library bump should be enough

rikatz commented 2 weeks ago

/hold cancel

rikatz commented 2 weeks ago

@divyenpatel I have tested the change here on my environment, passing DC as moid and works fine.

The only required change at this point was to bump govmomi, which I think is safe :)

k8s-ci-robot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, rikatz

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](https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/OWNERS)~~ [divyenpatel] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment