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.

Sign up for GitHub

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

Jump to bottom

✨ Allow using moid #3229

Merged
merged 4 commits into from
Nov 12, 2024
Merged

✨ Allow using moid #3229

merged 4 commits into from
Nov 12, 2024

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Oct 21, 2024

What this PR does / why we need it: Allow users passing MOID as part of the spec

In some cases, passing the name of an object may not be enough. There may be disambiguation problems on name usage, or special characters that are accepted by vSphere but not properly encoded to be used during VM provisioning.

This change allows users that can rely on passing MOID (eg.: automation, terraform, vSphere UI) to pass the MOID of an object instead of passing the canonical name

Note: This change doesn't cover yet Network Name and the Failure Domain fields, that can be either covered by this same change, or on a follow up PR.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2024
@rikatz rikatz force-pushed the allow-using-moid-main branch from 8eea79f to bd176b0 Compare October 21, 2024 19:57
@chrischdi
Copy link
Member

/test help

@k8s-ci-robot
Copy link
Contributor

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
  • /test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main
  • /test pull-cluster-api-provider-vsphere-test-main
  • /test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-vsphere-apidiff-main
  • pull-cluster-api-provider-vsphere-test-main
  • pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

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-sigs/prow repository.

@chrischdi
Copy link
Member

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@chrischdi chrischdi added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 22, 2024
@chrischdi chrischdi mentioned this pull request Oct 22, 2024
Closed
neolit123
neolit123 reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases, passing the name of an object may not be enough. There may be disambiguation problems on name usage, or special characters that are accepted by vSphere but not properly encoded to be used during VM provisioning.

This change allows users that can rely on passing MOID (eg.: automation, terraform, vSphere UI) to pass the MOID of an object instead of passing the canonical name

could you provide an example of users passing a name that doesn't work and how they would pass MOID instead?

should API field docs be updated to reflect that?

Note: This change doesn't cover yet Network Name and the Failure Domain fields, that can be either covered by this same change, or on a follow up PR.

best to include it in the same PR, IMO.

pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
pkg/services/govmomi/create_test.go Show resolved Hide resolved
pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
pkg/services/govmomi/create_test.go Show resolved Hide resolved
pkg/services/govmomi/vcenter/clone.go Outdated Show resolved Hide resolved
pkg/services/govmomi/vcenter/clone.go Outdated Show resolved Hide resolved
pkg/services/govmomi/vcenter/clone.go Outdated Show resolved Hide resolved
pkg/session/session.go Outdated Show resolved Hide resolved
pkg/session/session.go Outdated Show resolved Hide resolved
@neolit123
Copy link
Member

/cc @lubronzhan

@chrischdi
Copy link
Member

/retest

@rikatz rikatz changed the title ✨ Allow using moid main ✨ Allow using moid Oct 22, 2024
@chrischdi
Copy link
Member

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@rikatz
Copy link
Contributor Author

rikatz commented Oct 22, 2024

I will add the api docs as well, didn't finished taking care of all the comments yet :)

@fabriziopandini
Copy link
Member

fabriziopandini commented Oct 23, 2024 •
edited
Loading

I will try to get to this PR soon, but currently trying to finish some work in CAPI first; in the meantime it will be great if we can add an E2E test to this PR.

e.g. we can take this E2E test, duplicate it, write code that resolve names in env vars into Moid, then sets env vars with the Moid so the test will use them

@rikatz
Copy link
Contributor Author

rikatz commented Oct 23, 2024

sorry folks, last 2 days were really busy. I will come back to this tomorrow as my first task :)

@rikatz
Copy link
Contributor Author

rikatz commented Oct 24, 2024

#3235

will rebase over this one for the logging comments.

Then add some e2e tests as proposed by Fabrizio

@rikatz
Copy link
Contributor Author

rikatz commented Oct 28, 2024

@fabriziopandini just an update. Setting the e2e here is showing to be harder than I though, because in "theory" my e2e suite cannot access vcsim, which is running as a Pod on my cluster.

I am not sure we are expecting this to be possible or not, but in this case I will need to, instead of making my e2e test go to vcsim and get the MOIDs, make the vcsim envvar controller to add on its status the moid instead of the name.

wdyt? Which one should I follow?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024
@rikatz rikatz force-pushed the allow-using-moid-main branch from 30d839e to 3005d40 Compare October 28, 2024 22:50
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024
@rikatz
Copy link
Contributor Author

rikatz commented Oct 29, 2024 •
edited
Loading

hum, weird this error, it passes locally. Will have to check the issue

ra, running the file test errors, maybe I am using some global that is nullified after it, should check tomorrow

@rikatz
Copy link
Contributor Author

rikatz commented Oct 29, 2024

ok was able to fix e2e 🥳

Now just need to check the issue on unit test and we are good to go

@k8s-ci-robot 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 Oct 29, 2024
@rikatz rikatz force-pushed the allow-using-moid-main branch from e2a4173 to 2f0e18d Compare October 29, 2024 19:01
@rikatz
Copy link
Contributor Author

rikatz commented Nov 7, 2024

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2024
@rikatz
Copy link
Contributor Author

rikatz commented Nov 7, 2024

This is validated on my tests, works fine :)

@sbueringer @fabriziopandini @chrischdi for final review :)

Thanks!

neolit123
neolit123 reviewed Nov 7, 2024
View reviewed changes
pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
pkg/services/govmomi/create_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_setup_test.go Outdated Show resolved Hide resolved
test/infrastructure/vcsim/api/v1alpha1/envvar_types.go Outdated Show resolved Hide resolved
test/infrastructure/vcsim/controllers/envvar_controller.go Outdated Show resolved Hide resolved
test/infrastructure/vcsim/controllers/envvar_controller.go Outdated Show resolved Hide resolved
test/infrastructure/vcsim/controllers/envvar_controller.go Outdated Show resolved Hide resolved
test/infrastructure/vcsim/controllers/envvar_controller.go Outdated Show resolved Hide resolved
@rikatz
Copy link
Contributor Author

rikatz commented Nov 7, 2024

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@rikatz rikatz force-pushed the allow-using-moid-main branch from b630e7e to a0cf25d Compare November 7, 2024 23:32
@rikatz
Copy link
Contributor Author

rikatz commented Nov 7, 2024

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@rikatz rikatz force-pushed the allow-using-moid-main branch from a0cf25d to 65efed1 Compare November 12, 2024 15:55
@rikatz
Copy link
Contributor Author

rikatz commented Nov 12, 2024

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

@sbueringer
Copy link
Member

Thank you very much!!

Feel free to merge once tests are green

/lgtm
/approve
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e1d5181ace36378d168c423d67fad302276b36f4

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2024
dougm
dougm reviewed Nov 12, 2024
View reviewed changes
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rikatz
Copy link
Contributor Author

rikatz commented Nov 12, 2024

All tests passing, removing hold

Thanks all
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3c6373f into kubernetes-sigs:main Nov 12, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@viveksyngh viveksyngh viveksyngh left review comments

@neolit123 neolit123 neolit123 left review comments

@dougm dougm dougm left review comments

@lubronzhan lubronzhan lubronzhan approved these changes

@rgcostea rgcostea rgcostea approved these changes

@MaxRink MaxRink Awaiting requested review from MaxRink

@rvanderp3 rvanderp3 Awaiting requested review from rvanderp3

Assignees

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.
10 participants
@rikatz @chrischdi @k8s-ci-robot @neolit123 @fabriziopandini @sbueringer @dougm @viveksyngh @rgcostea @lubronzhan