Skip to content

[WIP] manage cluster capacity policy via KST #106

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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

ashleyvansp
Copy link

No description provided.

@Copilot Copilot AI review requested due to automatic review settings June 23, 2025 20:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces support for managing Kusto cluster capacity policies via KST by adding a new model, loader, and writer, and wiring them into the schema handler and database factory. It also updates JSON serialization to omit null values.

  • Define ClusterCapacityPolicy model with JSON serialization and script generation
  • Implement loader (ClusterCapacityPolicyLoader) and writer (ClusterCapacityPolicyWriter)
  • Wire capacity policy handling into KustoDatabaseHandlerFactory and KustoSchemaHandler

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Parser/KustoWriter/ClusterCapacityPolicyWriter.cs Writes .alter-merge cluster policy capacity commands
Parser/KustoLoader/ClusterCapacityPolicyLoader.cs Loads existing cluster capacity policy via .show query
Parser/KustoDatabaseHandlerFactory.cs Adds ClusterCapacityPolicyLoader and writer to factory
Model/Clusters.cs Adds CapacityPolicy property to clusters config
Model/ClusterCapacityPolicy.cs Defines capacity policy model and script creation
KustoSchemaHandler.cs Refactors handler to include capacity policy writer
Helpers/Serialization.cs Ignores nulls in JSON serialization
Tests/DemoData/.../partial-cluster-policy.yml Adds partial policy sample for tests
Tests/DemoData/.../comprehensive-cluster-policy.yml Adds full policy sample for tests
KustoSchemaTools.Tests/ClusterCapacityPolicyTests.cs Tests capacity policy loading, serialization, and script
Comments suppressed due to low confidence (2)

KustoSchemaTools.Tests/ClusterCapacityPolicyTests.cs:1

  • Missing using Xunit; and using System.IO; directives, which are required for the [Fact] attribute and for Path/File usage in this test file.
using KustoSchemaTools.Helpers;

KustoSchemaTools/Parser/KustoLoader/ClusterCapacityPolicyLoader.cs:24

  • The deserialized capacityPolicy is not stored on the database model, so the existing policy won't be tracked for comparison. Consider assigning it to database.CapacityPolicy.
                    var capacityPolicy = JsonConvert.DeserializeObject<ClusterCapacityPolicy>(capacityPolicyData);

private IDatabaseHandler<T> CreateDatabaseHandlerWithCapacityPolicy(string clusterUrl, string databaseName, ClusterCapacityPolicy? capacityPolicy)
{
// Create a logger - in a real application this would come from DI
var loggerFactory = LoggerFactory.Create(builder => { });
Copy link
Preview

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Instantiating a new LoggerFactory on each call is expensive; consider reusing a shared ILoggerFactory or injecting it once to reduce allocations.

Copilot uses AI. Check for mistakes.

// For now, we don't need to store it as we're only applying changes
}
}
catch (Exception)
Copy link
Preview

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Catching a broad Exception without any logging may hide underlying issues during load; consider logging a warning with exception details.

Copilot uses AI. Check for mistakes.

Comment on lines 55 to 56
return new KustoDatabaseHandlerFactory<T>(logger)
.WithReader<KustoDatabasePrincipalLoader>()
Copy link
Preview

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The reader/writer setup chain is duplicated between CreateDefault and CreateDatabaseHandlerWithCapacityPolicy; extract into a shared helper to adhere to DRY principles.

Suggested change
return new KustoDatabaseHandlerFactory<T>(logger)
.WithReader<KustoDatabasePrincipalLoader>()
var factory = new KustoDatabaseHandlerFactory<T>(logger);
factory.AddDefaultReadersAndWriters();
return factory.Create(cluster, database);
}
private void AddDefaultReadersAndWriters()
{
WithReader<KustoDatabasePrincipalLoader>()

Copilot uses AI. Check for mistakes.

Comment on lines +42 to +45
catch (System.Exception ex)
{
_logger.LogError(ex, "Failed to load cluster capacity policy.");
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.

Copilot Autofix

AI about 3 hours ago

To fix the issue, replace the generic catch clause with specific exception types that are likely to occur during the execution of the ExecuteControlCommandAsync method. For example:

  1. Catch Kusto.Data.Exceptions.KustoClientException for errors related to the Kusto client.
  2. Catch JsonException for errors during JSON deserialization.
  3. Add a fallback catch block for unexpected exceptions, if necessary, but avoid catching all exceptions indiscriminately.

This approach ensures that only expected errors are handled explicitly, while unexpected errors can propagate or be logged separately.


Suggested changeset 1
KustoSchemaTools/Parser/KustoClusterHandler.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/KustoSchemaTools/Parser/KustoClusterHandler.cs b/KustoSchemaTools/Parser/KustoClusterHandler.cs
--- a/KustoSchemaTools/Parser/KustoClusterHandler.cs
+++ b/KustoSchemaTools/Parser/KustoClusterHandler.cs
@@ -47,5 +47,13 @@
             }
-            catch (System.Exception ex)
+            catch (Kusto.Data.Exceptions.KustoClientException ex)
             {
-                _logger.LogError(ex, "Failed to load cluster capacity policy.");
+                _logger.LogError(ex, "Kusto client error occurred while loading cluster capacity policy.");
+            }
+            catch (JsonException ex)
+            {
+                _logger.LogError(ex, "JSON deserialization error occurred while processing cluster capacity policy.");
+            }
+            catch (Exception ex)
+            {
+                _logger.LogError(ex, "An unexpected error occurred while loading cluster capacity policy.");
             }
EOF
@@ -47,5 +47,13 @@
}
catch (System.Exception ex)
catch (Kusto.Data.Exceptions.KustoClientException ex)
{
_logger.LogError(ex, "Failed to load cluster capacity policy.");
_logger.LogError(ex, "Kusto client error occurred while loading cluster capacity policy.");
}
catch (JsonException ex)
{
_logger.LogError(ex, "JSON deserialization error occurred while processing cluster capacity policy.");
}
catch (Exception ex)
{
_logger.LogError(ex, "An unexpected error occurred while loading cluster capacity policy.");
}
Copilot is powered by AI and may make mistakes. Always verify output.
Assert.NotNull(policyChange);
Assert.Equal(2, policyChange!.PropertyChanges.Count);

