Skip to content

Rip out all the code that implements VirtualMachine processing, older… #1474

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

Merged
merged 5 commits into from
May 22, 2025

Conversation

MontrealSergiy
Copy link
Contributor

@MontrealSergiy MontrealSergiy commented Feb 24, 2025

Rip out all the code that implements VirtualMachine processing, older cloud related attributes in bourreaux and tool configs, ec2 etc.. see #1375 for discussion

@prioux
Copy link
Member

prioux commented Feb 24, 2025

I think you forgot to add the migration that leads to schema revision 20250224221218

@prioux
Copy link
Member

prioux commented Feb 25, 2025

You also forgot to create a data migration to clean up old ApplicationRecord that no longer exist

@prioux
Copy link
Member

prioux commented Feb 25, 2025

You have the scema migration but still don't have a data migration

end

create_table "tool_configs", force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_unicode_ci" do |t|
create_table "tool_configs", force: :cascade, options: "ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci" do |t|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool_configs table's definition has changed; this happens when your dev DB is different from the one we use for the standard codebase. Just revert this line (450) with a manual commit.

@prioux
Copy link
Member

prioux commented Mar 3, 2025

Please add a data migration for your changes. I think maybe you don't know what it means. You now have two schema migrations, which changes the columns or tables of the database. But your pull request also remove some model classes and the database could still have records associated with the removed classes. This would make the system crash, if at any point those rows were accessed. So the database need also its DATA to migrated. Thus the data migration. This will be a migration that only has a up direction, not a down direction (though you could still roll it back and it should then do nothing).

@MontrealSergiy MontrealSergiy requested a review from prioux March 5, 2025 20:48
@prioux
Copy link
Member

prioux commented Mar 6, 2025

I see you added a migration to destroy the objects in the table task_vm_allocations. That is useless given you're already dropping the entire table anyway!

This is not at all what I was talking about in my comments. I was talking about modifying the database for objects that are other existing tables. Look at your PR and all the things it does, it should be obvious! What are you erasing/modifying that has impact on other things? I'll give you the info because it's doesn't seem clear to you:

  1. all bourreau records that are linked to the destroyed "scir*.rb" classes need to be removed
  2. all CBRAIN tasks of type CbrainTask::StartVM need to be destroyed (be careful while writing this ApplicationRecord query, given the class code doesn't exist anymore)

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data migration is still missing, see my previous comment.

Also, the migration that destroys objects in the task_vm_allocations table should be removed from this PR because it's useless.

@prioux
Copy link
Member

prioux commented Mar 6, 2025

Also, please test your data migration thoroughly by creating objects in the console and running it up and down. Make sure the migration really performs what you expect it to do.

@prioux
Copy link
Member

prioux commented Apr 18, 2025

Please rebase and please add the missing migration, as I explained

@MontrealSergiy MontrealSergiy requested a review from prioux May 13, 2025 15:26
@prioux
Copy link
Member

prioux commented May 22, 2025

This PR looks good now. I will test it and merge it if everything is OK.

@prioux prioux merged commit 1f40a21 into aces:master May 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rip out all the code that implements VirtualMachine processing
2 participants