Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Problem: Pulp 2 stops working after installing Pulp 3 #128

Merged
merged 1 commit into from Aug 6, 2019

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Jul 17, 2019

Solution: make use of the 'pulp' group when setting permissions

Pulp 2.20 changed ownership of /var/lib/pulp and /etc/pulp from group 'apache' to group
'pulp'. This change to the installer ensures that the permissions on these directories
remain the same after the installer runs on a machine that already has Pulp 2 installed.

This patch ensures that the 'pulp_user' is added to the 'pulp' group. The connection is
reset after this to support installations where the ssh user being used for ansible is the
same as the pulp_user. This allows the user changes to affect.

closes: #5104
https://pulp.plan.io/issues/5104

Copy link

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

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

What about adding a PULP_GROUP instead of hardcoding group: pulp?

@dkliban
Copy link
Member Author

dkliban commented Jul 17, 2019

What about adding a PULP_GROUP instead of hardcoding group: pulp?

We could introduce a PULP_GROUP and have it default to 'pulp'. We introduced a group called 'pulp' in Pulp 2.20[0]. These changes are intended to allow installing Pulp 3 on the same machine as Pulp 2. I am interested in hearing other suggestions

[0] pulp/pulp-packaging#98

roles/pulp-database/tasks/main.yml Outdated Show resolved Hide resolved
@dkliban
Copy link
Member Author

dkliban commented Jul 18, 2019

@ekohl @sbernhard I updated the PR with your great suggestion.

@@ -41,6 +41,12 @@
check_mode: False
register: result

- name: Make sure {{ pulp_group }} group exists
group:
name: pulp
Copy link

Choose a reason for hiding this comment

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

Missed one:

Suggested change
name: pulp
name: '{{ pulp_group }}'

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@@ -41,6 +41,12 @@
check_mode: False
register: result

- name: Make sure {{ pulp_group }} group exists
Copy link

Choose a reason for hiding this comment

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

I don't if Ansible properly interpolates here or if you need to quote. Still a bit of magic to me.

@@ -49,20 +55,30 @@
system: true
when: developer_user is not defined

- name: Add user {{ pulp_user }} to 'pulp' group
Copy link

Choose a reason for hiding this comment

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

Haven't looked at the details, but would this still happen if developer_user is set? Should it add the developer user to the pulp group?

Copy link
Member Author

Choose a reason for hiding this comment

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

We usually set 'developer_user' to the same thing as 'pulp_user'. And that value is usually 'vagrant'.

Copy link

Choose a reason for hiding this comment

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

'Usually' sounds like it'd be safer to also do it for developer_user. Ansible should be idempotent so it shouldn't hurt when they do match.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed again with this change.

Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall I think it looks correct.

append: true
when: developer_user is defined

- name: Reset ssh conn to allow user changes to affect when ssh user and pulp user are the same
Copy link

Choose a reason for hiding this comment

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

Should this have a when to detect this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that, but got a warning saying that it is not allowed with the 'reset_connection' metatask.

@@ -8,6 +8,9 @@ pulp_install_dir: '/usr/local/lib/pulp'
pulp_install_plugins: {}
pulp_install_api_service: true
pulp_user: pulp
pulp_user_id:
pulp_group: pulp
pulp_group_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add these to the role README.

Also, what is the behavior when blank like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior is that the operating system assigns the UID. However, if it is provided, that number will be used. unless it's taken and tehn an error will occur.

Solution: make use of the 'pulp' group when setting permissions

Pulp 2.20 changed ownership of /var/lib/pulp and /etc/pulp from group 'apache' to group
'pulp'. This change to the installer ensures that the permissions on these directories
remain the same after the installer runs on a machine that already has Pulp 2 installed.

This patch introduces 3 new pulp settings called 'pulp_group', 'pulp_group_id', and
'pulp_user_id'. The 'pulp_group' defaults to 'pulp'. The 'pulp_user' is added to the
'pulp_ group' group. The connection is reset after this to support installations where
the ssh user being used for ansible is the same as the pulp_user. This allows the user
changes to take affect. The defaults for 'pulp_user_id' and 'pulp_group_id' are empty.
When specified, the installer will use values to set 'uid' and 'gid' respectively.

closes: #5104
https://pulp.plan.io/issues/5104
@dkliban dkliban merged commit de51fc4 into pulp:master Aug 6, 2019
@dkliban dkliban deleted the add-pulp-group branch August 6, 2019 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants