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

Introduce JUICEFS_IMMUTABLE, mount /etc/updatedb.conf only in mutable environments #680

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jun 28, 2023

Some Kubernetes distributions, such as Talos Linux, do not ship updatedb at all. In these environments, the JuiceFS CSI mount pod fails to start due to /etc/updatedb.conf not being present on the host for bind mounting, nor can FileOrCreate create it due to /etc being read-only. This patch introduces JUICEFS_IMMUTABLE as a new environment variable for both controller and node instances to handle these kinds of cases that need special attention in immutable environments. I've also updated updatedb-related hard-coded strings to use the constant definitions in common.go.

@zwwhdls
Copy link
Member

zwwhdls commented Jun 29, 2023

Hi @twelho , CSI Node does not mount /etc/updatedb.conf inside, so CSI Node pod can not stat it inside container.

We alreay add FileOrCreate in volume.hostPath, I think it doesn't matter.

@twelho
Copy link
Contributor Author

twelho commented Jul 5, 2023

Hmm, you're right, os.Stat inside the container won't work. However, since the filesystem in Talos is immutable, /etc is read-only, which means that FileOrCreate will not be able to create the file, leading to the failure I'm observing. Would it be possible to either detect this somehow, or expose an option to ignore updatedb.conf entirely? As a sanity check, I will try to patch the CSI driver locally to temporarily skip the updatedb.conf mount for now to check if that is all that is required to get it working.

@zwwhdls
Copy link
Member

zwwhdls commented Jul 7, 2023

Hi @twelho , you can set an environment variable to control not to mount updatedb.conf into mount pod or any job.

@twelho
Copy link
Contributor Author

twelho commented Jul 12, 2023

Sorry for the delay, but I can now confirm that JuiceFS works on Talos when commenting out the updatedb.conf mount, so that is the only blocker for immutable environments.

Hi @twelho , you can set an environment variable to control not to mount updatedb.conf into mount pod or any job.

To my knowledge Kubernetes doesn't provide this kind of generic control through an environment variable, or are you suggesting that I should add an environment variable to the CSI driver to control this? I can refactor this PR do that if that's what you intend.

@zwwhdls
Copy link
Member

zwwhdls commented Jul 13, 2023

@twelho , I mean to add an ENV in juicefs csi node&controller to control if to mount updatedb.conf in mount pod when creating

@twelho
Copy link
Contributor Author

twelho commented Jul 14, 2023

I've now added JUICEFS_MODIFY_UPDATEDB for controlling whether to mount updatedb.cfg or not. If not specified, it is enabled by default to keep the existing behavior. Right now I've only added it to the nodes and not the controllers, since only the nodes are responsible for spawning the mount pods. I've successfully tested this on a Talos Linux cluster by setting JUICEFS_MODIFY_UPDATEDB=0 for the juicefs-csi-node DaemonSet (I'm using the Helm chart as a base).

@twelho
Copy link
Contributor Author

twelho commented Jul 14, 2023

Now that I think about it, it might make sense to rename the variable to JUICEFS_IMMUTABLE, which could also cover any future aspects that JuiceFS might need to take into account in immutable environments if, for example, there should be another configuration file that needs to be modified on traditional, mutable systems, but which is restricted in immutable environments. Does this sound better? If we go with JUICEFS_IMMUTABLE, I will add it to both the CSI controllers and nodes.

@zwwhdls
Copy link
Member

zwwhdls commented Jul 19, 2023

@twelho sure, JUICEFS_IMMUTABLE is ok.

…able environments

In immutable environments, such as Talos Linux, most host directories are read-only.
As a result, for example, `FileOrCreate` for the `/etc/updatedb.conf` will fail to
create an empty file. This patch introduces `JUICEFS_IMMUTABLE` as a new environment
variable for both controller and node instances to handle these kinds of cases that
need special attention in immutable environments.
@twelho twelho changed the title Only mount /etc/updatedb.conf from the host if it exists Introduce JUICEFS_IMMUTABLE, mount /etc/updatedb.conf only in mutable environments Jul 19, 2023
@twelho twelho marked this pull request as ready for review July 19, 2023 19:21
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.40 ⚠️

Comparison is base (674ac83) 38.92% compared to head (03c6b11) 38.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
- Coverage   38.92%   38.52%   -0.40%     
==========================================
  Files          33       33              
  Lines        3895     3943      +48     
==========================================
+ Hits         1516     1519       +3     
- Misses       2233     2278      +45     
  Partials      146      146              
Impacted Files Coverage Δ
pkg/config/config.go 0.00% <ø> (ø)
pkg/juicefs/mount/builder/common.go 47.26% <100.00%> (+1.07%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zwwhdls zwwhdls merged commit 1c79901 into juicedata:master Jul 20, 2023
23 checks passed
@twelho twelho deleted the fix-updatedb-mount branch July 21, 2023 11:37
twelho added a commit to twelho/juicedata-charts that referenced this pull request Jul 21, 2023
twelho added a commit to twelho/juicedata-charts that referenced this pull request Jul 21, 2023
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.

3 participants