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

Add warning on user/group ownership during pool import (#376) #500

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

Hooverdan96
Copy link
Member

Fixes #376

add note to check ownership of shares post pool import

Checklist

  • With the proposed changes no Sphinx errors or warnings are generated.
  • I have added my name to the AUTHORS file, if required (descending alphabetical order).

@Hooverdan96
Copy link
Member Author

Take a look at the proposal to highlight the necessity to check that the ownership is still aligned after the import. Do we need other cross-references for this comment?

Sphinx link check errors related to usual culprits:

(interface/docker-based-rock-ons/plex-media-server: line   54) broken    https://ark.intel.com/ - 403 Client Error: Forbidden for url: https://ark.intel.com/
(interface/system/update_channels: line   16) broken    https://opensource.org/license/mit-0 - 403 Client Error: Forbidden for url: https://opensource.org/license/mit-0
(interface/system/update_channels: line   16) broken    https://opensource.org/license/apache-2-0 - 403 Client Error: Forbidden for url: https://opensource.org/license/apache-2-0

@phillxnet
Copy link
Member

Re:

Do we need other cross-references for this comment?

How about also linking the V3 to V4 migration doc entry we already have, as per #376 (comment). I.e. openSUSE by default puts new users in group 100 (users) where-as RedHat did not by-default use a shared 'users' group: but had each user in their own group by the same (user) name.

'Users and default group' https://rockstor.com/docs/howtos/v3_to_v4.html#users-and-default-group

@phillxnet
Copy link
Member

Regarding those dodgy/touchy links: how about we add them into our ignored list, i.e. we have this at the beginning of our link-check:

( data_loss: line 184) -ignored- https://web.libera.chat/#btrfs

@phillxnet
Copy link
Member

phillxnet commented Oct 22, 2024

@Hooverdan96 this is where we added that ignored url in the config:

rockstor-doc/conf.py

Lines 266 to 269 in 28b89dc

# -- Options for linkcheck ------------------------------------------------
linkcheck_retries = 2
linkcheck_timeout = 20
linkcheck_ignore = [r'https://web.libera.chat/#btrfs']

See @FroggyFlox 's comment when this came up before, and the following comment in the PR where this ignore was added. There is an upstream doc link there: reproduced below:

We have the following doc entry regarding ignoring problematic links via a pattern:

https://www.sphinx-doc.org/en/master/usage/configuration.html#options-for-the-linkcheck-builder

@Hooverdan96
Copy link
Member Author

I added the additional reference (only way I knew how to), and I think I also now got the linkcheck_ignore correct!

@phillxnet
Copy link
Member

@Hooverdan96 Re:

I also now got the linkcheck_ignore correct!

It does look to have taken: and if we add more https://opensource.org/license/* links we can move to using the wildcard match capability there.

(       data_loss: line  184) -ignored- https://web.libera.chat/#btrfs
(interface/docker-based-rock-ons/plex-media-server: line   54) -ignored- https://ark.intel.com/
(interface/system/update_channels: line   16) -ignored- https://opensource.org/license/mit-0
(interface/system/update_channels: line   16) -ignored- https://opensource.org/license/apache-2-0

If we end up needing more https://opensource.org/license/* links (and they fail) we can move to using the wildcard match capability.

Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@Hooverdan96 Thanks for seeing to this. And for sorting our flaky links re the link checker.

This is a nice note/warning, and I agree with its placement. We ideally need to have this resolved in rockstor-core, but that is for another testing phase now and this doc entry servers as a head up. We, I think, just don't do bottom-up but DB only on this front actually. So all looks to be root.root as that is the default. Plus there is not user import yet so extra (>=1000 UID) users as not known to the system. The underlying vol/subvol should still maintain the UID/GID previously assigned. Regardless this is a much-needed pointer and we have a tricky chicken-egg scenario until we properly do bottom-up from the pool re user:group by number if user does not yet exist etc.

@phillxnet phillxnet merged commit 75a58ee into rockstor:master Oct 25, 2024
3 checks passed
@phillxnet
Copy link
Member

PR product PRODUCTION published.

@phillxnet
Copy link
Member

As a follow-up here we may want more explanation in-time. I.e. prove pool imports with say 1111:1111 if previously set. And explain this as an import would then add prior users: helping with the catch 22 of the new addition via the PR of:

... ensure that the respective share user/group ownerships are set back to what they were before they were imported into the Rockstor installation.

But the prior users:groups are to be restored: which would further complicate their actual restore as we then have to try to match UID:GID to user-name:group-name in our restore. I think we try to do this. But if a config-save is applied before any users are created: we have a cleaner system to restore said users:groups. I.e. this addition could complicate things by pushing folks to re-create by-hand all users and groups so that they can then apply those, via the Web-UI, when on-disk, they already exist.

Again chickent-egg scenario as our Web-UI shows DB knowledge in this regard (user:group) not on-disk data (at least from my memory of this capability). So this should be clarified in core as to what exactly we need to do. And of course for that we need SSE (rockstor/rockstor-core#2739) or the like ideally. Oh well: bit by bit. And we are in the throws of updating all that is required to be more responsive directly to system state, and many of our other systems are already system-as-source-of-truth.

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.

add note to check ownership of shares post import
2 participants