internal database must be set with dbdisk option in ap and in cli #277

ailisp · 2017-09-29T22:40:45Z

ISSUE:
When setting internal database in appliance console cli, the user can set without given a --dbdisk option, which is not supported.
BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1431815
https://bugzilla.redhat.com/show_bug.cgi?id=1425153

\cc @yrudman @gtanzillo

@miq-bot add-label wip,bug

ailisp · 2017-09-30T00:34:40Z

@miq-bot remove-label wip

yrudman · 2017-10-02T16:13:47Z

@ailisp there is another check requested in the same BZ- do not allow blank password

carbonin · 2017-10-02T18:37:13Z

I believe this will break ManageIQ appliance builds.

See https://github.com/ManageIQ/manageiq-appliance-build/blob/master/kickstarts/partials/post/db_init.ks.erb#L7

I think we should come up with a way to allow this behavior through the CLI.

For example if someone configures a logical volume and wants to put the database there (as we do on the ManageIQ appliance), we should allow them to do that.

As I see it, the real issue is that we want the database on a separate filesystem not necessarily a separate disk. Unfortunately, we don't do any validation against filesystems so checking that case would be much more difficult.

carbonin · 2017-10-02T18:37:47Z

there is another check requested in the same BZ- do not allow blank password

@yrudman I think it would be better to do this one in a separate PR.

yrudman · 2017-10-02T20:52:17Z

@carbonin yes, it is better to do password present validation in different PR

ailisp · 2017-10-04T13:00:33Z

As I see it, the real issue is that we want the database on a separate filesystem not necessarily a separate disk. Unfortunately, we don't do any validation against filesystems so checking that case would be much more difficult.

@carbonin do you mean the device number of filesystem? In this question:
https://unix.stackexchange.com/questions/44249/how-to-check-if-two-directories-or-files-belong-to-same-filesystem
I did some try:

[boyao@localhost ~]$ df -h
Filesystem               Size  Used Avail Use% Mounted on
devtmpfs                 5.8G     0  5.8G   0% /dev
tmpfs                    5.8G  988K  5.8G   1% /dev/shm
tmpfs                    5.8G  1.6M  5.8G   1% /run
tmpfs                    5.8G     0  5.8G   0% /sys/fs/cgroup
/dev/mapper/fedora-root   50G  6.8G   40G  15% /
tmpfs                    5.8G   88K  5.8G   1% /tmp
/dev/sda1                477M  149M  299M  34% /boot
/dev/mapper/fedora-home  180G   26G  145G  16% /home
tmpfs                    1.2G   24K  1.2G   1% /run/user/42
tmpfs                    1.2G   32K  1.2G   1% /run/user/1000
[boyao@localhost ~]$ stat -c "%d" /
64769
[boyao@localhost ~]$ stat -c "%d" /var
64769
[boyao@localhost ~]$ stat -c "%d" /dev
6

ailisp · 2017-10-04T13:40:18Z

@yrudman Thanks for the note. Seems I should wait this PR merged then open another PR for checking password. otherwise some conflict will happen in prompts.rb

carbonin · 2017-10-04T14:12:14Z

@ailisp Generally, yes, but I don't think the place for that is in the cli class.

If we want a more complete fix, we could edit the InternalDatabaseConfiguration class so that we have an option to verify that the data directory is on a separate filesystem, alternatively you can check that APPLIANCE_PG_MOUNT_POINT is indeed a mount point.

yrudman · 2017-10-04T14:15:20Z

@ailisp you can create tracking branch:
git checkout -b feature_2 --track feature_1

ailisp · 2017-10-04T19:11:22Z

@miq-bot add-label wip

ailisp · 2017-10-04T19:14:26Z

@carbonin Yuri and I think this #276 can be closed now, because your suggestion to check APPLIANCE_PG_MOUNT_POINT should solve both BZs. What do you think?
Also, should we check database_replication_standby's mount point in the same way?

carbonin · 2017-10-04T20:05:56Z

lib/gems/pending/appliance_console/internal_database_configuration.rb

