✨ Add implementation of Node IPAM support #1666

tylerschultz · 2022-10-25T18:16:23Z

What this PR does / why we need it:

Adds addressesFromPools to the VSphereVM network device spec
Creates IPAddressClaims when AddressesFromPools is specified
IP addresses created by IPAM providers are assigned to vSphere machines via the VM metadata
IPv4 Gateways from IPAddresses (created by IPAM providers) are assigned to the Gateway4 field
IPv6 Gateways from IPAddresses (created by IPAM providers) are assigned to the Gateway6 field
All gateways in the same IP family assigned to a VSphereVM network device must be the same.
This implementation expects that an appropriate gateway for the IP family exists for every IPAddress that comes from an IPAM provider.
Adds the IPAddressClaimed Condition to provide visibility of the status of IP address acquisition
Adds the finalizer to prevent IPAMClaims from being deleted early
IPAddressClaims have an OwnerReference to VSphereVMs to ensure they are garbage-collected when the VM is deleted.

Co-authored-by: Aidan Obley aobley@vmware.com
Co-authored-by: Tyler Schultz tschultz@vmware.com
Co-authored-by: Christian Ang angc@vmware.com
Co-authored-by: Edwin Xie exie@vmware.com

Which issue(s) this PR fixes:
Fixes #1550

This PR implements this proposal: #1632

Special notes for your reviewer:

The e2e test will come in a separate PR. The IPAM feature requires an unreleased version of CAPI with a version of clusterctl with --ipam-provider flag and related IPAM CRDs. The main branch of CAPI also bumps to ginkgo v2. #1667

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

Release note:

Adds support for node IPAM

k8s-ci-robot · 2022-10-25T18:16:31Z

Hi @tylerschultz. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

schrej · 2022-10-27T08:50:22Z

/ok-to-test

controllers/vspherevm_controller.go

srm09

Did an initial pass, bunch of nits apart from setting the Conditions on the VSphereVM object. Need to fully review the reconcileIPAddresses function once, but the changes overall look good.

config/rbac/role.yaml

pkg/manager/manager.go

pkg/services/govmomi/service.go

pkg/util/machines.go

pkg/util/machines_test.go

apis/v1beta1/vspherevm_types.go

tylerschultz · 2022-10-31T21:30:43Z

/retest-required

tylerschultz · 2022-11-02T23:42:53Z

Apologies, the conversations were folded and I didn't see several items! All should be resolved now. Thanks!

srm09

lgtm apart from a few nits.
/approve
/hold

controllers/vspherevm_controller.go

pkg/services/govmomi/service.go

srm09 · 2022-11-03T21:21:38Z

@yastij @schrej Do take a pass at reviewing the PR and feel free to remove the hold once you feel it is good to go.

schrej

Very clean, well done!
Just two small things, otherwise
/lgtm

I didn't look at tests in detail though.

controllers/vspherevm_controller.go

pkg/services/govmomi/service.go

yastij

/approve

pkg/services/govmomi/service.go

pkg/util/machines.go

pkg/services/govmomi/service.go

k8s-ci-robot · 2022-11-10T11:05:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: srm09, yastij

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~~ [srm09,yastij]

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

yastij · 2022-11-10T11:06:14Z

lgtm pending answers to the question above

yastij

/hold

pkg/util/machines.go

- Adds `addressesFromPools` to the VSphereVM network device spec - Creates IPAddressClaims when AddressesFromPools is specified - IP addresses created by IPAM providers are assigned to vSphere machines via the VM metadata - IPv4 Gateways from IPAddresses (created by IPAM providers) are assigned to the Gateway4 field - IPv6 Gateways from IPAddresses (created by IPAM providers) are assigned to the Gateway6 field - All gateways in the same IP family assigned to a VSphereVM network device must be the same. - This implementation expects that an appropriate gateway for the IP family exists for every IPAddress that comes from an IPAM provider. - Adds the `IPAddressClaimed` Condition to provide visibility of the status of IP address acquisition - Adds the finalizer to prevent IPAMClaims from being deleted early - IPAddressClaims have an OwnerReference to VSphereVMs to ensure they are garbage-collected when the VM is deleted. Co-authored-by: Tyler Schultz <tschultz@vmware.com> Co-authored-by: Christian Ang <angc@vmware.com> Co-authored-by: Edwin Xie <exie@vmware.com>

k8s-ci-robot · 2022-11-10T18:48:46Z

