Support multiple NIC for machines #254
Conversation
|
@erkanerol, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction. |
|
@erkanerol, we have received your signed contributor license agreement. It will be reviewed by VMware shortly. Another comment will be added to the pull request to notify you when the merge can proceed. |
|
@erkanerol, VMware has approved your signed contributor license agreement. |
|
@erkanerol in the case that you want to support: is it that different machine deployments will have different sets of NICs? |
@arunmk at the moment we don't have this requirement. If i'm right this requirement might be already fulfilled with this PR as we can create different |
erkanerol
force-pushed
the
multiple-nic
branch
from
4e69c0a to
ce6af2c
Compare
|
@arunmk As @bavarianbidi says, this PR supports additional network attachments for VCDMachines. For different MachineDeployments, Users can use the same VCDMachineTemplate or different ones. Both scenarios are supported. |
There was a problem hiding this comment.
I have a basic comment that may change the way this is done: should this be done in the machine controller or cluster controller?
If we do it in the cluster controller (since the machines will anyways inherit it from the vApp), we may have a simpler approach.
| @@ -510,6 +513,12 @@ func (r *VCDMachineReconciler) reconcileNormal(ctx context.Context, cluster *clu | |||
| // VCDResourceSet can get bloated with VMs if the cluster contains a large number of worker nodes | |||
| } | |||
|
|
|||
| desiredNetworks := append([]string{vcdCluster.Spec.OvdcNetwork}, vcdMachine.Spec.AdditionalOvdcNetworks...) | |||
| if err = r.reconcileVMNetworks(vdcManager, vApp, vm, desiredNetworks); err != nil { | |||
There was a problem hiding this comment.
I have a basic question: If we put the networks into the vApp, all VMs get visibility to it. Can we do this in the vcdcluster controller itself? That can in fact be an input parameter to GetOrCreateVApp. Then we do not need to do the merge etc
| @@ -754,6 +764,113 @@ func (r *VCDMachineReconciler) reconcileNormal(ctx context.Context, cluster *clu | |||
| return ctrl.Result{}, nil | |||
| } | |||
|
|
|||
| // getPrimaryNetwork returns the primary network based on vm.NetworkConnectionSection.PrimaryNetworkConnectionIndex | |||
There was a problem hiding this comment.
Nice! 👍
@Anirudh9794 we should also use this in the VM network attach flow
| return nil | ||
| } | ||
|
|
||
| // containsTheSameElements checks all elements in the two array are the same regardless of order |
There was a problem hiding this comment.
The time-complexity can be potentially improved using a small map with the key as the network name. That is a common pattern used and hence will be easier to read. This is not a big deal since the number of networks is small. Only the readability aspect is of interest, but this is also fine.
erkanerol
force-pushed
the
multiple-nic
branch
2 times, most recently
from
ea7936e to
b15a2bc
Compare
erkanerol
force-pushed
the
multiple-nic
branch
from
b15a2bc to
5fd6bb4
Compare
|
@erkanerol we were waiting to complete the release on 1.0.0 before merging this. Can you please rebase and I will merge this right away. Sincere apologies for the delay. |
|
@sahithi shall we merge this? |
Description
Fixes #235
Checklist
API Changes
Are there API changes?
If yes, please fill in the below
./examples/capi-quickstart.yaml?./infrastructure-vcd/v1.0.0/?Issue
If applicable, please reference the relevant issue
Fixes #
This change is