secondary networks for servers#185
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ske-prow
Bot
added
the
size/M
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
hown3d
force-pushed
the
secondary-networks
branch
from
3913699 to
d849c35
Compare
|
Looks fine, but tests are missing? |
ske-prow
Bot
added
size/L
|
I added some tests |
|
|
||
| ## NetworkingSpec | ||
|
|
||
| Exactly one of the following must be set: |
There was a problem hiding this comment.
This needs to be rephrased.
networkid / nicids is mutually exclusive while secondarynetworkids is additional
| } | ||
| } | ||
|
|
||
| for i, secondaryNet := range networking.SecondaryNetworkIDs { |
There was a problem hiding this comment.
Also add a check that the list contains unique networkIDs and that they are not equal to the "primary" networkID (if set)
| return nil, status.Error(codes.Unavailable, fmt.Sprintf("failed to patch NICs for server: %v", err)) | ||
| } | ||
|
|
||
| if providerSpec.Networking != nil && len(providerSpec.Networking.SecondaryNetworkIDs) > 0 { |
There was a problem hiding this comment.
doesn't this has to happen before patching the networkinterfaces?
if this happens after patchNetworkInterface, the address from the new interface is not reflected in the CreateMachineResponse.Addresses and also it does not get the allowedAddresses patched.
This also means in the ensureAdditionalNetworks we have to attach the network and then wait until all networks are attached since this operation is async.
If we do not wait until all networks are attached, the patch function could skip some nics.
All this "waiting" feels a little cumbersome but its just how the create interface is designed. One call to rule em all.
3 participants
How to categorize this PR?
/kind enhancement
What this PR does / why we need it:
Support multiple additional networks for a server.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Breaking changes: