Skip to content

Commit ee9507e

Browse files
vishesh92vishesh92
committed
Address comments
1 parent e438a3f commit ee9507e

File tree

9 files changed

+240
-138
lines changed

9 files changed

+240
-138
lines changed

‎engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.Map;
2222

2323
import com.cloud.dc.DataCenter;
24+
import com.cloud.hypervisor.Hypervisor;
2425
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
2526
import org.apache.cloudstack.framework.config.ConfigKey;
2627
import org.apache.cloudstack.framework.config.ConfigKey.Scope;
@@ -144,6 +145,8 @@ void prepare(VirtualMachineProfile profile, DeployDestination dest, ReservationC
144145

145146
List<NicProfile> getNicProfiles(VirtualMachine vm);
146147

148+
List<NicProfile> getNicProfiles(Long vmId, Hypervisor.HypervisorType hypervisorType);
149+
147150
Map<String, String> getSystemVMAccessDetails(VirtualMachine vm);
148151

149152
Pair<? extends NetworkGuru, ? extends Network> implementNetwork(long networkId, DeployDestination dest, ReservationContext context)

‎engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4445,25 +4445,30 @@ private boolean getNicProfileDefaultNic(NicProfile nicProfile) {
44454445
}
44464446

44474447
@Override
4448-
public List<NicProfile> getNicProfiles(final VirtualMachine vm) {
4449-
final List<NicVO> nics = _nicDao.listByVmId(vm.getId());
4448+
public List<NicProfile> getNicProfiles(final Long vmId, HypervisorType hypervisorType) {
4449+
final List<NicVO> nics = _nicDao.listByVmId(vmId);
44504450
final List<NicProfile> profiles = new ArrayList<NicProfile>();
44514451

44524452
if (nics != null) {
44534453
for (final Nic nic : nics) {
44544454
final NetworkVO network = _networksDao.findById(nic.getNetworkId());
4455-
final Integer networkRate = _networkModel.getNetworkRate(network.getId(), vm.getId());
4455+
final Integer networkRate = _networkModel.getNetworkRate(network.getId(), vmId);
44564456

44574457
final NetworkGuru guru = AdapterBase.getAdapterByName(networkGurus, network.getGuruName());
44584458
final NicProfile profile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), networkRate,
4459-
_networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vm.getHypervisorType(), network));
4459+
_networkModel.isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(hypervisorType, network));
44604460
guru.updateNicProfile(profile, network);
44614461
profiles.add(profile);
44624462
}
44634463
}
44644464
return profiles;
44654465
}
44664466

4467+
@Override
4468+
public List<NicProfile> getNicProfiles(final VirtualMachine vm) {
4469+
return getNicProfiles(vm.getId(), vm.getHypervisorType());
4470+
}
4471+
44674472
@Override
44684473
public Map<String, String> getSystemVMAccessDetails(final VirtualMachine vm) {
44694474
final Map<String, String> accessDetails = new HashMap<>();

‎engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java‎

Lines changed: 25 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,14 @@
2323
import static com.cloud.network.NetworkModel.PASSWORD_FILE;
2424
import static com.cloud.network.NetworkModel.USERDATA_FILE;
2525
import static com.cloud.network.NetworkService.DEFAULT_MTU;
26+
import static org.apache.cloudstack.storage.configdrive.ConfigDriveUtils.mergeJsonArraysAndUpdateObject;
2627

2728
import java.io.File;
2829
import java.io.IOException;
2930
import java.nio.charset.StandardCharsets;
3031
import java.nio.file.Files;
3132
import java.nio.file.Path;
3233
import java.nio.file.Paths;
33-
import java.util.HashSet;
3434
import java.util.List;
3535
import java.util.Map;
3636
import java.util.Set;
@@ -130,6 +130,9 @@ public static String buildConfigDrive(List<NicProfile> nics, List<String[]> vmDa
130130
writeNetworkData(nics, supportedServices, openStackFolder);
131131
for (NicProfile nic: nics) {
132132
if (supportedServices.get(nic.getId()).contains(Network.Service.UserData)) {
133+
if (vmData == null) {
134+
throw new CloudRuntimeException("No VM metadata provided");
135+
}
133136
writeVmMetadata(vmData, tempDirName, openStackFolder, customUserdataParams);
134137

135138
linkUserData(tempDirName);
@@ -226,42 +229,23 @@ static void writeVmMetadata(List<String[]> vmData, String tempDirName, File open
226229
* First we generate a JSON object using {@link #getNetworkDataJsonObjectForNic(NicProfile, List)}, then we write it to a file called "network_data.json".
227230
*/
228231
static void writeNetworkData(List<NicProfile> nics, Map<Long, List<Network.Service>> supportedServices, File openStackFolder) {
229-
230232
JsonObject finalNetworkData = new JsonObject();
231-
for (NicProfile nic: nics) {
232-
List<Network.Service> supportedService = supportedServices.get(nic.getId());
233-
JsonObject networkData = getNetworkDataJsonObjectForNic(nic, supportedService);
234-
235-
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "links", "id", "type");
236-
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "networks", "id", "type");
237-
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "services", "address", "type");
233+
if (needForGeneratingNetworkData(supportedServices)) {
234+
for (NicProfile nic : nics) {
235+
List<Network.Service> supportedService = supportedServices.get(nic.getId());
236+
JsonObject networkData = getNetworkDataJsonObjectForNic(nic, supportedService);
237+
238+
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "links", "id", "type");
239+
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "networks", "id", "type");
240+
mergeJsonArraysAndUpdateObject(finalNetworkData, networkData, "services", "address", "type");
241+
}
238242
}
239243

240244
writeFile(openStackFolder, "network_data.json", finalNetworkData.toString());
241245
}
242246

243-
static void mergeJsonArraysAndUpdateObject(JsonObject finalObject, JsonObject newObj, String memberName, String keyPart1, String keyPart2) {
244-
JsonArray existingMembers = finalObject.has(memberName) ? finalObject.get(memberName).getAsJsonArray() : new JsonArray();
245-
JsonArray newMembers = newObj.has(memberName) ? newObj.get(memberName).getAsJsonArray() : new JsonArray();
246-
247-
if (existingMembers.size() > 0 || newMembers.size() > 0) {
248-
JsonArray finalMembers = new JsonArray();
249-
Set<String> idSet = new HashSet<>();
250-
for (JsonElement element : existingMembers.getAsJsonArray()) {
251-
JsonObject elementObject = element.getAsJsonObject();
252-
String key = String.format("%s-%s", elementObject.get(keyPart1).getAsString(), elementObject.get(keyPart2).getAsString());
253-
idSet.add(key);
254-
finalMembers.add(element);
255-
}
256-
for (JsonElement element : newMembers.getAsJsonArray()) {
257-
JsonObject elementObject = element.getAsJsonObject();
258-
String key = String.format("%s-%s", elementObject.get(keyPart1).getAsString(), elementObject.get(keyPart2).getAsString());
259-
if (!idSet.contains(key)) {
260-
finalMembers.add(element);
261-
}
262-
}
263-
finalObject.add(memberName, finalMembers);
264-
}
247+
static boolean needForGeneratingNetworkData(Map<Long, List<Network.Service>> supportedServices) {
248+
return supportedServices.values().stream().anyMatch(services -> services.contains(Network.Service.Dhcp) || services.contains(Network.Service.Dns));
265249
}
266250

267251
/**
@@ -309,22 +293,18 @@ static JsonObject createJsonObjectWithVmData(List<String[]> vmData, String tempD
309293
static JsonObject getNetworkDataJsonObjectForNic(NicProfile nic, List<Network.Service> supportedServices) {
310294
JsonObject networkData = new JsonObject();
311295

312-
if (supportedServices.contains(Network.Service.Dhcp)) {
313-
JsonArray links = getLinksJsonArrayForNic(nic);
314-
JsonArray networks = getNetworksJsonArrayForNic(nic);
315-
if (links.size() > 0) {
316-
networkData.add("links", links);
317-
}
318-
if (networks.size() > 0) {
319-
networkData.add("networks", networks);
320-
}
296+
JsonArray links = getLinksJsonArrayForNic(nic);
297+
JsonArray networks = getNetworksJsonArrayForNic(nic);
298+
if (links.size() > 0) {
299+
networkData.add("links", links);
300+
}
301+
if (networks.size() > 0) {
302+
networkData.add("networks", networks);
321303
}
322304

323-
if (supportedServices.contains(Network.Service.Dns)) {
324-
JsonArray services = getServicesJsonArrayForNic(nic);
325-
if (services.size() > 0) {
326-
networkData.add("services", services);
327-
}
305+
JsonArray services = getServicesJsonArrayForNic(nic);
306+
if (services.size() > 0) {
307+
networkData.add("services", services);
328308
}
329309

330310
return networkData;

‎engine/storage/configdrive/src/main/java/org/apache/cloudstack/storage/configdrive/ConfigDriveUtils.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package org.apache.cloudstack.storage.configdrive;
19+
20+
import com.google.gson.JsonArray;
21+
import com.google.gson.JsonElement;
22+
import com.google.gson.JsonObject;
23+
24+
import java.util.Arrays;
25+
import java.util.HashSet;
26+
import java.util.Set;
27+
28+
public class ConfigDriveUtils {
29+
30+
static void mergeJsonArraysAndUpdateObject(JsonObject finalObject, JsonObject newObj, String memberName, String... keys) {
31+
JsonArray existingMembers = finalObject.has(memberName) ? finalObject.get(memberName).getAsJsonArray() : new JsonArray();
32+
JsonArray newMembers = newObj.has(memberName) ? newObj.get(memberName).getAsJsonArray() : new JsonArray();
33+
34+
if (existingMembers.size() > 0 || newMembers.size() > 0) {
35+
JsonArray finalMembers = new JsonArray();
36+
Set<String> idSet = new HashSet<>();
37+
for (JsonElement element : existingMembers.getAsJsonArray()) {
38+
JsonObject elementObject = element.getAsJsonObject();
39+
String key = Arrays.stream(keys).map(elementObject::get).map(JsonElement::getAsString).reduce((a, b) -> a + "-" + b).orElse("");
40+
idSet.add(key);
41+
finalMembers.add(element);
42+
}
43+
for (JsonElement element : newMembers.getAsJsonArray()) {
44+
JsonObject elementObject = element.getAsJsonObject();
45+
String key = Arrays.stream(keys).map(elementObject::get).map(JsonElement::getAsString).reduce((a, b) -> a + "-" + b).orElse("");
46+
if (!idSet.contains(key)) {
47+
finalMembers.add(element);
48+
}
49+
}
50+
finalObject.add(memberName, finalMembers);
51+
}
52+
}
53+
54+
}

‎engine/storage/configdrive/src/test/java/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilderTest.java‎

Lines changed: 0 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -636,8 +636,6 @@ public void testWriteNetworkData() throws Exception {
636636
public void testWriteNetworkDataEmptyJson() throws Exception {
637637
// Setup
638638
NicProfile nicp = mock(NicProfile.class);
639-
Mockito.when(nicp.getId()).thenReturn(1L);
640-
641639
List<Network.Service> services1 = Collections.emptyList();
642640

643641
Map<Long, List<Network.Service>> supportedServices = new HashMap<>();
@@ -662,79 +660,4 @@ public void testWriteNetworkDataEmptyJson() throws Exception {
662660
Assert.assertEquals(expectedJsonObject, actualJson);
663661
folder.delete();
664662
}
665-
666-
@Test
667-
public void testMergeJsonArraysAndUpdateObjectWithEmptyObjects() {
668-
JsonObject finalObject = new JsonObject();
669-
JsonObject newObj = new JsonObject();
670-
ConfigDriveBuilder.mergeJsonArraysAndUpdateObject(finalObject, newObj, "links", "id", "type");
671-
Assert.assertEquals("{}", finalObject.toString());
672-
}
673-
674-
@Test
675-
public void testMergeJsonArraysAndUpdateObjectWithNewMembersAdded() {
676-
JsonObject finalObject = new JsonObject();
677-
678-
JsonObject newObj = new JsonObject();
679-
JsonArray newMembers = new JsonArray();
680-
JsonObject newMember = new JsonObject();
681-
newMember.addProperty("id", "eth0");
682-
newMember.addProperty("type", "phy");
683-
newMembers.add(newMember);
684-
newObj.add("links", newMembers);
685-
686-
ConfigDriveBuilder.mergeJsonArraysAndUpdateObject(finalObject, newObj, "links", "id", "type");
687-
Assert.assertEquals(1, finalObject.getAsJsonArray("links").size());
688-
JsonObject expectedObj = new JsonParser().parse("{'links': [{'id': 'eth0', 'type': 'phy'}]}").getAsJsonObject();
689-
Assert.assertEquals(expectedObj, finalObject);
690-
}
691-
692-
@Test
693-
public void testMergeJsonArraysAndUpdateObjectWithDuplicateMembersIgnored() {
694-
JsonObject finalObject = new JsonObject();
695-
JsonArray existingMembers = new JsonArray();
696-
JsonObject existingMember = new JsonObject();
697-
existingMember.addProperty("id", "eth0");
698-
existingMember.addProperty("type", "phy");
699-
existingMembers.add(existingMember);
700-
finalObject.add("links", existingMembers);
701-
702-
JsonObject newObj = new JsonObject();
703-
newObj.add("links", existingMembers); // same as existingMembers for duplication
704-
705-
ConfigDriveBuilder.mergeJsonArraysAndUpdateObject(finalObject, newObj, "links", "id", "type");
706-
Assert.assertEquals(1, finalObject.getAsJsonArray("links").size());
707-
JsonObject expectedObj = new JsonParser().parse("{'links': [{'id': 'eth0', 'type': 'phy'}]}").getAsJsonObject();
708-
Assert.assertEquals(expectedObj, finalObject);
709-
}
710-
711-
@Test
712-
public void testMergeJsonArraysAndUpdateObjectWithDifferentMembers() {
713-
JsonObject finalObject = new JsonObject();
714-
715-
JsonArray newMembers = new JsonArray();
716-
JsonObject newMember = new JsonObject();
717-
newMember.addProperty("id", "eth0");
718-
newMember.addProperty("type", "phy");
719-
newMembers.add(newMember);
720-
finalObject.add("links", newMembers);
721-
722-
JsonObject newObj = new JsonObject();
723-
newMembers = new JsonArray();
724-
newMember = new JsonObject();
725-
newMember.addProperty("id", "eth1");
726-
newMember.addProperty("type", "phy");
727-
newMembers.add(newMember);
728-
newObj.add("links", newMembers);
729-
730-
ConfigDriveBuilder.mergeJsonArraysAndUpdateObject(finalObject, newObj, "links", "id", "type");
731-
Assert.assertEquals(2, finalObject.getAsJsonArray("links").size());
732-
JsonObject expectedObj = new JsonParser().parse("{'links': [{'id': 'eth0', 'type': 'phy'}, {'id': 'eth1', 'type': 'phy'}]}").getAsJsonObject();
733-
Assert.assertEquals(expectedObj, finalObject);
734-
}
735-
736-
@Test(expected = NullPointerException.class)
737-
public void testMergeJsonArraysAndUpdateObjectWithNullObjects() {
738-
ConfigDriveBuilder.mergeJsonArraysAndUpdateObject(null, null, "services", "id", "type");
739-
}
740663
}

0 commit comments

Comments
 (0)