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

Move core dump setup into manage_operating_system #600

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Move core dump setup into manage_operating_system #600

merged 1 commit into from
Oct 16, 2023

Conversation

mw2q
Copy link
Contributor

@mw2q mw2q commented Oct 10, 2023

  • Move handling of installing helper packages to install debug symbols into manage_operating_system role; running the tools to install the debug symbols stay near respective package installation tasks
  • Set core size unlimited for all users, not just the database owner
  • Set core size limit in limits.conf, not in systemd
  • Do not adjust OS default setuid for potential core dumps
  • Stop overriding core dump locations, which confuses distro specific helper tools

Note that core dump handling is currently only handled for RHEL based systems and EPAS. These changes rearrange the functionality that is currently available.

@mw2q mw2q requested a review from hannahms October 10, 2023 19:58
@hannahms
Copy link
Collaborator

Where do the operations for the following items fall?

  • Set core size unlimited for all users, not just the database owner
  • Set core size limit in limits.conf, not in systemd
  • Do not adjust OS default setuid for potential core dumps
  • Stop overriding core dump locations, which confuses distro specific helper tools

I know for the last item, not overriding the location essentially means not defining our own core dump directory, as we have been doing. However, I don't see any updates to the tasks within the manage_operating_system role?

edb-as{{ pg_version }}-server-contrib
edb-as{{ pg_version }}-server-libs
edb-as{{ pg_version }}-server-client
when: enable_core_dump | bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

all good for removing the enable_core_dump var from defaults, but since it won't be defined there this should be

when: 
  - enable_core_dump is defined
  - enable_core_dump|bool

to ensure that it runs smoothly without enabling :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think I fixed this...

@@ -70,3 +70,15 @@
state: present
become: true
when: pg_ssl

Copy link
Collaborator

Choose a reason for hiding this comment

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

The yum-utils package still needs to be installed in order for the debuginfo-install command to work in the following task, here is what was present previously, with the same conditionals as the following task requires

- name: Install debuginfo helper packages
  ansible.builtin.package:
    name: yum-utils
    state: present
  become: true
  when: enable_core_dump is defined and enable_core_dump|bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, accidentally left out, but amended and push commit to include. roles/manage_operating_system/tasks/enable_core_dump.yml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if the yum-utils is installed in the manage_operating_system tasks, should the debuginfo packages also be installed in the same place? I don't see the need to break it up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, with the new roles/manage_operating_system/tasks/enable_core_dump.yml, do you plan on importing these tasks into a benchmark playbook? I think there should still be a way into enable_core_dump.yml through the roles/manage_operating_system/tasks/main.yml

- name: Enable core dumps
  ansible.builtin.include_tasks: enable_core_dump.yml
  when:
    - enable_core_dump | bool

and add the enable_core_dump: false var into the roles/manage_operating_system/defaults/main.yml to keep consistent with enable_user_profiling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the yum-utils is installed in the manage_operating_system tasks, should the debuginfo packages also be installed in the same place? I don't see the need to break it up

I think this might be a chicken and egg issue. We can't know what debug packages to install until we know what packages to install, but we need to have the tool to install debug packages before we try.

At least I think it makes sense to install the tool so that we can install debug packages, and the install them from the respective playbooks when it's known what the packages are.

I'm open to other ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • name: Enable core dumps
    ansible.builtin.include_tasks: enable_core_dump.yml
    when:
    • enable_core_dump | bool

Whoops, thanks for catching that. That's what I intended but I think I lost of changes swapping between system. Amended commit and forced push.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we kept it here but also placed it in the install_dbserver role? Like this in install_dbserver and keep the current yum-utils install in manage_operating_system because if it is already installed, this is not expensive to run and will be skipped?

- name: Install debuginfo helper packages when enable_core_dumps
  ansible.builtin.package:
    name: yum-utils
    state: present
  when:
    - enable_core_dumps is defined
    - enable_core_dumps | bool
  become: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if we kept it here but also placed it in the install_dbserver role? Like this in install_dbserver and keep the current yum-utils install in manage_operating_system because if it is already installed, this is not expensive to run and will be skipped?

Ok, I can put it back. I don't see the harm, I'm not sure if it's useful without the other operating system changes, but we can see how it goes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i understand, but I think including so that nothing fails here if this is run prior to the manage_operating_system role is useful