var ingestionChange = Assert.Single(policyChange.PropertyChanges, p => p.PropertyName == "IngestionCapacity");

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning test

Variable
policyChange
may be null at this access because of
this
assignment.

Copilot Autofix

AI about 3 hours ago

To fix the issue, we will ensure that policyChange is not null before it is dereferenced on line 81. This can be achieved by adding a null-forgiving operator (!) to the dereference on line 81, as the Assert.NotNull(policyChange) on line 78 already guarantees that policyChange is not null. This approach maintains the existing functionality while addressing the static analysis warning.

Suggested changeset 1
KustoSchemaTools.Tests/Changes/ClusterChangesTest.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/KustoSchemaTools.Tests/Changes/ClusterChangesTest.cs b/KustoSchemaTools.Tests/Changes/ClusterChangesTest.cs
--- a/KustoSchemaTools.Tests/Changes/ClusterChangesTest.cs
+++ b/KustoSchemaTools.Tests/Changes/ClusterChangesTest.cs
@@ -80,3 +80,3 @@
 
-            var ingestionChange = Assert.Single(policyChange.PropertyChanges, p => p.PropertyName == "IngestionCapacity");
+            var ingestionChange = Assert.Single(policyChange!.PropertyChanges, p => p.PropertyName == "IngestionCapacity");
             Assert.Equal("{\"CoreUtilizationCoefficient\":0.75}", ingestionChange.OldValue);
EOF
@@ -80,3 +80,3 @@

var ingestionChange = Assert.Single(policyChange.PropertyChanges, p => p.PropertyName == "IngestionCapacity");
var ingestionChange = Assert.Single(policyChange!.PropertyChanges, p => p.PropertyName == "IngestionCapacity");
Assert.Equal("{\"CoreUtilizationCoefficient\":0.75}", ingestionChange.OldValue);
Copilot is powered by AI and may make mistakes. Always verify output.
public async Task GenerateChangesFromFileAsync_ValidYamlFile_ReturnsChanges()
{
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Check notice

Code scanning / CodeQL

Call to System.IO.Path.Combine Note test

Call to 'System.IO.Path.Combine'.

Copilot Autofix

AI about 2 hours ago

To fix the issue, replace the call to Path.Combine with Path.Join. The Path.Join method is designed to concatenate paths without dropping earlier arguments, even if a later argument is an absolute path. This ensures that the resulting path is always constructed as intended.

The change will be made in the GenerateChangesFromFileAsync_ValidYamlFile_ReturnsChanges test method, specifically on line 404. No additional imports or definitions are required, as Path.Join is part of the System.IO namespace already included in the file.

Suggested changeset 1
KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
--- a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
+++ b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
@@ -403,3 +403,3 @@
             // Arrange
-            var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
+            var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
             
EOF
@@ -403,3 +403,3 @@
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Copilot is powered by AI and may make mistakes. Always verify output.
public async Task GenerateChangesFromFileAsync_VerifyLoggingCalled()
{
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Check notice

Code scanning / CodeQL

Call to System.IO.Path.Combine Note test

Call to 'System.IO.Path.Combine'.

Copilot Autofix

AI about 2 hours ago

To fix the issue, replace the call to Path.Combine with Path.Join. The Path.Join method is designed to concatenate paths without the risk of dropping earlier arguments if an absolute path is encountered. This change will ensure that the code behaves correctly regardless of whether the arguments are relative or absolute paths.

The change will be made on line 543 of the file KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs. No additional imports or dependencies are required, as Path.Join is part of the System.IO namespace.

Suggested changeset 1
KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
--- a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
+++ b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
@@ -542,3 +542,3 @@
             // Arrange
-            var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
+            var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
             
EOF
@@ -542,3 +542,3 @@
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Copilot is powered by AI and may make mistakes. Always verify output.
public async Task GenerateChangesFromFileAsync_LoadAsyncThrowsException_PropagatesException()
{
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Check notice

Code scanning / CodeQL

Call to System.IO.Path.Combine Note test

Call to 'System.IO.Path.Combine'.

Copilot Autofix

AI about 2 hours ago

To fix the issue, replace the call to Path.Combine with Path.Join. The Path.Join method is a safer alternative for constructing file paths because it does not ignore earlier arguments when an absolute path is encountered. This change ensures that the code behaves correctly even if any of the arguments are modified to be absolute paths in the future.

The change will be made on line 610 of the file KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs. No additional imports or dependencies are required since Path.Join is part of the System.IO namespace.


Suggested changeset 1
KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
--- a/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
+++ b/KustoSchemaTools.Tests/KustoClusterOrchestratorTests.cs
@@ -609,3 +609,3 @@
             // Arrange
-            var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
+            var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
             
EOF
@@ -609,3 +609,3 @@
// Arrange
var yamlFilePath = Path.Combine("DemoData", "ClusterScopedChanges", "multipleClusters.yml");
var yamlFilePath = Path.Join("DemoData", "ClusterScopedChanges", "multipleClusters.yml");

Copilot is powered by AI and may make mistakes. Always verify output.
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.

1 participant