cluster-api-provider-vsphere
cluster-api-provider-vsphere copied to clipboard
kubernetes-sigs
add ability to configure DHCP overrides
What this PR does / why we need it:
This PR adds the ability to configure DHCP overrides for vSphere machines. This is important for users who want to tweak behaviors of their networks, such as using only the DNS servers that they configured.
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 #1582
Special notes for your reviewer:
Our end-to-end test isn't concerned with the functional change these settings make, but with checking that capv writes the metadata correctly. Many of these settings are highly environment specific, knowing IPs or other configurations that could be difficult to account for in CI/dev environments. The responsibility of translating the metadata to network configuration falls on the distribution which we did not want to test here.
Release note:
Add ability to configure DHCP overrides
Hi @adobley. 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.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval by writing /assign @justinsb in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@adobley: PR needs rebase.
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.
@adobley This needs a rebase. /ok-to-test @yastij @vrabbi this PR could benefit from a review.
@adobley: The following tests 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 | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | false | /test pull-cluster-api-provider-vsphere-apidiff-main |
| pull-cluster-api-provider-vsphere-verify-crds | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-verify-crds |
| pull-cluster-api-provider-vsphere-verify-lint | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-verify-lint |
| pull-cluster-api-provider-vsphere-test | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-test |
| pull-cluster-api-provider-vsphere-integration-test | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-integration-test |
| pull-cluster-api-provider-vsphere-e2e | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-e2e |
| pull-cluster-api-provider-vsphere-verify-gen | 41d8fbe842900ca4cc33ce1e669125433f94fc57 | link | true | /test pull-cluster-api-provider-vsphere-verify-gen |
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.
/test pull-cluster-api-provider-vsphere-full-e2e To run the new e2e test introduced.
Considering the special notes that you have added, I wonder if the end to end test does provide something concrete? Could we not write unit and/or controller tests to mimic this behavior?
The coverage we see of the e2e tests is verifying that the DHCP values are read from the vsphereMachineTemplate and are applied accurately as metadata to the vSphere VM, unfortunately we cannot easily verify the effects of the overrides without knowing a lot of information about the environment.
We do have a unit test covering the util.GetMachineMetadata function responsible for rendering the metadata based on the template fields we set for the overrides. We can take a look at adding some controller tests to controllers/vspherevm_controller_test.go, however the template for rendering the metadata is being covered in the GetMachineMetadata test.
We think we've covered the changes, is there somewhere that we've overlooking? Would it be better to drop the e2e test and rely on unit coverage or replace it with a controller test?
Also if it is not too much, could you try adding a controller test as well.
Looking at controllers/vspherevm_controller_test.go from what we can tell it seems that the VMService is mocked out at that layer. There are no updates to the VM metadata to assert on with service mocked. Is there somewhere that we're missing that would call through down to reconcileMetadata?
Mostly lgtm pending tests passing, same ask as @srm09 regarding controller tests. Thanks for working on this!
We can take a look at adding some controller tests to controllers/vspherevm_controller_test.go, however the template for rendering the metadata is being covered in the GetMachineMetadata test. I am +1 to this part since the template rendering enhancement has already been tested out in the unit test in the
machines_test.gofile as well as the e2e test. Removing the hold and approving this one. @yastij Could you do a final pass and lgtm it, if all looks good?
[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
- ~~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