Alona Kaplan has posted comments on this change. Change subject: core: refactor of HostNetworkAttachmentPersister ......................................................................
Patch Set 43: (3 comments) https://gerrit.ovirt.org/#/c/36141/43/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 121: * Saves or updates network attachments which were provided by the user and were reported by the host Line 122: */ Line 123: private void saveUserNetworkAttachments() { Line 124: Map<Guid, NetworkAttachment> dbAttachmentsById = Entities.businessEntitiesById(getDbNetworkAttachments()); Line 125: //TODO MM: what should happen if there are more attachments referencing same network? Should it insert multiple records to db? Or should it update to last version? Or fail? You can assume the input is valid. You get it from an engine command, not directly form the user. Please remove the TODO comment. Line 126: //TODO MM: what if attachment has nicID already assigned? Line 127: for (NetworkAttachment attachment : userNetworkAttachments) { Line 128: if (networkConfiguredOnHost(attachment.getNetworkId())) { Line 129: VdsNetworkInterface reportedNicToWhichIsNetworkAttached = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 122: */ Line 123: private void saveUserNetworkAttachments() { Line 124: Map<Guid, NetworkAttachment> dbAttachmentsById = Entities.businessEntitiesById(getDbNetworkAttachments()); Line 125: //TODO MM: what should happen if there are more attachments referencing same network? Should it insert multiple records to db? Or should it update to last version? Or fail? Line 126: //TODO MM: what if attachment has nicID already assigned? You should act the same as you act when the '(networkConfiguredOnHost(attachment.getNetworkId())' returns false. You should ignore this attachment. Line 127: for (NetworkAttachment attachment : userNetworkAttachments) { Line 128: if (networkConfiguredOnHost(attachment.getNetworkId())) { Line 129: VdsNetworkInterface reportedNicToWhichIsNetworkAttached = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 130: attachment.setNicId(baseInterfaceOfNic(reportedNicToWhichIsNetworkAttached).getId()); Line 126: //TODO MM: what if attachment has nicID already assigned? Line 127: for (NetworkAttachment attachment : userNetworkAttachments) { Line 128: if (networkConfiguredOnHost(attachment.getNetworkId())) { Line 129: VdsNetworkInterface reportedNicToWhichIsNetworkAttached = reportedNicsByNetworkId.get(attachment.getNetworkId()); Line 130: attachment.setNicId(baseInterfaceOfNic(reportedNicToWhichIsNetworkAttached).getId()); As I wrote in the previous comment- if the nic_id on the attachment doesn't match the nic_id reported by the host, the attachment shouldn't be persisted. Line 131: boolean alreadyExistingAttachment = dbAttachmentsById.containsKey(attachment.getId()); Line 132: Line 133: if (alreadyExistingAttachment) { Line 134: networkAttachmentDao.update(attachment); -- To view, visit https://gerrit.ovirt.org/36141 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibecf02fe47f42c2184c1f010f794592944c55e62 Gerrit-PatchSet: 43 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
