Inject postgres admin into appliance console #153
Conversation
|
Looks good (I assume this is a 1-for-1 transfer with no changes aside from moving it to the lib dir - incidentally, git-filter-repo has a rename option, so it's possible to move the directory in the history and it will appear as if it was in the lib dir all along). The test failures seem legit as it's using a test helper that doesn't exist here. |
@Fryguy yeah, I could look into that, but currently my script didn't support that. However, I might give it a shot since it would be kind nice to have that feature. As mentioned in the description, I did a number of commands post extraction, as much of the reinsert functionality I worked on previous I decided to just punt on because I wasn't sure of a good way to add it with the script. That said, the rename stuff should be one to one, so I don't see that being too hard. Possibly I can also include the
Yeah, I just got the history moved over, and haven't really made changes to make it work in the test suite. Part of what I might work on today. That said, this might be something we can pull the trigger on first here, and then remove it in |
Not saying it's required, I just wasn't sure if you were aware of it based on the "Move to lib" commit. It's up to you. |
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
from
d11a110 to
2b08733
Compare
@Fryguy No, I think it made sense, and turns out it wasn't actually that hard to do, so wasn't really an issue. It actually ended up not being to big of a deal to change, and I even added the :name: inject_postgres_admin
:upstream: "[email protected]:NickLaMuro/manageiq-gems-pending.git"
:upstream_name: "ManageIQ/manageiq-gems-pending"
:extract_name: "postgres_admin"
:extractions:
- lib/gems/pending/util/postgres_admin.rb
- spec/util/postgres_admin_spec.rb
:renames:
- lib/gems/pending/util/postgres_admin.rb:lib/manageiq/appliance_console/postgres_admin.rb
- spec/util/postgres_admin_spec.rb:spec/postgres_admin_spec.rb
:extra_cmds:
- git remote add appliance_console [email protected]:NickLaMuro/manageiq-appliance_console
- git fetch appliance_console
- git checkout -b inject_postgres_admin_into_appliance_console
- git rebase --rebase-merges=rebase-cousins --root --onto appliance_console/master inject_postgres_admin_into_appliance_console
- git push --force -u appliance_console inject_postgres_admin_into_appliance_consoleHowever... the test failures made me realize there was a lot of other things that needed to be pulled over, and how I tackle that in the best way is still unclear. For context, these are all of the other helper files just related to running the specs:
Plus some stuff in the Here are some of the PRs that sort of provide context to what these files are doing: |
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
from
2b08733 to
9e1c595
Compare
|
So this is close, but ommits the Problem is, since there already is a :name: inject_postgres_admin
:upstream: "[email protected]:NickLaMuro/manageiq-gems-pending.git"
:upstream_name: "ManageIQ/manageiq-gems-pending"
:extract_name: "postgres_admin"
:extractions:
# - Rakefile
- lib/gems/pending/util/postgres_admin.rb
- spec/ci_helper.rb
- spec/postgres_runner_helper.rb
- spec/support/contexts/db_restore.rb
- spec/support/mixins/pg_environment_updater.rb
- spec/util/data/creating_pg_dump_and_pg_backup_files.md
- spec/util/data/pg_backup.tar.gz
- spec/util/data/pg_dump.gz
- spec/util/postgres_admin_spec.rb
:renames:
- lib/gems/pending/util/postgres_admin.rb:lib/manageiq/appliance_console/postgres_admin.rb
- spec/util/data/creating_pg_dump_and_pg_backup_files.md:spec/data/creating_pg_dump_and_pg_backup_files.md
- spec/util/data/pg_backup.tar.gz:spec/data/pg_backup.tar.gz
- spec/util/data/pg_dump.gz:spec/data/pg_dump.gz
- spec/util/postgres_admin_spec.rb:spec/postgres_admin_spec.rb
:extra_cmds:
- git remote add appliance_console [email protected]:NickLaMuro/manageiq-appliance_console
- git fetch appliance_console
- git checkout -b inject_postgres_admin_into_appliance_console
- git rebase --rebase-merges=rebase-cousins --root --onto appliance_console/master inject_postgres_admin_into_appliance_console
- git push --force -u appliance_console inject_postgres_admin_into_appliance_consoleHowever, as I was typing this up, a hack for this might be to just rename the |
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
4 times, most recently
from
0596864 to
ac3b06e
Compare
| desc "Run RSpec code examples (with local postgres dependencies)" | ||
| RSpec::Core::RakeTask.new("spec:dev") do |t| | ||
| # Load the PostgresRunner helper to facilitate a clean postgres environment | ||
| # for testing locally (not necessary for CI), and enables the postgres test |
There was a problem hiding this comment.
Even though it's not necessary for CI, this seems to over complicate the code (and making local runs "different"). Can we just run it in CI anyway presuming it's relatively inexpensive?
There was a problem hiding this comment.
So there is a couple of things here, but I did explain a lot of the architecture of this in the original PR here:
ManageIQ/manageiq-gems-pending#385
So I won't fully repeat that here (and I honestly forget a lot of my decisions without using that to cheat), but the main take aways were:
- A travis box is ephemeral, so it is fine it we just nuke the entire DB, but on a local machine, a developers DB(s) for
vmdb_developmentwould be trashed every time this is run locally - When trying to do TDD with this code, having a way to be able to validate you haven't borked anything locally (instead of waiting for Travis) is really nice.
That all said, I also agree that basically adding an "init script written in Ruby" (see spec/postgres_runner_helper.rb ) is basically a lot of code to accomplish this, for like 2 tests...
They are arguably important tests, but depending on how much we do or do not move away from this backup strategy might determine if it is still of value.
P.S. This is also still WIP, so definitely things are still subject to change a little more yet.
|
Overall this looks good. I have one comment there about over complicating the test setup, but that seems to be how it was over in gems pending, so it's not a big deal, and shouldn't stop the transfer. |
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
9 times, most recently
from
79f6a96 to
45cd227
Compare
NickLaMuro
changed the title
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
from
45cd227 to
3681285
Compare
Allows pre-fetching a backup_type when calling .restore, so no extra file type checking is done on the `opt[:local_file]` (which could be a streamed IO where that would break things down the line). This behaves as it had if this option isn't passed. (transferred from ManageIQ/manageiq-gems-pending@0505809)
With the previous implementation, the code would always attempt to find the magic number of the file for a `pg_dump` before checking if the `backup_type` value was instead a `:basebackup`. When streaming a file, this would cause it to fail farther down the line since it would have strip some of the content off from the beginning of the file, invalidating it as a proper backup. This change fixes by forcing the check for the backup type first to avoid reading the file data if possible, and then by inspecting the file contents when as a backup option. (transferred from ManageIQ/manageiq-gems-pending@5b9e017)
This change has a few benefits:
- It removes the dependency for an external gem (minor, but nice)
- It supports streaming files (important)
The first was simply a side affect, though one I admit I am happy with
the result about, but unfortunately was a required change to allow me to
do this without too much custom code.
`Archive::Tar::Minitar.unpack` does not support IO objects that don't
use `rewind`, since it uses `Archive::Tar::Minitar::Input` under the
hood:
https://github.com/halostatue/minitar/blob/41c3ca65/lib/archive/tar/minitar.rb#L249
https://github.com/halostatue/minitar/blob/41c3ca65/lib/archive/tar/minitar/input.rb#L6-L7
The main code that we wanted from the codebase, however, was the
`extract_entry` method from `Input`:
https://github.com/halostatue/minitar/blob/41c3ca65/lib/archive/tar/minitar/input.rb#L117
Which again, requires an `Input` model, and the check for if the `@io`
is "seekable" is done on instantiation:
https://github.com/halostatue/minitar/blob/41c3ca65/lib/archive/tar/minitar/input.rb#L76-L78
`Gem::Package.extract_tar_gz` method, however, only requires
instantiating a instance with a "gem name", which doesn't even have to
be a real gem, and then we have access to the method without worry, and
can pass in our own `IO` object as we see fit.
That said, it isn't ideal, but the alternative would be re-writing one
of these two methods:
https://github.com/halostatue/minitar/blob/41c3ca65/lib/archive/tar/minitar/input.rb#L117-L194
https://github.com/rubygems/rubygems/blob/master/lib/rubygems/package.rb#L381-L418
In which LJ probably wouldn't approve of all of the additions...
* * *
Of note, and a minor "fun fact": The code for `minitar` is based on the
same person's code, Mauricio Julio Fernández Pradier, as is found in
`rubygems`:
https://github.com/halostatue/minitar/blob/41c3ca65/README.rdoc#L37
https://github.com/halostatue/minitar/blob/41c3ca65/Licence.md#L7
https://github.com/rubygems/rubygems/blob/d43e4d01/lib/rubygems/package/tar_reader.rb#L4
(transferred from ManageIQ/manageiq-gems-pending@39ead16)
(transferred from ManageIQ/manageiq-gems-pending@38e2ab9)
(transferred from ManageIQ/manageiq-gems-pending@ab5379c)
It seems that the logic which previously would move the data directory from /var/lib/postgresql/ to /var/ramfs/postgresql/ before starting the database service has been removed. This also removes the logic that clears out that directory before starting the service because that would now delete the restored data (transferred from ManageIQ/manageiq-gems-pending@c6a2eb7)
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
from
6410be4 to
ee7ef88
Compare
NickLaMuro
changed the title
`make_tmpname` was removed in 2.5: ruby/ruby@25d56ea So use the method it was merged into, .create
This is technically already the case, since we don't end up using this file on CI prior to adding (PostgresAdmin to the project), and just end up using `.rspec`, so this trues up this with the main branch now that it is being used properly.
Many specs will make use of `LinuxAdmin::Service`, which is overwritten
in `spec/ci_helper.rb` to make use of `CiPostgresRunner` if the service
in question is `postgres`.
However, since calling the overwritten method requires the
`.service_name` method:
module LinuxAdmin
def Service.new(service_name)
if ManageIQ::ApplianceConsole::PostgresAdmin.service_name == service_name
CiPostgresRunner
else
super
end
end
end
That in turn requires `ENV["APPLIANCE_PG_SERVICE"]` to be set, which it
is not.
The simplest fix for this case is to just make sure the environment
variables are loaded in CI where we need them. Previously, they were
only needed when launching the sub process, but now with other specs
making use of `LinuxAdmin`, it is also required to be set all the time.
There was also a slight readability fix with the `system` call to remove
the temporary variable, to make it more clear what were helper vars, and
what was the command being run.
Since `super` doesn't seem to properly allow the previous functionality, pulling in the real implementation was the option applied here. Assuming that doesn't change, this should be fine, but this will be a spot that will break if the implementation of LinuxAdmin::Service.new were to change.
Pulls in changes from `manageiq-gems-pending`'s `spec_helper.rb` to set `:with_postgres_specs`, which is used trigger running specs that actually run database backups/restores.
The APPLIANCE_PG_DATA and APPLIANCE_PG_SERVICE environment variables are used by both the database_replication_spec.rb and postgres_admin_spec.rb specs. The previous implementation, specs in both database_replication_spec.rb and postgres_admin_spec.rb would delete/overwrite the ENV variables that were assumed to be set for the other to run it's processes. In this case, it is simpler to store the variable in a tmp variable before overwriting it for the purposes of the spec in question, and then reset the var to it's original value when the spec is finished. That way other specs will be able to use it with out issue, regardless of what order the specs are triggered from.
NickLaMuro
force-pushed
the
inject_postgres_admin_into_appliance_console
branch
from
ee7ef88 to
4edbc2f
Compare
|
@Fryguy okay, I think I have this where I want it. I have rebased one more time so that the last 13 commits are the ones making updates to allow this to work, and should be separated by the title change removing Let me know if you have any questions. |
|
Just verifying...I should basically just review NickLaMuro/manageiq-appliance_console@inject_postgres_admin_into_appliance_console~13...NickLaMuro:inject_postgres_admin_into_appliance_console ? |
|
@Fryguy Well, those are just the commits I added as part of this PR that went outside of what was done with the transfer, and I even rebased some of the ones that I did with the But looking at the file changes overall is probably fine as well. I just wanted to make a clear line as to what was transferred from |
Part of a effort to move database backups in ManageIQ to be only an appliance console concern.
Currently, the only places that
PostgresAdminis used is for database backups:https://github.com/search?p=1&q=org%3AManageIQ+PostgresAdmin&type=Code
The code on the
manageiqside only uses it for database backups (mostly inlib/evm_database_ops.rb, and the other place is here. Since this will be the only place using it otherwise, this ports it into frommanageiq-gems-pendinginto this repo, and moves it into a more reasonable location for this repo.Still a work in progress, but arguably could be merged, and the removal bit in
manageiq-gems-pendingcan be saved from later.Side Benefit: This might be the only requirement this repo has for
manageiq-gems-pending, so we might be able to remove that as well. We will see.Process for the extraction
Been working on converting
code-extractorto usegit-filter-repo, and the current implementation of that was using a purepython3implementation:juliancheal/code-extractor@master...NickLaMuro:use-python
The
yamlI used for that was pretty simple:(Updated to now include
extra_cmdsin theextractions.yml)Links
manageiq-gems-pending: Remove PostgresAdmin manageiq-gems-pending#516