@tylerschultz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-apidiff-main	`606f6d5`	link	false	`/test pull-cluster-api-provider-vsphere-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

srm09 · 2022-11-11T04:56:24Z

/unhold
I will let @yastij add the final lgtm which should let this one merge.

yastij · 2022-11-11T16:52:23Z

/lgtm

tylerschultz · 2022-11-11T17:52:14Z

Thanks all!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 25, 2022

k8s-ci-robot requested review from MaxRink and yastij October 25, 2022 18:16

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 25, 2022

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 25, 2022

adobley force-pushed the ipam-support branch from 961b987 to c8ec99e Compare October 25, 2022 19:02

tylerschultz mentioned this pull request Oct 25, 2022

[WIP] ✨ E2E for IPAM support #1667

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 27, 2022

flawedmatrix force-pushed the ipam-support branch from c8ec99e to 18faa82 Compare October 27, 2022 18:00

tylerschultz commented Oct 28, 2022

View reviewed changes

controllers/vspherevm_controller.go Outdated Show resolved Hide resolved

srm09 reviewed Oct 28, 2022

View reviewed changes

tylerschultz force-pushed the ipam-support branch 2 times, most recently from bb30246 to 2f8ed9e Compare October 31, 2022 21:01

tylerschultz force-pushed the ipam-support branch from 2f8ed9e to d9bc3d6 Compare November 2, 2022 19:24

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

tylerschultz force-pushed the ipam-support branch 2 times, most recently from c22fdf9 to 9ab673d Compare November 2, 2022 21:11

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

flawedmatrix force-pushed the ipam-support branch from 9ab673d to 8e11add Compare November 2, 2022 23:40

srm09 reviewed Nov 3, 2022

View reviewed changes

controllers/vspherevm_controller.go Show resolved Hide resolved

pkg/services/govmomi/service.go Outdated Show resolved Hide resolved

pkg/services/govmomi/service.go Show resolved Hide resolved

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 3, 2022

flawedmatrix force-pushed the ipam-support branch from 8e11add to 0559756 Compare November 3, 2022 22:56

schrej approved these changes Nov 9, 2022

View reviewed changes

controllers/vspherevm_controller.go Outdated Show resolved Hide resolved

pkg/services/govmomi/service.go Show resolved Hide resolved

pkg/services/govmomi/service.go Outdated Show resolved Hide resolved

flawedmatrix force-pushed the ipam-support branch from 0559756 to ef3ece3 Compare November 9, 2022 19:55

yastij reviewed Nov 10, 2022

View reviewed changes

pkg/services/govmomi/service.go Show resolved Hide resolved

pkg/util/machines.go Outdated Show resolved Hide resolved

pkg/services/govmomi/service.go Show resolved Hide resolved

yastij reviewed Nov 10, 2022

View reviewed changes

pkg/util/machines.go Show resolved Hide resolved

christianang force-pushed the ipam-support branch from ef3ece3 to 1034ef5 Compare November 10, 2022 18:34

srm09 added this to the v1.5.0 milestone Nov 10, 2022

christianang force-pushed the ipam-support branch from 1034ef5 to 606f6d5 Compare November 10, 2022 18:46

This was referenced Nov 10, 2022

IPAM status conditions enhancements #1689

Closed

IPAM: create IPAddressClaims sooner #1690

Closed

christianang mentioned this pull request Nov 11, 2022

Add Node IPAM support for vSphere vmware-tanzu/tanzu-framework#3899

Merged

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 11, 2022

k8s-ci-robot assigned yastij Nov 11, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2022

k8s-ci-robot merged commit 26fa80a into kubernetes-sigs:main Nov 11, 2022

tylerschultz deleted the ipam-support branch November 11, 2022 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add implementation of Node IPAM support #1666

✨ Add implementation of Node IPAM support #1666

tylerschultz commented Oct 25, 2022 •

edited by srm09

Loading

k8s-ci-robot commented Oct 25, 2022

schrej commented Oct 27, 2022

srm09 left a comment

tylerschultz commented Oct 31, 2022

tylerschultz commented Nov 2, 2022

srm09 left a comment

srm09 commented Nov 3, 2022

schrej left a comment •

edited

Loading

yastij left a comment

k8s-ci-robot commented Nov 10, 2022

yastij commented Nov 10, 2022

yastij left a comment

k8s-ci-robot commented Nov 10, 2022

srm09 commented Nov 11, 2022

yastij commented Nov 11, 2022

tylerschultz commented Nov 11, 2022

✨ Add implementation of Node IPAM support #1666

✨ Add implementation of Node IPAM support #1666

Conversation

tylerschultz commented Oct 25, 2022 • edited by srm09 Loading

k8s-ci-robot commented Oct 25, 2022

schrej commented Oct 27, 2022

srm09 left a comment

Choose a reason for hiding this comment

tylerschultz commented Oct 31, 2022

tylerschultz commented Nov 2, 2022

srm09 left a comment

Choose a reason for hiding this comment

srm09 commented Nov 3, 2022

schrej left a comment • edited Loading

Choose a reason for hiding this comment

yastij left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2022

yastij commented Nov 10, 2022

yastij left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2022

srm09 commented Nov 11, 2022

yastij commented Nov 11, 2022

tylerschultz commented Nov 11, 2022

tylerschultz commented Oct 25, 2022 •

edited by srm09

Loading

schrej left a comment •

edited

Loading