Configuration backup/restore in online mode #145

mchoma · 2016-11-14T09:58:49Z

The ConfigurationFileBackup set of offline commands facilitate taking a backup of the server configuration and later restoring it. It would be very useful to have the same possibility in the online mode, e.g. using the existing snapshot function.

Ladicek · 2016-11-14T10:48:04Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+ * destroy command does nothing.
+ * </p>
+ */
+


Empty line.

Ladicek · 2016-11-14T10:48:56Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+        public void apply(OnlineCommandContext occ) throws CommandFailedException, IOException, CliException {
+            if (SnapshotBackup.this.snapshotName != null) {
+                throw new CommandFailedException("Snapshot was already backed up as "
+                        + SnapshotBackup.this.snapshotName);


"Snapshot was already taken: " + SnapshotBackup.this.snapshotName

Ladicek · 2016-11-14T10:49:37Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+                snapshot = modelNodeResult.stringValue();
+            }
+            // returns absolute path, but we need just the name of snapshot
+            snapshot = snapshot.substring(snapshot.lastIndexOf(File.separator) + 1);


Is using File.separator here accurate? What happens if I'm running on Linux and the server is on Windows?

Any idea how to handle this properly? Thing is snapshotName as value of --server-config parameter for reload could not be absolute path.
Note, File.separator is already used in AddModule.

Well file names on Windows can contain neither / nor \. So if the string contains /, it obviously isn't Windows and you can split on /, otherwise you split on \.

Re AddModule: this command only makes sense when running on the same system as the server. That doesn't apply in this situation.

Ladicek · 2016-11-14T10:51:05Z

testsuite/common/src/main/java/org/wildfly/extras/creaper/test/WildFly11Tests.java

@@ -0,0 +1,5 @@
+package org.wildfly.extras.creaper.test;
+
+/** Category marker for tests that require WildFly and can't run with AS7. */


The comment is clearly copy & paste from WildFlyTests. Please update.

Ladicek · 2016-11-14T10:52:21Z

testsuite/pom.xml

@@ -28,12 +28,16 @@
        <version.wildfly10>10.0.0.Final</version.wildfly10>
        <version.wildfly10.core>2.0.10.Final</version.wildfly10.core>
        <version.wildfly10.arquillian>1.1.0.Alpha1</version.wildfly10.arquillian>
+        <version.wildfly11>11.0.0.Alpha1-SNAPSHOT</version.wildfly11>


Snapshot dependencies are bad. No idea how to fix this...

It can be replaced with <version.wildfly11>11.0.0.Alpha1</version.wildfly11>.
It will be first valid public value. It can lead user to realize what is going on here. Better than SNAPSHOT, which looks like a bug.

Hmm, OK, but at least add a comment. Something like:

 <version.wildfly11>11.0.0.Alpha1</version.wildfly11>

And a similar comment to the wildfly11 profile.

Ladicek · 2016-11-14T10:54:17Z

.../src/test/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackupTest.java

+
+    @Before
+    public void connect() throws IOException {
+        client = getManagementClient();


Is the getManagementClient method used elsewhere? I can only see that it's used here, in which case please inline it.

Ladicek · 2016-11-14T10:54:57Z

.../src/test/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackupTest.java

+    }
+
+    @Test
+    public void correctUsage() throws CommandFailedException, IOException, OperationException, InterruptedException {


restoreTwice is apparently also correct usage, so we'll need to update the names.

Ladicek · 2016-11-14T10:57:30Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+import com.google.common.base.Strings;
+
+/**
+ * Provides a pair of online commands to backup and then restore the snapshot.


This needs to be reworded. We're not backing up and then restoring the snapshot. We're backing up and then restoring application server configuration, and we're using the snapshot features to do so. I'd suggest:

Provides a pair of online commands to backup and then restore application server configuration while the server is running. Backup consists of taking a snapshot, restore then means reloading the server from the snapshot.

Ladicek · 2016-11-14T10:59:16Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+ * Provides a pair of online commands to backup and then restore the snapshot.
+ * <p>
+ * The {@code backup} command must be applied before {@code restore}, and
+ * {@code backup} can be applied once. If any one of these rules is violated, an


This reads weird. I'd suggest: The {@code backup} command must be applied before {@code restore}, {@code backup} can be applied once, and then {@code restore} can be applied many times.

Ladicek · 2016-11-14T13:04:35Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+         * @param snapshotAbsolutePath Absolute path to snapshot
+         * @return name of snapshot
+         */
+        private String getSnapshotName(String snapshotAbsolutePath) {


As the first line of the method body, I'd expect a comment along the lines of Windows file names can't contain /, so if there's a / in the full path, it's not Windows.

Ladicek · 2016-11-14T13:05:35Z

OK, looks mostly good. When you fix the one last thing, please also squash.

mchoma · 2016-11-15T06:52:43Z

PR now contains just squashed commit.

Ladicek · 2016-11-15T06:57:40Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+        public void apply(OnlineCommandContext occ) throws CommandFailedException, IOException, InterruptedException,
+            TimeoutException {
+            if (SnapshotBackup.this.snapshotName == null) {
+                throw new CommandFailedException("There's no snapshot backup to restore");


Ah, one more! :-) There's no snapshot to restore

Ladicek · 2016-11-15T06:58:58Z

testsuite/pom.xml

+        <!-- doesn't exist yet; update when there's a public release of WildFly 11 -->
+        <version.wildfly11>11.0.0.Alpha1</version.wildfly11>
+        <version.wildfly11.core>3.0.0.Alpha10</version.wildfly11.core>
+        <version.wildfly11.arquillian>2.0.0.Final</version.wildfly11.arquillian>

        <managementPort.as7>9999</managementPort.as7>
        <managementPort.wildfly>9990</managementPort.wildfly>

        <ignoredCategory.as7>org.wildfly.extras.creaper.test.WildFlyTests</ignoredCategory.as7>


Now I'm thinking... should this line also contain WildFly11Tests?

I think no, because SnapshotBackupTest is declared as Category {WildFlyTests, WildFly11Tests}

So the WildFly11Tests category is meant as a "subset" of WildFlyTests? And everytime a test is in category WildFly11Tests, it also has to be in category WildFlyTests? That kinda sorta makes sense, but should be at least documented.

I have added comment to WildFly11Tests.

Ladicek

OK, this is the last thing :-)

Ladicek · 2016-11-16T07:33:30Z

...ands/src/main/java/org/wildfly/extras/creaper/commands/foundation/online/SnapshotBackup.java

+            // returns absolute path, but we need just the name of snapshot
+            String snapshotName = getSnapshotName(snapshotAbsolutePath);
+            if (Strings.isNullOrEmpty(snapshotAbsolutePath)) {
+                throw new CommandFailedException("Unable to take snapshot");


Ah, here's an idea: if the :take-snapshot operation fails, we're silently dropping the error message. That's bad. If there's one, it should be added to this exception.

Looking into the code, I believe execute() should take care of that - if result is failed throws exception. Now it seems to me this check is unnecessary. Also unnecessary is if (modelNodeResult.isSuccess() WDYT?

OnlineManagementClient.execute won't throw in case of an error. However, you can do ModelNodeResult.assertSuccess(), which would be enough in this case IMHO.

Ladicek · 2016-11-16T10:47:22Z

Could you please remove the [14] from the commit message? I'll merge then.

mchoma · 2016-11-16T11:57:14Z

Done

Ladicek · 2016-11-16T12:02:50Z

Not really, you only renamed the PR :-)

mchoma · 2016-11-16T12:48:39Z

Done

mchoma · 2016-11-16T13:17:27Z

I can't believe that :O

Ladicek · 2016-11-16T13:24:32Z

I'd like to point out that there were PRs that took more than a month to merge. Compared to that, this one was pretty fast ;-)

Ladicek requested changes Nov 14, 2016

View reviewed changes

mchoma force-pushed the issue12_backup_restore_online branch from 1c3a8af to af7d435 Compare November 14, 2016 13:46

Ladicek requested changes Nov 15, 2016

View reviewed changes

mchoma force-pushed the issue12_backup_restore_online branch 2 times, most recently from 621e113 to aa5f06b Compare November 16, 2016 06:18

Ladicek requested changes Nov 16, 2016

View reviewed changes

mchoma force-pushed the issue12_backup_restore_online branch from aa5f06b to aa7663a Compare November 16, 2016 10:10

Ladicek approved these changes Nov 16, 2016

View reviewed changes

Ladicek self-assigned this Nov 16, 2016

Ladicek added this to the 1.6.0 milestone Nov 16, 2016

mchoma changed the title ~~[14] Configuration backup/restore in online mode~~ Configuration backup/restore in online mode Nov 16, 2016

Configuration backup/restore in online mode

d5a591f

mchoma force-pushed the issue12_backup_restore_online branch from aa7663a to d5a591f Compare November 16, 2016 12:27

Ladicek merged commit d2c17b5 into wildfly-extras:master Nov 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration backup/restore in online mode #145

Configuration backup/restore in online mode #145

mchoma commented Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

mchoma Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

mchoma Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek Nov 14, 2016

Ladicek commented Nov 14, 2016

mchoma commented Nov 15, 2016

Ladicek Nov 15, 2016

Ladicek Nov 15, 2016

mchoma Nov 15, 2016

Ladicek Nov 15, 2016

mchoma Nov 16, 2016

Ladicek left a comment

Ladicek Nov 16, 2016

mchoma Nov 16, 2016

Ladicek Nov 16, 2016

Ladicek commented Nov 16, 2016

mchoma commented Nov 16, 2016

Ladicek commented Nov 16, 2016

mchoma commented Nov 16, 2016

mchoma commented Nov 16, 2016

Ladicek commented Nov 16, 2016

		@@ -0,0 +1,5 @@
		package org.wildfly.extras.creaper.test;

		/** Category marker for tests that require WildFly and can't run with AS7. */

Configuration backup/restore in online mode #145

Configuration backup/restore in online mode #145

Conversation

mchoma commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Nov 14, 2016

mchoma commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Nov 16, 2016

mchoma commented Nov 16, 2016

Ladicek commented Nov 16, 2016

mchoma commented Nov 16, 2016

mchoma commented Nov 16, 2016

Ladicek commented Nov 16, 2016