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

ReloadOnChange of AddJsonFile is invalid in kubernetes. #36091

Closed
expcat opened this issue Feb 26, 2019 · 47 comments · Fixed by #55664
Closed

ReloadOnChange of AddJsonFile is invalid in kubernetes. #36091

expcat opened this issue Feb 26, 2019 · 47 comments · Fixed by #55664
Assignees
Labels
area-System.IO bug Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@expcat
Copy link

expcat commented Feb 26, 2019

AB#1282969

Describe the bug

I tried to bind config.json in kubernetes with configmap volumeMounts.
When I modify the config.json in the configmap, the contents of the config.json in the container are the same as those in the configmap (delay about 10 seconds after the modification will be synchronized to the container), but there is no change in the program.

To Reproduce

This is my test code https://github.com/expcat/configmap-test
I use configmap-test.yaml to run it in the internal kubernetes test.

Expected behavior

curl localhost/api/values response value returned should be consistent with the contents of config.json.

Screenshots

When the container is initialized:
step1

After modification the configmap(delay about 10 sec):
step2

Additional context

kubernetes ver: 1.13.3
asp.net core ver: 2.2.2

English is not my native language. Please forgive me if any wrong expression.

@expcat
Copy link
Author

expcat commented Feb 26, 2019

I use dotnet run to run on my computer.

PS E:\k8s\configmap> curl http://localhost:5000/api/values | Select Content

Content
-------
["v1"]


PS E:\k8s\configmap> curl http://localhost:5000/api/values | Select Content

Content
-------
["v1.2"]

@expcat
Copy link
Author

expcat commented Feb 26, 2019

Only use docker run, the result changes with the modification mapping file config.json.
It seems that the problem is the configmap in kubernetes.

@muratg
Copy link

muratg commented Feb 26, 2019

Yeah, as long as the json file is there, it should work the same way it does elsewhere. Were you able to figure out the configmap issue?

@muratg
Copy link

muratg commented Feb 26, 2019

You can try this to see if your config.json file is there:

  • Run kubectl exec -it PODNAME bash to get a shell at your pod (replace PODNAME with your pod name.)
  • Go the the folder with your app and ls to see if the file is there.

@expcat
Copy link
Author

expcat commented Feb 27, 2019

@muratg thank you for your reply.
The operation in the screenshot above is to run in the pod.
I use putty to enter the pod again and run ls like this.
step3
The config.json file is there and the content has changed, but the value of getting the binding from the api has not changed.

@fbeltrao
Copy link

Hi @ajcvickers, any update on this?
Came across the same issue. Would love an update :)

@ajcvickers
Copy link
Member

@fbeltrao This issue is in the Backlog milestone. This means that it is not going to happen for the 3.0 release. We will re-assess the backlog following the 3.0 release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

@fbeltrao
Copy link

I came up with a solution while we wait for a fix:
https://github.com/fbeltrao/ConfigMapFileProvider

@syedhassaanahmed
Copy link

@ajcvickers Now that 3.0 is GA, any update on the fix?

@fstugren
Copy link

Ha! I just ran into this issue myself. I built this web application which consumes a configuration file deployed via a configmap. The configuration has to be tweaked often enough to justify adding support for automatic config updates, so that the pod won't have to be redeployed or killed and restarted each time the configuration changes. Setting this up in .NET core 2.2 was easy enough (although I had to work around some issues with singletons), but sadly, after the local testing proved the update model worked fine I deployed the AKS pod with high hopes only to be sorely disappointed. A quick search brought me here. I am sure there's workarounds to this - like the file content monitor mentioned by an earlier comment - but it would be great to have this feature work natively.

Thanks
F.

@expcat
Copy link
Author

expcat commented Jan 21, 2020

Is it in 5.0 plan?

@ghost
Copy link

ghost commented May 8, 2020

As part of the migration of components from dotnet/extensions to dotnet/runtime (aspnet/Announcements#411) we will be bulk closing some of the older issues. If you are still interested in having this issue addressed, just comment and the issue will be automatically reactivated (even if you aren't the author). When you do that, I'll page the team to come take a look. If you've moved on or workaround the issue and no longer need this change, just ignore this and the issue will be closed in 7 days.

If you know that the issue affects a package that has moved to a different repo, please consider re-opening the issue in that repo. If you're unsure, that's OK, someone from the team can help!

@expcat
Copy link
Author

expcat commented May 8, 2020

I think this should not be closed.

@ghost
Copy link

ghost commented May 8, 2020

Paging @dotnet/extensions-migration ! This issue has been revived from staleness. Please take a look and route to the appropriate repository.

@analogrelay analogrelay transferred this issue from dotnet/extensions May 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 8, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@analogrelay analogrelay added this to the Future milestone May 8, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@peacecwz
Copy link

@ajcvickers any update? I got this same issue in .NET Core 3.1. We can't develop Cloud Native Application:(

@RainingNight
Copy link

any news?

@btbensoft
Copy link

+1

@expcat
Copy link
Author

expcat commented Sep 22, 2020

This issue has the most 👀 in the opened issues of this repo.
Is there any plan to fix it in 6.0?

@maryamariyan maryamariyan modified the milestones: Future, 6.0.0 Oct 6, 2020
@maryamariyan
Copy link
Member

Is there any plan to fix it in 6.0?

Will try to fix this in 6.0

@davidfowl
Copy link
Member

This is likely because the file watcher when running in a container is busted. Did you try setting the environment variable DOTNET_USE_POLLING_FILE_WATCHER=true

@expcat
Copy link
Author

expcat commented Oct 7, 2020

I tried to add environment variables, the result did not change.
k8s-yaml:

      containers:
        - name: configmap-test
          image: expcat/configmap-test:1.2
          env: 
            - name: DOTNET_USE_POLLING_FILE_WATCHER
              value: "true"

Inside the container:

root@configmap-test-7b8f7b4777-td65r:~# echo $DOTNET_USE_POLLING_FILE_WATCHER
true
root@configmap-test-7b8f7b4777-td65r:~# cat /src/conf/config.json
{
  "config": {
    "message": "v1.1"
  }
}
root@configmap-test-7b8f7b4777-td65r:~# curl localhost/api/values
["v1"]

@ericstj
Copy link
Member

ericstj commented Feb 26, 2021

For those that prefer a smaller workaround and don't care about Windows support, here's a minimal one:

<PackageReference Include="Mono.Posix.NETStandard" Version="1.0.0" />
    .ConfigureAppConfiguration(c => c.AddSymLinkJsonFile("config/appsettings.json", optional: true, reloadOnChange: true));
using Microsoft.Extensions.FileProviders;
using Mono.Unix;
using System.IO;
using System.Runtime.InteropServices;

namespace Microsoft.Extensions.Configuration
{
    internal static class JsonSymlinkConfigurationExtensions
    {
        internal static void AddSymLinkJsonFile(this IConfigurationBuilder c, string relativePath, bool optional, bool reloadOnChange)
        {
            var fileInfo = c.GetFileProvider().GetFileInfo(relativePath);

            if (TryGetSymLinkTarget(fileInfo.PhysicalPath, out string targetPath))
            {
                string targetDirectory = Path.GetDirectoryName(targetPath);

                if (TryGetSymLinkTarget(targetDirectory, out string symlinkDirectory))
                {
                    targetDirectory = symlinkDirectory;
                }

                c.AddJsonFile(new PhysicalFileProvider(targetDirectory), Path.GetFileName(targetPath), optional, reloadOnChange);
            }
            else
            {
                c.AddJsonFile(relativePath, optional, reloadOnChange);
            }
        }

        private static bool TryGetSymLinkTarget(string path, out string target, int maximumSymlinkDepth = 32)
        {
            target = null;

            int depth = 0;

            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                var symbolicLinkInfo = new UnixSymbolicLinkInfo(path);
                while (symbolicLinkInfo.Exists && symbolicLinkInfo.IsSymbolicLink)
                {
                    target = symbolicLinkInfo.ContentsPath;

                    if (!Path.IsPathFullyQualified(target))
                    {
                        target = Path.GetFullPath(target, Path.GetDirectoryName(symbolicLinkInfo.FullName));
                    }

                    symbolicLinkInfo = new UnixSymbolicLinkInfo(target);
                    
                    if (depth++ > maximumSymlinkDepth)
                    {
                        throw new InvalidOperationException("Exceeded maximum symlink depth");
                    }
                }
            }
            return target != null;
        }
    }
}

@LirazShay
Copy link

We also need this feature please

@ghost
Copy link

ghost commented Mar 19, 2021

Played around with @ericstj solution. Works, but had to make certain adjustments to make it work on absolute paths.

using Microsoft.Extensions.FileProviders;
using Mono.Unix;
using System.IO;
using System.Runtime.InteropServices;

namespace Microsoft.Extensions.Configuration
{
    internal static class JsonSymlinkConfigurationExtensions
    {
        internal static void AddSymLinkJsonFile(this IConfigurationBuilder c, string configFileFullPath, bool optional, bool reloadOnChange)
        {
            if (TryGetSymLinkTarget(fileInfo.PhysicalPath, out string targetPath))
            {
                if (Path.IsPathFullyQualified(targetPath) == false)
                {
                     targetPath = Path.GetFullPath(targetPath, Path.GetDirectoryName(configFileFullPath));                     
                }
                c.AddJsonFile(new PhysicalFileProvider(Path.GetDirectoryName(targetPath)), Path.GetFileName(targetPath), optional, reloadOnChange);
            }
            else
            {
                c.AddJsonFile(relativePath, optional, reloadOnChange);
            }
        }

        private static bool TryGetSymLinkTarget(string path, out string target)
        {
            target = null;

            if (File.Exists(path) && !RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            {
                var symbolicLinkInfo = new UnixSymbolicLinkInfo(path);
                if (symbolicLinkInfo.IsSymbolicLink)
                {
                    target = symbolicLinkInfo.ContentsPath;
                    return true;
                }
            }
            return false;
        }
    }
}

@ericstj
Copy link
Member

ericstj commented Mar 30, 2021

Works, but had to make certain adjustments to make it work on absolute path

Glad you were able to make it work. I was previously trying to get the full path by going through the file provider, but perhaps that wasn't working on all systems. I only tested on Windows + Ubuntu in WSL.

@ericstj
Copy link
Member

ericstj commented Apr 2, 2021

I see, you were faced with relative paths as the target of symlinks. We should handle that. It's also possible to have recursive symlinks, as well as a symlink on the directory itself. I updated my sample above to accommodate this.

@ericstj
Copy link
Member

ericstj commented Apr 5, 2021

@smithago was looking at this and discovered that following the symlink isn't good enough. Kubernetes doesn't actually update files. It creates a series of symlinks then modifies the one in the middle.

So A > B > C. C is never updated, instead D is created and B is changed to point to D. This means that we won't capture this by following symlinks, nor would any INotify solution really help here since we'd need to add new files to watch on the fly. I think the simplest solution to this is to not follow the symlinks, instead create a polling watcher that examines the modified time on the target file, as @jasper-d previously suggested. I'm going to see If I can prototype something that does this using either mono.posix or direct PInvokes like I did for following symlinks.

@anthonyliriano
Copy link

+1, is this enhancement being considered in 6.0?

@Kampfmoehre
Copy link

Is it possible to have an external watcher (config map watcher) that sends an event to the ASP .NET Core application (for example a signal) that would trigger a config reload?

@tmds
Copy link
Member

tmds commented May 10, 2021

was looking at this and discovered that following the symlink isn't good enough.
So A > B > C. C is never updated, instead D is created and B is changed to point to D.

For example:

drwxr-sr-x. 2 root 1003220000 36 May 10 09:52 ..2021_05_10_09_52_07.053597227
lrwxrwxrwx. 1 root root       31 May 10 09:52 ..data -> ..2021_05_10_09_52_07.053597227
lrwxrwxrwx. 1 root root       29 May 10 09:52 application.properties -> ..data/application.properties

becomes:

drwxr-sr-x. 2 root 1003220000 36 May 10 09:55 ..2021_05_10_09_55_18.243959372
lrwxrwxrwx. 1 root 1003220000 31 May 10 09:55 ..data -> ..2021_05_10_09_55_18.243959372
lrwxrwxrwx. 1 root root       29 May 10 09:52 application.properties -> ..data/application.properties

FileInfo internally uses lstat syscall instead of stat

It could be considered to change this so the FileInfo reflects what is linked to instead of the link.

@Joe4evr
Copy link
Contributor

Joe4evr commented May 10, 2021

It could be considered to change this so the FileInfo reflects what is linked to instead of the link.

#24271.

@tmds
Copy link
Member

tmds commented May 10, 2021

#24271.

I mean: change behavior of the exiting API so it follows the link by default.

@tmds
Copy link
Member

tmds commented May 19, 2021

@jozkee @ericstj afaik FileSystemWatcher isn't used for this in the container images, but polling the file is used instead. So changes to FilesystemWatcher won't resolve this issue.

The APIs added in #24271 can be used to handle the symbolic links manually.
Though imo this is a preferable API: #52908.

@ericstj
Copy link
Member

ericstj commented May 19, 2021

Agreed that the fix here needs to consume API that can poll the target of symlinks.

@jozkee jozkee added the Priority:1 Work that is critical for the release, but we could probably ship without label Jun 3, 2021
@jeffhandley jeffhandley self-assigned this Jun 24, 2021
@jeffhandley
Copy link
Member

As has been discussed on this issue, the proper resolution for this involves using new symbolic link APIs that we are still working to land in .NET 6.0 Preview 7. It's possible the resolution won't make Preview 7 since the prerequisite PR (#54253) is still in progress. If the Preview 7 window closes before this can be resolved, this will become an RC1 candidate.

@jozkee
Copy link
Member

jozkee commented Jul 13, 2021

The fix I've planned for this is meant to use the new Symbolic Link APIs added in .NET 6 (#54253). This will make the fix to be available only for .NET 6 users, if you are using netstandard2.0 or .NET framework you will have to migrate. Please let us know if that doesn't work for you.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO bug Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.