@mw2q
Copy link
Contributor Author

mw2q commented Oct 16, 2023

Where do the operations for the following items fall?

  • Set core size unlimited for all users, not just the database owner
  • Set core size limit in limits.conf, not in systemd

Oops, I left out roles/manage_operating_system/tasks/enable_core_dump.yml, amended and pushed. This affect the first 2 bullets.

  • Do not adjust OS default setuid for potential core dumps

This was removed here: https://github.com/EnterpriseDB/edb-ansible/pull/600/files#diff-385a6f45153d7ee74e427aacefdcc208ce959f822314c84d440ca3ab025514d3L36

  • Stop overriding core dump locations, which confuses distro specific helper tools

I know for the last item, not overriding the location essentially means not defining our own core dump directory, as we have been doing. However, I don't see any updates to the tasks within the manage_operating_system role?

Right, the issue is because different distros manage this differently. They may use different locations from each other. On top of that, different distros have their own distro specific tools for managing/analyzing/etc. The couple that I've looked at do not like it (i.e. can't handle) a different location than their own defaults. So we have to be aware of what that location is for whatever distro we're using.

So this role is purely about setting things up. For the time being, the collection and analysis of core dumps is in edb-benchmarks as part of its data collection playbooks.

@mw2q
Copy link
Contributor Author

mw2q commented Oct 16, 2023

Force pushed update. Hopefully got everything this time...

@mw2q mw2q requested a review from hannahms October 16, 2023 17:00
@@ -6,7 +6,6 @@ pg_version: 14
pg_tuner_version: 1
pg_owner: "{{ 'enterprisedb' if pg_type == 'EPAS' else 'postgres' }}"
enable_core_dump: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this intended to be left in place? I noticed they were removed in other roles' defaults

Copy link
Contributor Author

@mw2q mw2q Oct 16, 2023

Choose a reason for hiding this comment

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

was this intended to be left in place? I noticed they were removed in other roles' defaults

Yeah, I thought I should leave enable_core_dump in there because it was used in roles/install_dbserver/tasks/EPAS_RedHat_install.yml. Should I handle that differently?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, i think I initially thought this wasn't present, which is why I asked you to include the condition when: enable_core_dump is defined, but I see this now. I don't think keeping the conditionals with the is defined is an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll re-edit that is defined. ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can leave as is if you would like to edit less files :)

@mw2q mw2q requested a review from hannahms October 16, 2023 20:04
Copy link
Collaborator

@hannahms hannahms left a comment

Choose a reason for hiding this comment

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

Add variable enable_core_dump: false to variable defaults roles/manage_operating_system/defaults/main.yml

@@ -70,3 +70,15 @@
state: present
become: true
when: pg_ssl

Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we kept it here but also placed it in the install_dbserver role? Like this in install_dbserver and keep the current yum-utils install in manage_operating_system because if it is already installed, this is not expensive to run and will be skipped?

- name: Install debuginfo helper packages when enable_core_dumps
  ansible.builtin.package:
    name: yum-utils
    state: present
  when:
    - enable_core_dumps is defined
    - enable_core_dumps | bool
  become: true

* Move handling of installing helper packages to install debug symbols
  into manage_operating_system role; running the tools to install the
  debug symbols stay near respective package installation tasks
* Set core size unlimited for all users, not just the database owner
* Set core size limit in limits.conf, not in systemd
* Do not adjust OS default setuid for potential core dumps
* Stop overriding core dump locations, which confuses distro specific
  helper tools

Note that core dump handling is currently only handled for RHEL based
systems and EPAS.  These changes rearrange the functionality that is
currently available.
@mw2q
Copy link
Contributor Author

mw2q commented Oct 16, 2023

Make another pass and forced push.

@mw2q
Copy link
Contributor Author

mw2q commented Oct 16, 2023

Add variable enable_core_dump: false to variable defaults roles/manage_operating_system/defaults/main.yml

I think i missed this earlier, but got it now.

@mw2q mw2q requested a review from hannahms October 16, 2023 22:43
Copy link
Collaborator

@hannahms hannahms left a comment

Choose a reason for hiding this comment

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

LGTM

@hannahms hannahms merged commit aa39e53 into EnterpriseDB:main Oct 16, 2023
16 checks passed
@mw2q mw2q deleted the os branch October 16, 2023 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants