[WIP] wait for static ip allocation #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jzhoucliqr
previously approved these changes
Jul 1, 2020
controllers/vspherevm_controller.go
Outdated
| @@ -307,6 +307,11 @@ func (r vmReconciler) reconcileNormal(ctx *context.VMContext) (reconcile.Result, | |||
| // TODO(akutz) Implement selection of VM service based on vSphere version | |||
| var vmService services.VirtualMachineService = &govmomi.VMService{} | |||
|
|
|||
| if r.isWaitingForStaticIPAllocation(ctx) { | |||
| r.Logger.Info("vm is waiting for static ip to be available", "namespace", ctx.VSphereVM.Namespace, "name", ctx.VSphereVM.Name) | |||
There was a problem hiding this comment.
use ctx.Logger which already have namespace/name.
controllers/vspherevm_controller.go
Outdated
| if dev.DHCP4 || dev.DHCP6 { | ||
| return false | ||
| } else { | ||
| if len(dev.Gateway4) <= 0 || len(dev.IPAddrs) <= 0 { |
There was a problem hiding this comment.
Ack. The 2 gateways are required when the respective DHCP flags are false.
There was a problem hiding this comment.
At this point we don't know if IPAddrs has ipv4 or ipv6. So we cant determine which specific gateway(4/6) needs to be available. May be the gateway check is not required here.
|
ignore the approve, we should not merge. |
abhinavnagaraj
changed the title
jzhoucliqr
reviewed
Jul 1, 2020
controllers/vspherevm_controller.go
Outdated
| if dev.DHCP4 || dev.DHCP6 { | ||
| return false | ||
| } else { | ||
| if (!dev.DHCP4 && len(dev.Gateway4) <= 0) || (!dev.DHCP6 && len(dev.Gateway6) <= 0) || len(dev.IPAddrs) <= 0 { |
There was a problem hiding this comment.
should it be len()<= 0 ? or just len()==0 is enough?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
CAPV has support to assign static IP to VSphereMachine, but the static IP itself can be assigned by different means. For eg., one such implementation can be using a third party controller to modify VSphereMachine to add static IP after the object is created.
Irrespective of the implementation choice to assign static IP, if DHCP is disabled, the CAPV controllers have to wait for static IPs to be available and then continue to provision VMs.
Currently, if both DHCP4 and DHCP6 are disabled and a staticIP is set in HAProxyLB's Network.Devices.IpAddrs, the VSphereVM for HAProxyLB goes to 'Ready' without any IPAddress in the status. The HAProxyLB then never goes to 'Ready' since its indefinitely waiting for the VSphereVM's IPAddress.
This PR checks if both DHCP4 and DHCP6 are disabled for all network devices(NetworkSpec.devices) and if there is no static IP provided in the IPAddrs list, then the CAPV controllers wait for the static IP to be available/assigned.
This PR also makes sure the VSphereVM is set to 'Ready' only after network is reconciled.
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 #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: