Hot Reload Validation #2405

RubenCerna2079 · 2024-10-02T22:47:01Z

Why make this change?

This change fixes issue #2396

What is this change?

In order to validate the hot-reload, first, new logic was added to the RuntimeConfigLoader and FileSystemRuntimeConfigLoader, so when hot reload occurs, it updates the new states added to the loader which are isNewConfigDetected, isNewConfigValidated, and lastValidRuntimeConfig.
Once the states are updated, it will try to parse and validate the new RuntimeConfig, in the case that it fails at any step of the way, the hot-reloading process will be canceled and the RuntimeConfig file will be changed so that it uses the information from lastValidRuntimeConfig so that the user can still use their last configuration without any problems.

How was this tested?

Integration Tests
Unit Tests

…piDoc' into dev/rubencerna/HotReload_Validation

src/Config/FileSystemRuntimeConfigLoader.cs

src/Core/Configurations/RuntimeConfigProvider.cs

src/Core/Services/OpenAPI/OpenApiDocumentor.cs

src/Core/Services/OpenAPI/IOpenApiDocumentor.cs

src/Config/RuntimeConfigLoader.cs

RubenCerna2079 · 2024-10-02T23:02:57Z

src/Core/Configurations/RuntimeConfigProvider.cs

+            IFileSystem fileSystem = new FileSystem();
+            ILoggerFactory loggerFactory = new LoggerFactory();
+            ILogger<RuntimeConfigValidator> logger = loggerFactory.CreateLogger<RuntimeConfigValidator>();
+            RuntimeConfigValidator runtimeConfigValidator = new(this, fileSystem, logger, true);


Not sure if this is fine, since there is no logger inside the provider

For now, this is the only straightforward way of achieving validation during a hot reload.

My original response was the following, however we need to re-evaluate how we configure services in startup.cs such that we don't need to have runtimeconfigprovider resolved. That will need to be tracked via a separate task.
--------------------Obsolete-----------------------
There looks to be a better way of accomplishing instantiation of the RuntimeConfigValidator.

In Startup.cs:

IFileSystem fileSystem = new FileSystem(); FileSystemRuntimeConfigLoader configLoader = new(fileSystem, hotReloadEventHandler, configFileName, connectionString); RuntimeConfigProvider configProvider = new(configLoader); RuntimeConfigValidator configValidator = new( runtimeConfigProvider: configProvider, fileSystem: fileSystem, logger: , // this is an issue, we can't resolve an ilogger in Startup::ConfigureServices isValidateOnly: true); services.AddSingleton(fileSystem); services.AddSingleton(configProvider); services.AddSingleton(configLoader); services.AddSingleton<configValidator>(); // instead of "services.AddSingleton<RuntimeConfigValidator>();"

I was not able to find the code you put from Startup.cs and that way of instantiating RuntimeConfigValidator did not work for me.

RubenCerna2079 · 2024-10-03T04:13:52Z

/azp run

azure-pipelines · 2024-10-03T04:13:58Z

Pull request contains merge conflicts.

src/Config/FileSystemRuntimeConfigLoader.cs

src/Config/RuntimeConfigLoader.cs

src/Core/Configurations/RuntimeConfigProvider.cs

src/Core/Services/OpenAPI/OpenApiDocumentor.cs

src/Core/Configurations/RuntimeConfigProvider.cs

seantleonard · 2024-10-08T17:23:23Z

src/Config/FileSystemRuntimeConfigLoader.cs

You should add a little more detail to your PR description describing how these mechanisms work:

Hot Reload occurs which updates state in the Loader -> NewConfigDetected, isNewConfigValidated, "used"/newRuntimeConfigObject

any service which attempts to RuntimeConfigProvider::GetRuntimeConfig() will trigger RuntimeConfigProvider::GetRuntimeConfig()'s internal logic to check the loaders state for isnewconfigValidated, and attempt to validate the config. then return the validated config.
Then describe what the expectations should be when a hotreloadedconfig is invalid.
Another thing to think about:

do we need to introduce a lock in RuntimeConfigProvider::GetRuntimeConfig() if the condition holds that the new runtimeconfig needs validation? e.g. if multiple dependent services attempt to fetch a hotreloaded config, only only operation should trigger validation, and the remaining callers can utilize that result as opposed to trigger multiple validation operations on the same config.

Updated the description

I changed it so that FileSystemRuntimeConfig::HotReloadConfig() is the one that changes the state of isNewConfigValidated and isNewConfigDetected, so I believe that the internal logic to validate the config will only happen when there is a hot reload.

The changes I made, make it so that it only makes the validation on the first OnConfigChangeEvent while the others use the result.

RubenCerna2079 · 2024-10-11T18:05:00Z

/azp run

RubenCerna2079 · 2024-10-11T21:34:57Z

/azp run

RubenCerna2079 · 2024-10-11T22:35:23Z

/azp run

seantleonard · 2024-10-14T17:51:39Z

src/Core/Configurations/RuntimeConfigProvider.cs

+                _configLoader.isNewConfigValidated = true;
+                _configLoader.RuntimeConfig = _configLoader.lastValidRuntimeConfig;
+
+                throw new DataApiBuilderException(
+                    message: "Failed validation of configuration file.",
+                    statusCode: HttpStatusCode.ServiceUnavailable,
+                    subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);


Because you're reverting to a Last Known Good (LKG) runtimeconfig where isNewConfigValidated is true:

Do you still need to raise an exception?

What would catch that exception and would such an exception imply that DAB should shut down?

I still think that we should raise an exception that allows the user to know that their changes to the hot reload were not successful and that they are using their old configuration settings. As for the exception, it is catched in ConfigFileWatcher::OnConfigFileChange which from my understanding, implies that DAB should still be running, and it gives a chance to the user to correctly change their config file for hot reload.

seantleonard · 2024-10-14T17:53:39Z

src/Config/RuntimeConfigLoader.cs

+
+    public bool isNewConfigDetected;
+
+    public bool isNewConfigValidated = true;


Should this default to false? only switch to true when validated?

Yes, isNewConfigDetected is by default false and should only switch to true when there is going to be a hot-reload

seantleonard · 2024-10-14T17:56:58Z

src/Core/Configurations/RuntimeConfigProvider.cs

+                _configLoader.isNewConfigValidated = true;
+                _configLoader.RuntimeConfig = _configLoader.lastValidRuntimeConfig;


perhaps this should be a function internal to the ConfigLoader so that ConfigLoader handles its own state whenever possible versus the Provider doing it, What do you think of this?

_configLoader.RestoreLkgConfig() -> internally restores loader.lastValidConfig to loader.Config

seantleonard · 2024-10-14T19:38:28Z

src/Core/Configurations/RuntimeConfigProvider.cs

+            IFileSystem fileSystem = new FileSystem();
+            ILoggerFactory loggerFactory = new LoggerFactory();
+            ILogger<RuntimeConfigValidator> logger = loggerFactory.CreateLogger<RuntimeConfigValidator>();
+            RuntimeConfigValidator runtimeConfigValidator = new(this, fileSystem, logger, true);


For now, this is the only straightforward way of achieving validation during a hot reload.

My original response was the following, however we need to re-evaluate how we configure services in startup.cs such that we don't need to have runtimeconfigprovider resolved. That will need to be tracked via a separate task.
--------------------Obsolete-----------------------
There looks to be a better way of accomplishing instantiation of the RuntimeConfigValidator.

In Startup.cs:

IFileSystem fileSystem = new FileSystem(); FileSystemRuntimeConfigLoader configLoader = new(fileSystem, hotReloadEventHandler, configFileName, connectionString); RuntimeConfigProvider configProvider = new(configLoader); RuntimeConfigValidator configValidator = new( runtimeConfigProvider: configProvider, fileSystem: fileSystem, logger: , // this is an issue, we can't resolve an ilogger in Startup::ConfigureServices isValidateOnly: true); services.AddSingleton(fileSystem); services.AddSingleton(configProvider); services.AddSingleton(configLoader); services.AddSingleton<configValidator>(); // instead of "services.AddSingleton<RuntimeConfigValidator>();"

seantleonard · 2024-10-14T19:39:33Z

src/Core/Configurations/RuntimeConfigProvider.cs

@@ -74,6 +76,34 @@ public RuntimeConfigProvider(RuntimeConfigLoader runtimeConfigLoader)
    /// <exception cref="DataApiBuilderException">Thrown when the loader is unable to load an instance of the config from its known location.</exception>
    public RuntimeConfig GetConfig()
    {
+        // Only used in hot reload to validate the configuration file
+        if (_configLoader.isNewConfigDetected && !_configLoader.isNewConfigValidated)


your pr description checks the box for "integration tests." how do existing integration tests check hot reload and this new functionality? I'd suggest outline in your PR description how you tested this, even if manually tested.

Aniruddh25 · 2024-10-15T22:57:59Z

src/Core/Configurations/RuntimeConfigProvider.cs

+                    message: "Failed validation of configuration file.",
+                    statusCode: HttpStatusCode.ServiceUnavailable,
+                    subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
+            }


how have we tested this? Is there a way to add an automated integration test in ConfigurationTests.cs?

Aniruddh25 · 2024-10-15T22:59:09Z

src/Config/RuntimeConfigLoader.cs

@@ -31,6 +31,12 @@ public abstract class RuntimeConfigLoader
    // state in place of using out params.
    public RuntimeConfig? RuntimeConfig;

+    public RuntimeConfig? lastValidRuntimeConfig;


All these properties are public properties. So, naming convention wise they should be capitalized. LastValidRuntimeConfig, IsNewConfigDetected, IsNewConfigValidated.

Aniruddh25

Waiting for testing.

aaronburtle and others added 15 commits September 6, 2024 17:55

save state before merge main

c73ce91

event for reloading document

f2c4b7c

save branch

fb8bb62

merge

0dd804a

save progress

ec4af37

save progress

c672dd3

name change

401cd33

hot reload open api doc

66e2158

let create document hotreload

f2b1093

comments

0abb169

add ool to interface

6a81b95

handle null check for create doc

d6cc930

Merge remote-tracking branch 'origin/dev/aaronburtle/EventHandleOpenA…

42e5a3a

…piDoc' into dev/rubencerna/HotReload_Validation

Validation Changes

635688c

Validation changes

7f08fb2

RubenCerna2079 requested review from Aniruddh25, seantleonard, aaronburtle, abhishekkumams, ravishetye, rohkhann, neeraj-sharma2592 and sourabh1007 as code owners October 2, 2024 22:47

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

src/Config/FileSystemRuntimeConfigLoader.cs Outdated Show resolved Hide resolved

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

src/Core/Configurations/RuntimeConfigProvider.cs Show resolved Hide resolved

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

src/Core/Services/OpenAPI/OpenApiDocumentor.cs Outdated Show resolved Hide resolved

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

src/Core/Services/OpenAPI/IOpenApiDocumentor.cs Outdated Show resolved Hide resolved

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

src/Config/RuntimeConfigLoader.cs Outdated Show resolved Hide resolved

RubenCerna2079 commented Oct 2, 2024

View reviewed changes

RubenCerna2079 linked an issue Oct 3, 2024 that may be closed by this pull request

Hot-Reload Validate New Config File #2396

Open

RubenCerna2079 self-assigned this Oct 7, 2024

seantleonard reviewed Oct 8, 2024

View reviewed changes

Comment changes

a8b9dd9

RubenCerna2079 requested a review from sezal98 as a code owner October 10, 2024 16:42

Merge branch 'main' into dev/rubencerna/HotReload_Validation

cdd597a

Merge Conflict Changes

0581101

seantleonard reviewed Oct 14, 2024

View reviewed changes

Aniruddh25 reviewed Oct 15, 2024

View reviewed changes

Aniruddh25 requested changes Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hot Reload Validation #2405

Hot Reload Validation #2405

RubenCerna2079 commented Oct 2, 2024 •

edited

Loading

RubenCerna2079 Oct 2, 2024

seantleonard Oct 14, 2024

RubenCerna2079 Oct 15, 2024

RubenCerna2079 commented Oct 3, 2024

azure-pipelines bot commented Oct 3, 2024

seantleonard Oct 8, 2024

RubenCerna2079 Oct 10, 2024

RubenCerna2079 Oct 15, 2024

RubenCerna2079 commented Oct 11, 2024

RubenCerna2079 commented Oct 11, 2024

RubenCerna2079 commented Oct 11, 2024

seantleonard Oct 14, 2024

RubenCerna2079 Oct 15, 2024 •

edited

Loading

seantleonard Oct 14, 2024

RubenCerna2079 Oct 15, 2024

seantleonard Oct 14, 2024

seantleonard Oct 14, 2024

seantleonard Oct 14, 2024

Aniruddh25 Oct 15, 2024

Aniruddh25 Oct 15, 2024

Aniruddh25 left a comment


		public bool isNewConfigDetected;

		public bool isNewConfigValidated = true;

		_configLoader.isNewConfigValidated = true;
		_configLoader.RuntimeConfig = _configLoader.lastValidRuntimeConfig;

Hot Reload Validation #2405

Are you sure you want to change the base?

Hot Reload Validation #2405

Conversation

RubenCerna2079 commented Oct 2, 2024 • edited Loading

Why make this change?

What is this change?

How was this tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RubenCerna2079 commented Oct 3, 2024

azure-pipelines bot commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RubenCerna2079 commented Oct 11, 2024

RubenCerna2079 commented Oct 11, 2024

RubenCerna2079 commented Oct 11, 2024

Choose a reason for hiding this comment

RubenCerna2079 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aniruddh25 left a comment

Choose a reason for hiding this comment

RubenCerna2079 commented Oct 2, 2024 •

edited

Loading

RubenCerna2079 Oct 15, 2024 •

edited

Loading