Skip to content

Conversation

@kode54
Copy link

@kode54 kode54 commented Feb 22, 2025

User description

It should not be setting permissions on data that lives in the image.

Fixes #716


PR Type

Bug fix


Description

  • Restricts permissions setting to /data directory only

  • Prevents unnecessary chown on /app during container startup

  • Addresses Docker container startup hang issue


Changes walkthrough 📝

Relevant files
Bug fix
run
Restrict chown operation to /data directory only                 

root/etc/s6-overlay/s6-rc.d/init-rdt-client/run

  • Removes /app from the recursive chown command
  • Now only /data has its ownership set to abc:abc
  • Prevents permission changes on image-internal files
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • It should not be setting permissions on data that lives in the image.
    
    Fixes rogerfar#716
    @rogerfar
    Copy link
    Owner

    Is this right though? /app is an internal path where the .NET application lives. It should set the owner so that it's able to read from /data.

    @kode54
    Copy link
    Author

    kode54 commented Feb 23, 2025

    Is this right though? /app is an internal path where the .NET application lives. It should set the owner so that it's able to read from /data.

    /app is in the Docker image. My system is slowly changing the ownership of the files from root:root to abc:abc at a rate of about one file every 10-20 seconds as it alters the overlays.

    @rogerfar rogerfar closed this Apr 13, 2025
    @rogerfar
    Copy link
    Owner

    I'm going to close this as I don't think this is correct. The App folder should have that permission.

    @kode54
    Copy link
    Author

    kode54 commented Apr 13, 2025

    You're right about the need for it. And the slowdown was a problem with the overlayfs when hosting the apps/docker on a spinning rust ZFS volume. Somehow, each file permission change, on the individual level, took several seconds before moving on to the next.

    @qodo-code-review
    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    716 - Fully compliant

    Compliant requirements:

    • Fix Docker container hanging on startup at "Setting permissions" stage
    • Prevent recursive permission setting on the /app folder inside the Docker image
    • Only apply ownership permissions to /data directory

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-code-review
    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add error handling

    Consider adding error handling for the chown command. If the permission change
    fails, the script continues without notification, potentially causing runtime
    issues.

    root/etc/s6-overlay/s6-rc.d/init-rdt-client/run [12-13]

    -chown -R abc:abc \
    -	/data
    +if ! chown -R abc:abc /data; then
    +    echo "Failed to set permissions on /data"
    +    exit 1
    +fi
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: Adding error handling to the chown command improves script robustness by ensuring permission issues are caught early, but this is a standard best practice rather than a critical fix.

    Medium
    • More

    @rogerfar
    Copy link
    Owner

    The more I think about it, I think it makes sense. Why would RDT be wanting to changing all the permissions of existing files?

    /ask "Does this make sense"?

    @rogerfar
    Copy link
    Owner

    @kode54 Why does it matter that it re-set the permission on the /app folder? I was thinking it's a problem that it's setting all permissions on the /data folder?

    @Cucumberrbob
    Copy link
    Collaborator

    chown -R abc:abc \
    /app /data

    Does the svc file not also need changing? My understanding of s6-overlay isn't great

    @mentalblank
    Copy link
    Contributor

    I have some thoughts as to why this is happening, I've shared them here: #716 (comment)

    I'm not sure if the solution I proposed in that comment is ideal, but I've noted a much quicker startup time on my end, along with lower CPU usage and other stats as a result.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Docker container hangs on startup "Setting permissions" "(unhealthy)"

    5 participants