Arik Hadas has posted comments on this change.
Change subject: core, engine: Prevent snapshot in ppc64
......................................................................
Patch Set 1:
(5 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 150: }
Line 151:
Line 152: private MemoryImageBuilder createMemoryImageBuilder() {
Line 153: boolean archSupportSnapshot =
isSnapshotSupportedByArchitecture(getVm().getClusterArch(),
getVm().getVdsGroupCompatibilityVersion());
Line 154: if
(!FeatureSupported.memorySnapshot(getVm().getVdsGroupCompatibilityVersion()) ||
archSupportSnapshot) {
should be !archSupportSnapshot, right?
Line 155: return new NullableMemoryImageBuilder();
Line 156: }
Line 157:
Line 158: if (getParameters().getSnapshotType() ==
SnapshotType.STATELESS) {
Line 250: }
Line 251:
Line 252: boolean archSupportSnapshot =
isSnapshotSupportedByArchitecture(getVm().getClusterArch(),
getVm().getVdsGroupCompatibilityVersion());
Line 253: if (getParameters().isSaveMemory()
Line 254: && (archSupportSnapshot ||
!FeatureSupported.memorySnapshot(getVm().getVdsGroupCompatibilityVersion()))) {
same here (!archSupportSnapshot) ? how about extract this check to separate
method like 'isMemorySnapshotSupported' that checks the cluster compatibility
version and the architecture (because I see that is appears multiple times
here) ?
Line 255: log.warnFormat("A memory snapshot was required but not
created, since VM architecture does not support it.", getVmId());
Line 256: }
Line 257:
Line 258: incrementVmGeneration();
Line 251:
Line 252: boolean archSupportSnapshot =
isSnapshotSupportedByArchitecture(getVm().getClusterArch(),
getVm().getVdsGroupCompatibilityVersion());
Line 253: if (getParameters().isSaveMemory()
Line 254: && (archSupportSnapshot ||
!FeatureSupported.memorySnapshot(getVm().getVdsGroupCompatibilityVersion()))) {
Line 255: log.warnFormat("A memory snapshot was required but not
created, since VM architecture does not support it.", getVmId());
this log is not accurate because it might be printed when
getParameters().isSaveMemory() is true and FeatureSupported.memorySnapshot() is
false - no matter what the architecture be and if so it might confuse us
Line 256: }
Line 257:
Line 258: incrementVmGeneration();
Line 259:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java
Line 165: /**
Line 166: * Check if architecture supports snapshot
Line 167: * @return
Line 168: */
Line 169: public boolean isSnapshotSupportedByArchitecture
(ArchitectureType architecture, Version compatibilityVersion) {
I think that it is not the best place for this method since it probably won't
be used for most of the commands that inherit VmCommand. It can be accessible
to all the snapshot related commands by putting it in SnapshotsManager (as
static method) which is a more appropriate place for it imo. Maybe it even
worth that this method will get VM and will check both the cluster
compatibility version and the architecture
Line 170: boolean supported = false;
Line 171: if (architecture != ArchitectureType.undefined) {
Line 172: supported =
ArchStrategyFactory.getStrategy(architecture).run(new
IsSnapshotSupported(compatibilityVersion)).returnValue();
Line 173: }
Line 167: * @return
Line 168: */
Line 169: public boolean isSnapshotSupportedByArchitecture
(ArchitectureType architecture, Version compatibilityVersion) {
Line 170: boolean supported = false;
Line 171: if (architecture != ArchitectureType.undefined) {
Did you consider to add "Nullable strategy" for undefined architecture? it will
save those checks before using the ArchStrategyFactory, and I think the
implementation will be cleaner
Line 172: supported =
ArchStrategyFactory.getStrategy(architecture).run(new
IsSnapshotSupported(compatibilityVersion)).returnValue();
Line 173: }
Line 174:
Line 175: return supported;
--
To view, visit http://gerrit.ovirt.org/21657
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f201b63ddf441a9bc76a37fda33f54fa766d937
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
<gustavo.pedr...@eldorado.org.br>
Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Vitor de Lima <vitor.l...@eldorado.org.br>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
