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.

Sign up for GitHub

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

Jump to bottom

Hot Reload Validation #2405

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+57 −1
Open

Hot Reload Validation #2405

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c73ce91
save state before merge main
aaronburtle Sep 7, 2024
f2c4b7c
event for reloading document
aaronburtle Sep 7, 2024
fb8bb62
save branch
aaronburtle Sep 10, 2024
0dd804a
merge
aaronburtle Sep 12, 2024
ec4af37
save progress
aaronburtle Sep 13, 2024
c672dd3
save progress
aaronburtle Sep 13, 2024
401cd33
name change
aaronburtle Sep 13, 2024
66e2158
hot reload open api doc
aaronburtle Sep 13, 2024
f2b1093
let create document hotreload
aaronburtle Sep 14, 2024
0abb169
comments
aaronburtle Sep 14, 2024
6a81b95
add ool to interface
aaronburtle Sep 14, 2024
d6cc930
handle null check for create doc
aaronburtle Sep 18, 2024
42e5a3a
Merge remote-tracking branch 'origin/dev/aaronburtle/EventHandleOpenA…
Sep 28, 2024
635688c
Validation Changes
Oct 1, 2024
7f08fb2
Validation changes
Oct 2, 2024
a8b9dd9
Comment changes
Oct 10, 2024
cdd597a
Merge branch 'main' into dev/rubencerna/HotReload_Validation
RubenCerna2079 Oct 11, 2024
0581101
Merge Conflict Changes
Oct 11, 2024
111bd59
Merge branch 'main' into dev/rubencerna/HotReload_Validation
RubenCerna2079 Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/Config/FileSystemRuntimeConfigLoader.cs
View file
Open in desktop
RubenCerna2079 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,20 @@ public bool TryLoadConfig(
}

config = RuntimeConfig;

if (lastValidRuntimeConfig is null)
{
lastValidRuntimeConfig = RuntimeConfig;
}

return true;
}

if (lastValidRuntimeConfig is not null)
{
RuntimeConfig = lastValidRuntimeConfig;
}

config = null;
return false;
}
Expand Down Expand Up @@ -202,7 +213,16 @@ public override bool TryLoadKnownConfig([NotNullWhen(true)] out RuntimeConfig? c
public void HotReloadConfig(string defaultDataSourceName, ILogger? logger = null)
{
logger?.LogInformation(message: "Starting hot-reload process for config: {ConfigFilePath}", ConfigFilePath);
TryLoadConfig(ConfigFilePath, out _, replaceEnvVar: true, defaultDataSourceName: defaultDataSourceName);
if (!TryLoadConfig(ConfigFilePath, out _, replaceEnvVar: true, defaultDataSourceName: defaultDataSourceName))
{
throw new DataApiBuilderException(
message: "Deserialization of the configuration file failed.",
statusCode: HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
}

isNewConfigDetected = true;
isNewConfigValidated = false;
SendEventNotification();
}

Expand Down
6 changes: 6 additions & 0 deletions src/Config/RuntimeConfigLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ public abstract class RuntimeConfigLoader
// state in place of using out params.
public RuntimeConfig? RuntimeConfig;

public RuntimeConfig? lastValidRuntimeConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


public bool isNewConfigDetected;

public bool isNewConfigValidated = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry not sure why both vars were highlighted, I meant public bool isNewConfigValidated = true; should isNewConfigValidated be defaulted to false? e.g. a config is invalid until proven otherwise.


public RuntimeConfigLoader(HotReloadEventHandler<HotReloadEventArgs>? handler = null, string? connectionString = null)
{
_handler = handler;
Expand Down
30 changes: 30 additions & 0 deletions src/Core/Configurations/RuntimeConfigProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@

RubenCerna2079 marked this conversation as resolved.
Show resolved Hide resolved
using System.Data.Common;
using System.Diagnostics.CodeAnalysis;
using System.IO.Abstractions;
using System.Net;
using Azure.DataApiBuilder.Config;
using Azure.DataApiBuilder.Config.Converters;
using Azure.DataApiBuilder.Config.NamingPolicies;
using Azure.DataApiBuilder.Config.ObjectModel;
using Azure.DataApiBuilder.Service.Exceptions;
using Microsoft.Extensions.Logging;

namespace Azure.DataApiBuilder.Core.Configurations;

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
IFileSystem fileSystem = new FileSystem();
ILoggerFactory loggerFactory = new LoggerFactory();
ILogger<RuntimeConfigValidator> logger = loggerFactory.CreateLogger<RuntimeConfigValidator>();
RuntimeConfigValidator runtimeConfigValidator = new(this, fileSystem, logger, true);
Comment on lines +82 to +85
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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>();"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that's what I meant by "obsolete." The code sample i provided isn't possible, but in an ideal world, it would be. To be doable, other refactors are necessary. That's the background behind my comment of "For now, this (your current code change) is the only straightforward way of achieving validation during a hot reload."


_configLoader.isNewConfigDetected = false;
_configLoader.isNewConfigValidated = runtimeConfigValidator.TryValidateConfig(ConfigFilePath, loggerFactory).Result;

// Saves the lastValidRuntimeConfig as the new RuntimeConfig if it is validated for hot reload
if (_configLoader.isNewConfigValidated)
{
_configLoader.lastValidRuntimeConfig = _configLoader.RuntimeConfig;
}
else
{
_configLoader.isNewConfigValidated = true;
_configLoader.RuntimeConfig = _configLoader.lastValidRuntimeConfig;
Comment on lines +97 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

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


throw new DataApiBuilderException(
message: "Failed validation of configuration file.",
statusCode: HttpStatusCode.ServiceUnavailable,
subStatusCode: DataApiBuilderException.SubStatusCodes.ErrorInInitialization);
Comment on lines +97 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. Do you still need to raise an exception?
  2. What would catch that exception and would such an exception imply that DAB should shut down?

Copy link
Contributor Author

@RubenCerna2079 RubenCerna2079 Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

if (_configLoader.RuntimeConfig is not null)
{
return _configLoader.RuntimeConfig;
Expand Down