Skip to content
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

[WIP] Bind mount the log dir instead of using a symlink #318

Closed
wants to merge 1 commit into from

Conversation

agrare
Copy link
Member

@agrare agrare commented May 14, 2019

Using a symlink to the log directory leads to a dirty git tree, if we
add a log/.keep and also touch /var/log/manageiq/.keep we can bind mount
the /var/log/manageiq directory without dirtying the git status and also
remove the issues with not having a log/ directory in any of the repos
necessitating a bin/setup script to get tests to run.

TODO:

  • Add the bind mount to /etc/fstab so the log dir is mounted on boot

Using a symlink to the log directory leads to a dirty git tree, if we
add a log/.keep and also touch /var/log/manageiq/.keep we can bind mount
the /var/log/manageiq directory without dirtying the git status and also
remove the issues with not having a log/ directory in any of the repos
necessitating a bin/setup script to get tests to run.
@agrare
Copy link
Member Author

agrare commented May 14, 2019

@carbonin
Copy link
Member

Will this cause an issue when we try to mount a new volume on the appliance?

That's the only other place I feel like we might need to consider.
Specifically here https://github.com/ManageIQ/manageiq-appliance_console/blob/master/lib/manageiq/appliance_console/logfile_configuration.rb#L96

@miq-bot miq-bot added the wip label May 14, 2019
@agrare
Copy link
Member Author

agrare commented May 14, 2019

Hm in that case there isn't already a var_log logical volume?

In the upstream appliance build the log volume is precreated

logvol /var/log --name=lv_var_log --vgname=vg_system --size=11264 --fstype=xfs

I'm thinking the way to deal with this generally is to create a systemd mount unit that evmserverd would depend on so that it always gets mounted before starts, that way appliance console can setup the log disk after the system image is created. That way we don't have to modify /etc/fstab at runtime but I'm open to other ideas (including just doing what we do now).

@miq-bot
Copy link
Member

miq-bot commented May 14, 2019

Checked commit agrare@cd0e473 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@carbonin
Copy link
Member

Yeah, if you use the option I think you end up with an unused volume upstream. I don't actually recall how it works downstream ... @simaishi do we also ship with a volume for the logs there or do we expect people to provide something for that?

As for fstab the configuration in the console updates that at runtime here https://github.com/ManageIQ/manageiq-appliance_console/blob/master/lib/manageiq/appliance_console/logical_volume_management.rb#L81

@simaishi
Copy link
Contributor

Yeah, if you use the option I think you end up with an unused volume upstream. I don't actually recall how it works downstream ... @simaishi do we also ship with a volume for the logs there or do we expect people to provide something for that?

Just like upstream, we ship downstream with a volume.

@agrare
Copy link
Member Author

agrare commented May 14, 2019

Should appliance-console be adding the disk to the vg_system volume group then vg_resize && lv_resize then?

@carbonin
Copy link
Member

Probably, but I think that's a separate thing, right @agrare? While it would definitely be an improvement I don't think it will help with the git status thing you're trying to solve, right?

@agrare
Copy link
Member Author

agrare commented May 14, 2019

Oh correct it wouldn't fix the git status issue, it would simplify things in that we wouldn't need to unmount the bind, mount the new log disk, then remount the bind if someone added a disk with appliance_console though

@carbonin
Copy link
Member

Yeah, but I'm not even sure what's going on with that option as it is....
How would the symlink interact with us trying to mount the new volume on /var/www/miq/vmdb/log?

Seems like the whole thing needs to be rethought either way.

@agrare
Copy link
Member Author

agrare commented May 14, 2019

😱 I think it would mount it on top of what log is symlinked to (/var/log/manageiq)

@agrare
Copy link
Member Author

agrare commented May 14, 2019

Yup that's what it does, so you'd have something like

/dev/mapper/vg_system-lv_var_log on /var/log
/dev/mapper/vg_miq_logs-lv_miq_logs on /var/log/manageiq through /var/www/miq/vmdb/log

@carbonin
Copy link
Member

Well at least we're not throwing away the rest of the space in the filesystem created for /var/log but still ... ew.

@agrare
Copy link
Member Author

agrare commented May 14, 2019

Yeah I don't think that was intentional, also it would mount over any existing logs that were there from before creating that disk so they wouldn't be visible nor would they ever be rotated/purged

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

thanks for addressing this

@@ -20,8 +20,8 @@ ln -vs $vmdb_root $manageiq_root

# Replace the log directory with the larger logvol
mkdir -p /var/log/manageiq
rm -rf $vmdb_root/log
ln -vs /var/log/manageiq $vmdb_root/log
touch /var/log/manageiq/.keep
Copy link
Member

Choose a reason for hiding this comment

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

I like the mount much better. can we drop the touch though?

Copy link
Member

@kbrock kbrock Jun 6, 2019

Choose a reason for hiding this comment

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

huh - looks like .keep is back in the repo.

The current PR works for me. :shipit:

@miq-bot miq-bot closed this Dec 9, 2019
@miq-bot
Copy link
Member

miq-bot commented Dec 9, 2019

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants