Merge pull request #321 from Hornwitser/instance-status

Rework instance status

packages/lib/link/messages.js

@@ -290,11 +290,14 @@ messages.listInstances = new Request({
 			type: "array",
 			items: {
 				additionalProperties: false,
-				required: ["name", "id", "assigned_slave"],
+				required: ["name", "id", "assigned_slave", "status"],
 				properties: {
 					"name": { type: "string" },
 					"id": { type: "integer" },
 					"assigned_slave": { type: ["null", "integer"] },
+					"status": { enum: [
+						"unknown", "unassigned", "stopped", "starting", "running", "creating_save", "exporting_data",
+					]},
 				},
 			},
 		},
@@ -804,7 +807,7 @@ messages.debugWsMessage = new Event({
 
 messages.instanceInitialized = new Event({
 	type: "instance_initialized",
-	links: ["instance-slave", "slave-master"],
+	links: ["instance-slave"],
 	eventProperties: {
 		"instance_id": { type: "integer" },
 		"plugins": {
@@ -813,18 +816,14 @@ messages.instanceInitialized = new Event({
 		},
 	},
 });
-messages.instanceStarted = new Event({
-	type: "instance_started",
-	links: ["instance-slave", "slave-master"],
-	eventProperties: {
-		"instance_id": { type: "integer" },
-	},
-});
-messages.instanceStopped = new Event({
-	type: "instance_stopped",
+messages.instanceStatusChanged = new Event({
+	type: "instance_status_changed",
 	links: ["instance-slave", "slave-master"],
 	eventProperties: {
 		"instance_id": { type: "integer" },
+		"status": { enum: [
+			"stopped", "starting", "running", "creating_save", "exporting_data",
+		]},
 	},
 });
 
@@ -840,7 +839,9 @@ messages.updateInstances = new Event({
 				required: ["serialized_config", "status"],
 				properties: {
 					"serialized_config": { type: "object" },
-					"status": { enum: ["stopped", "initialized", "running"] },
+					"status": { enum: [
+						"stopped", "starting", "running", "creating_save", "exporting_data",
+					]},
 				},
 			},
 		},

packages/lib/plugin.js

@@ -226,10 +226,23 @@ class BaseMasterPlugin {
 	 *
 	 * Invoked when the master server has received notice from a slave that
 	 * the running status of an instance has changed.  The possible statuses
-	 * are `stopped` meaning it is not loaded or running, `initialized`
-	 * meaning the instance has initialized and is starting up the Factorio
-	 * server, and `running` which means the Factorio server it manages is
-	 * up and running.
+	 * that can be notified about are:
+	 * - `stopped`: Instance is no longer running.
+	 * - `starting`: Instance is in the process of starting up.
+	 * - `running`: Instance startup completed and is now running.
+	 * - `creating_save`: Instance is in the process of creating a save.
+	 * - `exporting_data`: Instance is in the process of exporting item
+	 *   icons and locale data.
+	 *
+	 * On master startup all known instances gets the status `unknown` if
+	 * they are assigned to a slave, when the slave then connects to the
+	 * master and informs of the current status of its instances this hook
+	 * is invoked for all those instances.  You can detect this situation by
+	 * checking if prev equals `unknown`.
+	 *
+	 * Note that it's possible for status change notification to get lost in
+	 * the case of network outages if reconnect fails to re-establish the
+	 * session between the master server and the slave.
 	 *
 	 * @param {Object} instance - the instance that changed.
 	 * @param {string} prev - the previous status of the instance.

packages/master/master.js

@@ -273,7 +273,8 @@ async function loadInstances(databaseDirectory, file) {
 		for (let serializedConfig of serialized) {
 			let instanceConfig = new config.InstanceConfig();
 			await instanceConfig.load(serializedConfig);
-			instances.set(instanceConfig.get("instance.id"), { config: instanceConfig });
+			let status = instanceConfig.get("instance.assigned_slave") === null ? "unassigned" : "unknown";
+			instances.set(instanceConfig.get("instance.id"), { config: instanceConfig, status });
 		}
 
 	} catch (err) {
@@ -611,6 +612,7 @@ class ControlConnection extends BaseConnection {
 				id: instance.config.get("instance.id"),
 				name: instance.config.get("instance.name"),
 				assigned_slave: instance.config.get("instance.assigned_slave"),
+				status: instance.status,
 			});
 		}
 		return { list };
@@ -625,7 +627,7 @@ class ControlConnection extends BaseConnection {
 		if (db.instances.has(instanceId)) {
 			throw new errors.RequestError(`Instance with ID ${instanceId} already exists`);
 		}
-		db.instances.set(instanceId, { config: instanceConfig });
+		db.instances.set(instanceId, { config: instanceConfig, status: "unassigned" });
 	}
 
 	async deleteInstanceRequestHandler(message, request) {
@@ -948,27 +950,11 @@ class SlaveConnection extends BaseConnection {
 		}
 	}
 
-	async instanceInitializedEventHandler(message, event) {
+	async instanceStatusChangedEventHandler(message, event) {
 		let instance = db.instances.get(message.data.instance_id);
 		let prev = instance.status;
-		instance.status = "initialized";
-		console.log(`Clusterio | Instance ${instance.config.get("instance.name")} Initialized`);
-		await plugin.invokeHook(masterPlugins, "onInstanceStatusChanged", instance, prev);
-	}
-
-	async instanceStartedEventHandler(message, event) {
-		let instance = db.instances.get(message.data.instance_id);
-		let prev = instance.status;
-		instance.status = "running";
-		console.log(`Clusterio | Instance ${instance.config.get("instance.name")} Started`);
-		await plugin.invokeHook(masterPlugins, "onInstanceStatusChanged", instance, prev);
-	}
-
-	async instanceStoppedEventHandler(message, event) {
-		let instance = db.instances.get(message.data.instance_id);
-		let prev = instance.status;
-		instance.status = "stopped";
-		console.log(`Clusterio | Instance ${instance.config.get("instance.name")} Stopped`);
+		instance.status = message.data.status;
+		console.log(`Clusterio | Instance ${instance.config.get("instance.name")} State: ${instance.status}`);
 		await plugin.invokeHook(masterPlugins, "onInstanceStatusChanged", instance, prev);
 	}
 
@@ -994,15 +980,16 @@ class SlaveConnection extends BaseConnection {
 				if (masterInstance.status !== instance.status) {
 					let prev = masterInstance.status;
 					masterInstance.status = instance.status;
-					if (prev !== undefined) {
-						await plugin.invokeHook(masterPlugins, "onInstanceStatusChanged", instance, prev);
-					}
+					await plugin.invokeHook(masterPlugins, "onInstanceStatusChanged", instance, prev);
 				}
 				continue;
 			}
 
 			instanceConfig.set("instance.assigned_slave", this._id);
-			db.instances.set(instanceConfig.get("instance.id"), { config: instanceConfig });
+			db.instances.set(instanceConfig.get("instance.id"), {
+				config: instanceConfig,
+				status: instance.status,
+			});
 			await link.messages.assignInstance.send(this, {
 				instance_id: instanceConfig.get("instance.id"),
 				serialized_config: instanceConfig.serialize(),

packages/slave/slave.js

@@ -176,9 +176,15 @@ class Instance extends link.Link{
 		instanceFactorioAutosaveSize.labels(String(this.config.get("instance.id"))).set(stat.size);
 	}
 
+	notifyStatus(status) {
+		link.messages.instanceStatusChanged.send(this, {
+			instance_id: this.config.get("instance.id"), status,
+		});
+	}
+
 	notifyExit() {
 		this._running = false;
-		link.messages.instanceStopped.send(this, { instance_id: this.config.get("instance.id") });
+		this.notifyStatus("stopped");
 
 		this.config.off("fieldChanged", this._configFieldChanged);
 
@@ -461,7 +467,7 @@ class Instance extends link.Link{
 		await plugin.invokeHook(this.plugins, "onStart");
 
 		this._running = true;
-		link.messages.instanceStarted.send(this, { instance_id: this.config.get("instance.id") });
+		this.notifyStatus("running");
 	}
 
 	/**
@@ -483,7 +489,7 @@ class Instance extends link.Link{
 		await plugin.invokeHook(this.plugins, "onStart");
 
 		this._running = true;
-		link.messages.instanceStarted.send(this, { instance_id: this.config.get("instance.id") });
+		this.notifyStatus("running");
 	}
 
 	/**
@@ -642,6 +648,7 @@ class Instance extends link.Link{
 	}
 
 	async createSaveRequestHandler() {
+		this.notifyStatus("creating_save");
 		try {
 			console.log("Clusterio | Writing server-settings.json");
 			await this.writeServerSettings();
@@ -660,6 +667,7 @@ class Instance extends link.Link{
 	}
 
 	async exportDataRequestHandler() {
+		this.notifyStatus("exporting_data");
 		try {
 			console.log("Clusterio | Writing server-settings.json");
 			await this.writeServerSettings();
@@ -813,19 +821,14 @@ class InstanceConnection extends link.Link {
 	}
 
 	async instanceInitializedEventHandler(message, event) {
-		this.status = "initialized";
 		this.plugins = new Map(Object.entries(message.data.plugins));
-		this.forwardEventToMaster(message, event);
 	}
 
-	async instanceStartedEventHandler(message, event) {
-		this.status = "running";
-		this.forwardEventToMaster(message, event);
-	}
-
-	async instanceStoppedEventHandler(message, event) {
-		this.status = "stopped";
-		this.slave.instanceConnections.delete(this.instanceId);
+	async instanceStatusChangedEventHandler(message, event) {
+		this.status = message.data.status;
+		if (this.status === "stopped") {
+			this.slave.instanceConnections.delete(this.instanceId);
+		}
 		this.forwardEventToMaster(message, event);
 	}
 }
@@ -1108,6 +1111,9 @@ class Slave extends link.Link {
 			path.join(instanceInfo.path, "instance.json"),
 			JSON.stringify(warnedOutput, null, 4)
 		);
+
+		// Notify master the instance is no longer usassigned
+		link.messages.instanceStatusChanged.send(this, { instance_id, status: "stopped" });
 	}
 
 	/**

test/integration/clusterio.js

@@ -8,6 +8,16 @@ const link = require("@clusterio/lib/link");
 const { slowTest, get, execCtl, sendRcon, getControl, instancesDir } = require("./index");
 
 
+async function getInstances() {
+	let result = await link.messages.listInstances.send(getControl());
+	return new Map(result.list.map(instance => [instance.id, instance]));
+}
+
+async function checkInstanceStatus(id, status) {
+	let instances = await getInstances();
+	assert.equal(instances.get(44).status, status, "incorrect instance status");
+}
+
 describe("Integration of Clusterio", function() {
 	describe("clusterioctl", function() {
 		describe("slave list", function() {
@@ -22,15 +32,19 @@ describe("Integration of Clusterio", function() {
 		});
 
 		describe("instance create", function() {
-			it("runs", async function() {
+			it("creates the instance", async function() {
 				await execCtl("instance create test --id 44");
+				let instances = await getInstances();
+				assert(instances.has(44), "instance was not created");
+				assert.equal(instances.get(44).status, "unassigned", "incorrect instance status");
 			});
 		});
 
 		describe("instance assign", function() {
 			it("creates the instance files", async function() {
 				await execCtl("instance assign test 4");
-				assert(await fs.exists(path.join(instancesDir, "test")), "Instance was not created");
+				assert(await fs.exists(path.join(instancesDir, "test")), "Instance directory was not created");
+				await checkInstanceStatus(44, "stopped");
 			});
 		});
 
@@ -38,6 +52,7 @@ describe("Integration of Clusterio", function() {
 			it("creates a save", async function() {
 				slowTest(this);
 				await execCtl("instance create-save test");
+				await checkInstanceStatus(44, "stopped");
 			});
 		});
 
@@ -46,6 +61,7 @@ describe("Integration of Clusterio", function() {
 				slowTest(this);
 				await execCtl("instance export-data test");
 				assert(await fs.exists(path.join("static", "export", "locale.json")), "Export was not created");
+				await checkInstanceStatus(44, "stopped");
 			});
 		});
 
@@ -53,7 +69,7 @@ describe("Integration of Clusterio", function() {
 			it("starts the instance", async function() {
 				slowTest(this);
 				await execCtl("instance start test");
-				// TODO check that the instance actually started
+				await checkInstanceStatus(44, "running");
 			});
 		});
 
@@ -200,7 +216,7 @@ describe("Integration of Clusterio", function() {
 			it("stops the instance", async function() {
 				slowTest(this);
 				await execCtl("instance stop test");
-				// TODO check that the instance actually stopped
+				await checkInstanceStatus(44, "stopped");
 			});
 		});
 
@@ -208,7 +224,9 @@ describe("Integration of Clusterio", function() {
 			it("deletes the instance", async function() {
 				slowTest(this);
 				await execCtl("instance delete test");
-				assert(!await fs.exists(path.join(instancesDir, "test")), "Instance was not deleted");
+				assert(!await fs.exists(path.join(instancesDir, "test")), "Instance files was not deleted");
+				let instances = await getInstances();
+				assert(!instances.has(44), "instance was not deleted from master");
 			});
 		});
 

test/lib/command.js

@@ -24,7 +24,7 @@ describe("lib/command", function() {
 				seq: 1, type: "list_instances_response",
 				data: {
 					seq: message.seq,
-					list: [{ id: 57, assigned_slave: 4, name: "Test Instance" }],
+					list: [{ id: 57, assigned_slave: 4, name: "Test Instance", status: "stopped" }],
 				},
 			});
 		} else if (message.type === "list_roles_request") {