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

feat(config): allow customisation of runtime group name (#440) #440

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

mdonadoni
Copy link
Member

@mdonadoni mdonadoni changed the title feat(config): allow customisation of runtime group name (#431) feat(config): allow customisation of runtime group name (#440) Feb 2, 2024
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (36ce4e0) 36.35% compared to head (5cec305) 36.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #440      +/-   ##
==========================================
- Coverage   36.35%   36.33%   -0.03%     
==========================================
  Files          26       26              
  Lines        1576     1577       +1     
==========================================
  Hits          573      573              
- Misses       1003     1004       +1     
Files Coverage Δ
reana_commons/config.py 0.00% <0.00%> (ø)

@@ -303,6 +303,9 @@ def kubernetes_node_label_to_dict(node_label):
WORKFLOW_RUNTIME_USER_NAME = os.getenv("WORKFLOW_RUNTIME_USER_NAME", "reana")
"""Default OS user name for running job controller."""

WORKFLOW_RUNTIME_GROUP_NAME = os.getenv("WORKFLOW_RUNTIME_GROUP_NAME", "reana")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder whether we need to create a new group name here at all. We do need to use the GID=0 during runtime for the appropriate workspace file access permissions, but cannot we simply use the default root group name directly without introducing a new "reana" group?

$ docker run -i -t --rm docker.io/library/ubuntu:20.04 /bin/bash -c 'grep :0: /etc/group'
root:x:0:

That said, it seems nice and consistent to introduce the new variable WORKFLOW_RUNTIME_GROUP_NAME instead of using hard-coded "root". But perhaps its default value could be simply "root" and not "reana"? And perhaps we might not actually need to create any such a group and just use the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, creating a new group is not needed if we keep WORKFLOW_RUNTIME_USER_GID set to zero. However that is needed if the GID is not zero, as otherwise the useradd call fails.

We can check whether the group already exists or not, and if it doesn't then groupadd is called. What do you think?

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 have updated reana-workflow-controller's PR to only create the group if there is no group with the given GID

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. We may perhaps want to set WORKFLOW_RUNTIME_GROUP_NAME to be root by default, so that its value would correspond with WORKFLOW_RUNTIME_USER_GID's default value being 0.

Copy link
Member Author

@mdonadoni mdonadoni Feb 13, 2024

Choose a reason for hiding this comment

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

Regarding the default name, I would keep reana as I see two scenarios:

  • gid is zero, so normally the root group already exists -> nothing to do
  • gid is not zero, so we create a new group reana, as we cannot name this root

What do you think?

Copy link
Member Author

@mdonadoni mdonadoni Feb 13, 2024

Choose a reason for hiding this comment

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

After discussing among us, I have changed reana to root and I have also added a comment explaining what to do in case the group id needs to be changed.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's nice to have default values expressing the "reality", hence 0=root. And if someone would like to change them, they will find a little "how to" note on what to do.

Copy link
Member

@tiborsimko tiborsimko left a comment

Choose a reason for hiding this comment

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

Works nicely. Left a minor comment about the default value of WORKFLOW_RUNTIME_GROUP_NAME.

@mdonadoni mdonadoni merged commit 5cec305 into reanahub:master Feb 13, 2024
17 checks passed
@mdonadoni mdonadoni deleted the fix-groupadd branch February 20, 2024 09:30
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.

invalid groupadd command when running job-controller
2 participants