Remove EvmDatabaseOps and related rake tasks

[NickLaMuro]

Well... mostly.

A single (set of) method(s) is needed as the appliance_console uses this method to check the current database connection.

This code could be moved into the console itself, but since it was released recently, this allows that code to still function until a new version not requiring it can be released.

Links

• Remove Database Backups from core (move to appliance console) #21100
• Remove PostgresAdmin manageiq-gems-pending#516
• Depends on:
  • Remove DatabaseBackup #21415
  • Pull in .db_connections code from core manageiq-appliance_console#172
  • Release new version of appliance_console with PR above

[Fryguy]

@jrafanie Please review.

[NickLaMuro]

Realized this morning that we should also probably remove the DatabaseBackups code still with this change, so will be looking at getting those changes back in here as well.

[kbrock]

Really excited about the cleanup work you are are doing
Heh. Every time I see green code I wonder why we are adding it here and not in the appliance console. (the gc code before and now this)

[NickLaMuro]

I also made another branch to remove the DatabaseBackup code as well, which is a bit more than this, but possibly something we should consider:

master...NickLaMuro:remove-database-ops-and-backups

I could see doing it both ways

• where we do this PR as is first, and then follow up
• do this all this at once

The second option would probably need a UI and schema PR, though.

[Fryguy]

I am for deleting features as one set of deletes per feature. I know this started as trying to delete FileDepot, which isn't a feature so much as a library, so I am +1 on deleting the dependent features first, then ultimately killing FileDepot. I also generally like to split UI and API "usage" of a backend feature into separate PRs, but mostly for review purposes, as UI/API PR can generally be merged in their entirety before deleting the backend.

In that sense, the feature in question is database backups, and deleting that includes deleting the model, the relationships, and the "tools" which is the rake tasks (I assume the UI is already deleted). I could see the "tools" being like the UI/API, which can be merged independently, so from a review perspective, I feel like having them as a separate PR is best.

[NickLaMuro]

(I assume the UI is already deleted)

No, this isn't the case yet. I need to make that PR on it's own again as well.

[jrafanie]

Heh. Every time I see green code I wonder why we are adding it here and not in the appliance console. (the gc code before and now this)

Maybe they were added when there was no appliance_console repo so they all lived here? Either way, I agree, if we're only using it in console, it should go there if we even want to keep some of the features.

[NickLaMuro]

Every time I see green code I wonder why we are adding it here and not in the appliance console.

This whole thing came about from the concept that the UI conversion of the ops view for database backups needed an API endpoint, and that specifically was determined that it was a feature we didn't want to support, so axing the view was a better idea.

To not completely strip the database backups functionality, it was simplified to be self contained, and so we are now deleting the features specific to the database backups specifically. FileDepot and friends would have been nice to delete, but those still have their hands in other parts of the code base, so they can't be removed.

This gc feature, I don't know where it is used, and so I don't want to touch it. But since PostgresAdmin will no longer exist, at a minimum, I wanted to preserve is functionality, and it's removal can be determined at a later date.

[jrafanie]

This gc feature, I don't know where it is used, and so I don't want to touch it. But since PostgresAdmin will no longer exist, at a minimum, I wanted to preserve is functionality, and it's removal can be determined at a later date.

WAIT. I will redeem myself from poo poing your delete of file depot due to pxe server. 💩

I very much believe nothing is calling gc anymore. See: ManageIQ/manageiq-ui-classic#7535
and #20877

NOTE: As long as we drop it from appliance console, db backup, miq_schedule, evm_database_ops, and evm_dba.rake but that's what you're mostly doing here. I think. If you remove the rest, I think we're good.

[Fryguy]

Good eyes @jrafanie. It seems that in #20877 run_adhoc_db_gc was removed but for some reason not adhoc_db_gc. Unless there are dynamic callers, I can't find a caller for adhoc_db_gc

[Fryguy]

@NickLaMuro I noticed that DatabaseBackup.gc calls EvmDatbaseOps.gc, so at least that method must be removed from this PR.

manageiq/app/models/database_backup.rb

Line 61 in 1ad1f72

EvmDatabaseOps.gc(current_db_opts.merge(options))

[Fryguy]

Actually also noticed EvmDatabaseOps.backup in there - here's the whole list of callers that I can find:

https://github.com/search?q=org%3AManageIQ+EvmDatabaseOps&type=code

relevant ones:

/Users/jfrey/dev/manageiq-appliance_console/lib/manageiq/appliance_console/utilities.rb:21:                                :params => ["exit EvmDatabaseOps.database_connections"],
/Users/jfrey/dev/manageiq/app/models/database_backup.rb:48:    EvmDatabaseOps.backup(current_db_opts, connect_opts)
/Users/jfrey/dev/manageiq/app/models/database_backup.rb:61:    EvmDatabaseOps.gc(current_db_opts.merge(options))

[NickLaMuro]

lib/manageiq/appliance_console/utilities.rb:21

I think I noticed this as well, but forgot to act on it. Was going to just hard code that logic into appliance_console, since there is no reason to keep it calling out to core for that.

app/models/database_backup.rb...

I plan on deleting that as mentioned here:

#21384 (comment)

But, that probably does mean that we should delete DatabaseBackup code first in a separate PR, then delete this code.

---

Whew... @jrafanie who knew that deleting code was so complex! 😩

[NickLaMuro]

I very much believe nothing is calling gc anymore.

Wasn't sure if gc was being used as just a rake task on it's own.

manageiq/lib/tasks/evm_dba.rake

Lines 107 to 124 in 1ad1f72

	desc "clean up database"
	task :gc do
	opts = EvmDba.with_options(:db_credentials) do
	opt :aggressive, "Aggressive gc: vaccume with all options and reindexing"
	opt :vacuum, "Vacuum database"
	opt :reindex, "Reindex database (or table if --table specified)"
	opt :analyze, "Vacuum with analyze"
	opt :full, "Vacuum full"
	opt :verbose, "Vacuum with verbose information printed"
	opt :table, "Tablename to reindex (if only perorm on one)", :type => :string
	end
	opts = opts.delete_if { |_, v| v == false }
	EvmDatabaseOps.gc(opts)
	exit # exit so that parameters to the first rake task are not run as rake tasks
	end

If it isn't, I am entirely fine dropping this entirely, I just was working under the assumption that this task might be called outside of just the UI potentially. If not, we can axe it as well.

[Fryguy]

Searching in github and locally I don't see rake with gc called anywhere

ag-miq "rake.+gc"

[NickLaMuro]

@Fryguy I was more assuming rake evm:db:gc was used by support and recommended when working with customers/end users, not called by anything directly.

[Fryguy]

I don't recall them running it, personally. Usually it was straight vacuumdb calls if really needed.

[NickLaMuro]

Fair enough. I can axe that method then as well. Makes the code change easier. 👍

[Fryguy]

Merged ManageIQ/manageiq-appliance_console#172

[miq-bot]

Checked commit NickLaMuro@4d3d218 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[kbrock]

Now that this is removed from appliance console, I think these can be removed from here, even before the ui PR is merged.
Yes, the ui won't do much good but c'est la vie.

food for future PRs:

Many moons ago, we needed manageiq to create the REGION file first.
Going forward, we can probably set the region when creating the database and delete this set region migration rake task.

running REGION=x rake db:create db:migrate is what actually sets the region.
The file gook is currently only needed because we need to remember the region between setting the region and creating the database.

As you probably already know, it is the first migration that creates miq_databases that sets the region and not the create database call.

[NickLaMuro]

Now that this is removed from appliance console, I think these can be removed from here, even before the ui PR is merged.
Yes, the ui won't do much good but c'est la vie.

While yes, I would like to finally have this checkbox marked off on my TODO, I am also in no rush and I don't think anyone else is. I would rather no merge things where errors could crop up that need to at least be addressed by explaining "why".

We can wait until the UI is resolved, then merge. Won't take much effort on my end once a decision is made, but I am not going to pressure for a review on something that really isn't a big concern right now.