@@ -121,6 +123,11 @@ def copy_template(src, src_dir = self.class.postgresql_template, dest_dir = Post
      end
    end

+    def check_pg_mount_point
+      is_mount_point = Class.new.include(LinuxAdmin::Mountable).mount_point_exists?(mount_point)


Can you not just use LinuxAdmin::LogicalVolume here?

carbonin · 2017-10-04T20:10:53Z

lib/gems/pending/appliance_console/internal_database_configuration.rb

@@ -82,6 +83,7 @@ def initialize_postgresql_disk

    def initialize_postgresql
      log_and_feedback(__method__) do
+        check_pg_mount_point


I think this would make more sense as a check in activate or even in ask_questions.

Right after choose disk we could print some error message unless disk || have_pg_mount_point?

carbonin · 2017-10-04T20:11:50Z

@ailisp I think we can close the other PR. I would like to do the checking earlier though.

https://bugzilla.redhat.com/show_bug.cgi?id=1431815 https://bugzilla.redhat.com/show_bug.cgi?id=1425153

miq-bot · 2017-10-06T18:58:48Z

Checked commits ailisp/manageiq-gems-pending@7ae42c0~...99937e2 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 2 offenses detected

lib/gems/pending/appliance_console/cli.rb

❗ - Line 217, Col 7 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 218, Col 7 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

ailisp · 2017-10-06T19:01:30Z

@miq-bot remove-label wip

ailisp · 2017-10-06T19:02:15Z

@yrudman @carbonin
works fine in both appliance console and cli now

miq-bot added bug wip labels Sep 29, 2017

ailisp force-pushed the cli-internal-db-disk branch 2 times, most recently from ce2c751 to 949164c Compare September 30, 2017 00:22

miq-bot changed the title ~~[WIP] internal database must be set with dbdisk option in cli~~ internal database must be set with dbdisk option in cli Sep 30, 2017

miq-bot removed the wip label Sep 30, 2017

ailisp closed this Oct 4, 2017

ailisp force-pushed the cli-internal-db-disk branch from 949164c to 795d14e Compare October 4, 2017 15:28

ailisp reopened this Oct 4, 2017

miq-bot changed the title ~~internal database must be set with dbdisk option in cli~~ [WIP] internal database must be set with dbdisk option in cli Oct 4, 2017

miq-bot added the wip label Oct 4, 2017

carbonin reviewed Oct 4, 2017

View reviewed changes

ailisp mentioned this pull request Oct 4, 2017

must give password to set db in cli #281

Merged

ailisp changed the title ~~[WIP] internal database must be set with dbdisk option in cli~~ [WIP] internal database must be set with dbdisk option in ap and in cli Oct 5, 2017

ailisp force-pushed the cli-internal-db-disk branch 3 times, most recently from eb88bda to 84b62a7 Compare October 6, 2017 13:28

check and only allow to create database in a mount point

7ae42c0

https://bugzilla.redhat.com/show_bug.cgi?id=1431815 https://bugzilla.redhat.com/show_bug.cgi?id=1425153

ailisp force-pushed the cli-internal-db-disk branch from b0f98c9 to 5c590a8 Compare October 6, 2017 15:35

ailisp added 3 commits October 6, 2017 14:56

check mount point when configure database in cli

3292eb2

cli spec for check mount point

fc8f3dc

new spec for check_disk_is_mount_point

99937e2

ailisp force-pushed the cli-internal-db-disk branch from ee52e5a to 99937e2 Compare October 6, 2017 18:57

miq-bot changed the title ~~[WIP] internal database must be set with dbdisk option in ap and in cli~~ internal database must be set with dbdisk option in ap and in cli Oct 6, 2017

miq-bot removed the wip label Oct 6, 2017

carbonin approved these changes Oct 13, 2017

View reviewed changes

carbonin self-assigned this Oct 13, 2017

carbonin added the appliance/console label Oct 13, 2017

carbonin merged commit 367b50d into ManageIQ:master Oct 13, 2017

carbonin added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 13, 2017

ailisp mentioned this pull request Oct 14, 2017

Force to use different partition for db #276

Closed

carbonin mentioned this pull request Oct 16, 2017

Pass a string to mount_point_exists? method #286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal database must be set with dbdisk option in ap and in cli #277

internal database must be set with dbdisk option in ap and in cli #277

ailisp commented Sep 29, 2017 •

edited

Loading

ailisp commented Sep 30, 2017

yrudman commented Oct 2, 2017

carbonin commented Oct 2, 2017

carbonin commented Oct 2, 2017

yrudman commented Oct 2, 2017

ailisp commented Oct 4, 2017 •

edited

Loading

ailisp commented Oct 4, 2017 •

edited

Loading

carbonin commented Oct 4, 2017

yrudman commented Oct 4, 2017

ailisp commented Oct 4, 2017

ailisp commented Oct 4, 2017 •

edited

Loading

carbonin Oct 4, 2017

carbonin Oct 4, 2017

carbonin commented Oct 4, 2017

miq-bot commented Oct 6, 2017

ailisp commented Oct 6, 2017

ailisp commented Oct 6, 2017

internal database must be set with dbdisk option in ap and in cli #277

internal database must be set with dbdisk option in ap and in cli #277

Conversation

ailisp commented Sep 29, 2017 • edited Loading

ailisp commented Sep 30, 2017

yrudman commented Oct 2, 2017

carbonin commented Oct 2, 2017

carbonin commented Oct 2, 2017

yrudman commented Oct 2, 2017

ailisp commented Oct 4, 2017 • edited Loading

ailisp commented Oct 4, 2017 • edited Loading

carbonin commented Oct 4, 2017

yrudman commented Oct 4, 2017

ailisp commented Oct 4, 2017

ailisp commented Oct 4, 2017 • edited Loading

carbonin Oct 4, 2017

Choose a reason for hiding this comment

carbonin Oct 4, 2017

Choose a reason for hiding this comment

carbonin commented Oct 4, 2017

miq-bot commented Oct 6, 2017

ailisp commented Oct 6, 2017

ailisp commented Oct 6, 2017

ailisp commented Sep 29, 2017 •

edited

Loading

ailisp commented Oct 4, 2017 •

edited

Loading

ailisp commented Oct 4, 2017 •

edited

Loading

ailisp commented Oct 4, 2017 •

edited

Loading