Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace managed object reference with provider ID #668

Merged
merged 2 commits into from Nov 21, 2019

Conversation

@akutz
Copy link
Contributor

akutz commented Nov 20, 2019

What this PR does / why we need it:
This patch removes the storage of the VM's managed object reference as part of a VSphereMachine resource's spec. Instead, the provider ID is used at the beginning of a reconcile request to resolve the VM's managed object reference. The new machine identity workflow is described in a sequence diagram:

machine-controller-reconcile

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 #661

Special notes for your reviewer:
This PR should unblock support for the Cluster API upgrade tool with CAPV. @frapposelli had asked @andrewsykim to look into the upgrade tool and CAPV, and Andrew discovered an issue that prevented support for the tool. This PR should resolve that. For more information, please see #661.

Release note:

* ManagedObjectReference values are no longer stored as part of VSphereMachine resource specs.
@akutz

This comment has been minimized.

Copy link
Contributor Author

akutz commented Nov 20, 2019

/kind feature

@akutz akutz requested review from yastij and sidharthsurana and removed request for figo and sflxn Nov 20, 2019
@akutz akutz changed the title Remove MoRef and use provider ID Replace managed object reference with provider ID Nov 20, 2019
@akutz akutz force-pushed the akutz:feature/rm-moref branch from dbb82fb to 00caa44 Nov 20, 2019
pkg/util/machines.go Outdated Show resolved Hide resolved
Copy link
Member

frapposelli left a comment

just one minor nit, everything else LGTM

pkg/context/session.go Show resolved Hide resolved
pkg/services/govmomi/util.go Outdated Show resolved Hide resolved
@akutz

This comment has been minimized.

Copy link
Contributor Author

akutz commented Nov 21, 2019

Hi @figo and @frapposelli,

I've addressed all of your feedback. Thank you both!

This patch adds support for custom build release types via the script
"hack/release.sh". For example:

        hack/release.sh -t akutz/capv

The above command will build the manager and manifest images as
"akutz/capv-manager:TAG" and "akutz/capv-manifests:TAG".
@akutz akutz force-pushed the akutz:feature/rm-moref branch from 4778aa2 to 0c29644 Nov 21, 2019
This patch removes the storage of the VM's managed object reference as
part of a VSphereMachine resource's spec. Instead, the provider ID is
used at the beginning of a reconcile request to resolve the VM's managed
object reference.
@akutz akutz force-pushed the akutz:feature/rm-moref branch from 0c29644 to 9ca6819 Nov 21, 2019
@akutz

This comment has been minimized.

Copy link
Contributor Author

akutz commented Nov 21, 2019

FWIW, I'm rebasing this PR to squash the commits that were just to address feedback.

@akutz

This comment has been minimized.

Copy link
Contributor Author

akutz commented Nov 21, 2019

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

@frapposelli

This comment has been minimized.

Copy link
Member

frapposelli commented Nov 21, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, frapposelli

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:

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 k8s-ci-robot merged commit a455906 into kubernetes-sigs:master Nov 21, 2019
8 of 9 checks passed
8 of 9 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation akutz authorized
Details
pull-cluster-api-provider-vsphere-e2e Job succeeded.
Details
pull-cluster-api-provider-vsphere-test Job succeeded.
Details
pull-cluster-api-provider-vsphere-verify-crds Job succeeded.
Details
pull-cluster-api-provider-vsphere-verify-fmt Job succeeded.
Details
pull-cluster-api-provider-vsphere-verify-lint Job succeeded.
Details
pull-cluster-api-provider-vsphere-verify-shell Job succeeded.
Details
pull-cluster-api-provider-vsphere-verify-vet Job succeeded.
Details
@akutz akutz added this to the v1alpha3 milestone Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.