-
Notifications
You must be signed in to change notification settings - Fork 5
Run the Manager locally without Proxmox (make dev + mock provisioning) #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
81aba61
fa4e8a2
eb55aaa
8d07bc7
55ee04e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| node_modules | ||
| .env | ||
| .tmp-verify/ | ||
| data/ | ||
| *.sqlite | ||
| .playwright-mcp/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| .env | ||
| node_modules | ||
| data/ | ||
| *.sqlite | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are constantly fighting this bug with sqlite which is why production and the current docker compose stack use postgresql. I'm trying to avoid merging sqlite specific fixes with plans to remove sqlite support in a future PR. Officially Postgres is the only supported DB. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| 'use strict'; | ||
|
|
||
| /** | ||
| * SQLite's changeColumn recreates the table and accidentally applied a column- | ||
| * level UNIQUE on Containers.nodeId. A node (Proxmox host) can own many | ||
| * containers, so the constraint is wrong. The correct uniqueness guarantee is | ||
| * already enforced by the (nodeId, containerId) composite index. | ||
| * | ||
| * Postgres was never affected because its ALTER COLUMN does not rebuild tables. | ||
| * This migration is a no-op on Postgres (removeConstraint on a non-existent | ||
| * constraint will throw, so we guard). | ||
| */ | ||
| module.exports = { | ||
| async up(queryInterface, Sequelize) { | ||
| const dialect = queryInterface.sequelize.getDialect(); | ||
|
|
||
| if (dialect === 'sqlite') { | ||
| // SQLite requires a full table rebuild to drop a column-level constraint. | ||
| await queryInterface.sequelize.transaction(async (t) => { | ||
| // 1. Create replacement table without the UNIQUE on nodeId | ||
| await queryInterface.sequelize.query( | ||
| `CREATE TABLE "Containers_new" AS SELECT * FROM "Containers" WHERE 0`, | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query(`DROP TABLE "Containers_new"`, { transaction: t }); | ||
|
|
||
| // Recreate properly using Sequelize's createTable so the model sync | ||
| // matches. Easier: use raw DDL mirroring the current schema minus UNIQUE. | ||
| const [[{ sql }]] = await queryInterface.sequelize.query( | ||
| `SELECT sql FROM sqlite_master WHERE type='table' AND name='Containers'`, | ||
| { transaction: t }, | ||
| ); | ||
|
|
||
| // Replace the column-level UNIQUE on nodeId | ||
| const fixed = sql.replace( | ||
| /`nodeId` INTEGER NOT NULL UNIQUE/, | ||
| '`nodeId` INTEGER NOT NULL', | ||
| ); | ||
|
|
||
| if (fixed === sql) { | ||
| // Constraint not present — nothing to do | ||
| return; | ||
| } | ||
|
|
||
| const newName = 'Containers_new'; | ||
| await queryInterface.sequelize.query( | ||
| fixed.replace(/CREATE TABLE "Containers"/, `CREATE TABLE "${newName}"`), | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query( | ||
| `INSERT INTO "${newName}" SELECT * FROM "Containers"`, | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query(`DROP TABLE "Containers"`, { transaction: t }); | ||
| await queryInterface.sequelize.query( | ||
| `ALTER TABLE "${newName}" RENAME TO "Containers"`, | ||
| { transaction: t }, | ||
| ); | ||
|
|
||
| // Recreate the indexes that were on the old table | ||
| await queryInterface.sequelize.query( | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS "containers_node_id_container_id_unique" ON "Containers" ("nodeId", "containerId")`, | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query( | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS "containers_site_hostname_unique_idx" ON "Containers" ("siteId", "hostname")`, | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query( | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS "containers_site_ipv4_unique_idx" ON "Containers" ("siteId", "ipv4Address")`, | ||
| { transaction: t }, | ||
| ); | ||
| await queryInterface.sequelize.query( | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS "containers_site_mac_unique_idx" ON "Containers" ("siteId", "macAddress")`, | ||
| { transaction: t }, | ||
| ); | ||
| }); | ||
| } | ||
| // Postgres: nodeId was never accidentally made UNIQUE — nothing to do. | ||
| }, | ||
|
|
||
| async down(queryInterface, Sequelize) { | ||
| // Intentionally left empty — restoring the erroneous constraint would be | ||
| // counter-productive. | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ const { | |
| User, | ||
| Group, | ||
| Setting, | ||
| Site, | ||
| Node, | ||
| ExternalDomain, | ||
| PasswordResetToken, | ||
| InviteToken, | ||
|
|
@@ -68,6 +70,29 @@ if (process.env.NODE_ENV !== 'production') { | |
| const isAdmin = req.body?.role === 'admin'; | ||
| const uid = isAdmin ? 'dev-admin' : 'dev-user'; | ||
|
|
||
| // Ensure a localhost site exists to work with. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These new defaults look like they're going to conflict with the bootstrapping done by the Docker Compose setup. I'll need to be at my laptop to test it though.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirmed this messed up the docker-compose bootstrap. It imports the nodes and containers into this localhost site instead of the intended "Development" site which has the IP ranges configured properly. |
||
| const [site] = await Site.findOrCreate({ | ||
| where: { name: 'localhost' }, | ||
| defaults: { internalDomain: 'localhost', externalIp: '127.0.0.1' }, | ||
| }); | ||
|
|
||
| // Ensure a local dev node exists so containers can be created without | ||
| // a real Proxmox host. `apiUrl: 'local'` marks it as the mock node; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to reiterate my previous comment, I don't like mocks, but this implementation relying on in-band signaling is a massive anti-pattern since it overloads the use of that database field creating a convention contributors have to "just know" to understand why their mock is or isn't working. It's not really self-documenting. If I'm going to have to merge mock support, at least give me a dedicated field like 'dummyNode' (boolean) which only gets used if there's no real nodes available. |
||
| // container creation short-circuits provisioning for it in dev. | ||
| await Node.findOrCreate({ | ||
| where: { siteId: site.id, apiUrl: 'local' }, | ||
| defaults: { name: 'local', tokenId: 'local', secret: 'local', ipv4Address: '127.0.0.1' }, | ||
| }); | ||
|
|
||
| // Ensure a default external domain exists so HTTP services (added | ||
| // automatically by image templates) have a domain to bind to. Without | ||
| // this the new-container form is unsubmittable for any image that | ||
| // exposes an HTTP port. | ||
| await ExternalDomain.findOrCreate({ | ||
| where: { name: 'localhost' }, | ||
| defaults: { siteId: site.id }, | ||
| }); | ||
|
|
||
| let user = await User.findOne({ | ||
| where: { uid }, | ||
| include: [{ association: 'groups' }], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| */ | ||
|
|
||
| const express = require('express'); | ||
| const crypto = require('crypto'); | ||
| const { | ||
| Container, | ||
| Service, | ||
|
|
@@ -347,6 +348,30 @@ router.post( | |
| } | ||
| } | ||
|
|
||
| // Local dev: short-circuit Proxmox provisioning. The on-demand `local` | ||
| // node (apiUrl === 'local') has no real hypervisor, so mark the container | ||
| // running immediately with placeholder VMID/MAC/IP. This is the hook point | ||
| // where a real Docker-based local provisioner can plug in later. | ||
| if (process.env.NODE_ENV !== 'production' && node.apiUrl === 'local') { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I can express enough how much of a bad idea I think this is. If changes are hitting this code path, they need to be QAed against a real Proxmox API. That's the whole point of the docker-compose setup. I have always taken a hard stance against mocks because its impossible to guarantee this is going to even remotely replicate what the actual code path is doing. Adding this will make my job as a code review significantly harder going forward because I will have no assurance that changes were ever tried against a Proxmox environment. I'm willing to entertain a lighterweight WebUI development workflow, but I won't approve mock providers since it drastically changes the assumptions about the environment.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a follow-up, I would be happier with this if there was an abstraction layer instead of a dedicated code path. Currently we just have the ProxmoxApi class, but I envision that providing an interface (NodeApi) which could then be implemented by a DummyApi class that the mock node uses. Code that requires interacting with Nodes would call through the appropriate Api class (determined by a new nodeType field). This would still require running the job-runner.js alongside the server if you need containers to get created, but that's a benefit to me since it ensures the full container creation code path if being exercised. |
||
| const fakeVmid = 9000 + container.id; | ||
| const hex = () => crypto.randomBytes(1)[0].toString(16).padStart(2, '0').toUpperCase(); | ||
| await container.update( | ||
| { | ||
| containerId: fakeVmid, | ||
| macAddress: `BC:24:11:${hex()}:${hex()}:${hex()}`, | ||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
| ipv4Address: `10.0.0.${100 + (container.id % 150)}`, | ||
| status: 'running', | ||
| }, | ||
| { transaction: t }, | ||
| ); | ||
| await t.commit(); | ||
| return created(res, { | ||
| containerId: container.id, | ||
| hostname: container.hostname, | ||
| status: 'running', | ||
| }); | ||
| } | ||
|
|
||
| const job = await Job.create( | ||
| { | ||
| command: `node bin/create-container.js --container-id=${container.id}`, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here if its already in the root .gitignore?