-
Notifications
You must be signed in to change notification settings - Fork 450
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
Unified Queue: tentative DB schema, start refactoring scripts #25215
Changes from all commits
0683df1
b0056ae
960e93c
36575f5
5379011
47c370d
2f76b14
68b9ec4
6fa982b
eb96e09
e62c174
359cc84
4e450c8
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
* Added script execution to the new `upcoming_activities` table. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
package tables | ||
|
||
import ( | ||
"database/sql" | ||
) | ||
|
||
func init() { | ||
MigrationClient.AddMigration(Up_20250106162751, Down_20250106162751) | ||
} | ||
|
||
func Up_20250106162751(tx *sql.Tx) error { | ||
_, err := tx.Exec(` | ||
CREATE TABLE upcoming_activities ( | ||
id INT UNSIGNED NOT NULL AUTO_INCREMENT, | ||
getvictor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
host_id INT UNSIGNED NOT NULL, | ||
-- priority 0 is normal, > 0 is higher priority, < 0 is lower priority. | ||
priority INT NOT NULL DEFAULT 0, | ||
-- user_id is the user that triggered the activity, it may be null if the | ||
-- activity is fleet-initiated or the user was deleted. Additional user | ||
-- information (name, email, etc.) is stored in the JSON payload. | ||
user_id INT UNSIGNED NULL, | ||
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. If user was deleted, the id should point to the new 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'm not sure I'll be able to add that feature as part of this already-huge story. I'm trying to keep the scope in check as there are still tons of unknowns, storing the deleted user's info in the JSON for now. |
||
-- type of activity to be executed, currently we only support those, but as | ||
-- more activity types get added, we can enrich the ENUM with an ALTER TABLE. | ||
activity_type ENUM('script', 'software_install', 'vpp_app_install') NOT NULL, | ||
-- execution_id is the identifier of the activity that will be used when | ||
-- executed - e.g. scripts and software installs have an execution_id, and | ||
-- it is sometimes important to know it as soon as the activity is enqueued, | ||
-- so we need to generate it immediately. | ||
execution_id VARCHAR(255) NOT NULL, | ||
-- those are all columns and not JSON fields because we need FKs on them to | ||
-- do processing ON DELETE, otherwise we'd have to check for existence of | ||
-- each one when executing the activity (we need the enqueue next activity | ||
-- action to be efficient). | ||
script_id INT UNSIGNED NULL, | ||
script_content_id INT UNSIGNED NULL, | ||
policy_id INT UNSIGNED NULL, | ||
setup_experience_script_id INT UNSIGNED NULL, | ||
payload JSON NOT NULL, | ||
Comment on lines
+39
to
+44
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. This doesn't feel very clean -- a bunch of columns specific to one type of activity. What if we need to add another column -- will we keep growing this table? What do you think about normalizing and having a separate table for this script-specific data? We can still have FKs by having the JOIN column like script_activity_id. If we have this column, then maybe we also don't need activity_type/execution_id columns. Or at least the activity_type can be autogenerated by simply checking if script_activity_id is null or not. 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. Yeah I think a join table makes sense for specific activity stuff that is outside of the JSON payload. I think I'll keep the activity type as a) it cleanly describes the supported types and b) we may have some activity types that don't require a join table at all. As mentioned, I'll address those things by going forward in the next PR, please approve if there's nothing else. |
||
-- Using DATETIME instead of TIMESTAMP to prevent future Y2K38 issues | ||
created_at DATETIME(6) NOT NULL DEFAULT NOW(6), | ||
updated_at DATETIME(6) NOT NULL DEFAULT NOW(6) ON UPDATE NOW(6), | ||
PRIMARY KEY (id), | ||
UNIQUE KEY idx_upcoming_activities_execution_id (execution_id), | ||
INDEX idx_upcoming_activities_host_id_activity_type (host_id, priority, created_at, activity_type), | ||
CONSTRAINT fk_upcoming_activities_script_id | ||
FOREIGN KEY (script_id) REFERENCES scripts (id) ON DELETE SET NULL, | ||
CONSTRAINT fk_upcoming_activities_script_content_id | ||
FOREIGN KEY (script_content_id) REFERENCES script_contents (id) ON DELETE CASCADE, | ||
CONSTRAINT fk_upcoming_activities_policy_id | ||
FOREIGN KEY (policy_id) REFERENCES policies (id) ON DELETE SET NULL, | ||
CONSTRAINT fk_upcoming_activities_setup_experience_script_id | ||
FOREIGN KEY (setup_experience_script_id) REFERENCES setup_experience_scripts (id) ON DELETE SET NULL, | ||
CONSTRAINT fk_upcoming_activities_user_id | ||
FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE SET NULL | ||
) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci`, | ||
) | ||
return err | ||
} | ||
|
||
func Down_20250106162751(tx *sql.Tx) error { | ||
return nil | ||
} |
Large diffs are not rendered by default.
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.
Keep in mind that this will still evolve - it only handles scripts for now, as I try to take small bites at that huge story. Be ready to reset your DB with future changes to this same migration if you try that out.