From e08c16e935072a264e241d3f9dd5021d26f5dba2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 18:18:21 +0000 Subject: [PATCH 1/9] Add .github/copilot-instructions.md with comprehensive codebase conventions and patterns Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/02aeb2ce-6190-40ff-9285-7e7b7bec8d14 Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 392 ++++++++++++++++++++++++++++++++ 1 file changed, 392 insertions(+) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..653001b146 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,392 @@ +# Copilot Instructions for VirtualClient + +## Project Overview + +VirtualClient is a cross-platform benchmarking and workload execution framework built by Microsoft. It provides a standardized, profile-driven way to run performance benchmarks, stress tests, and qualification workloads on systems (particularly Azure VMs) and collect structured metrics. It supports Windows and Linux on x64 and ARM64 architectures. + +The application is a .NET CLI tool that reads declarative execution profiles (JSON or YAML), installs dependencies, runs workload executors, collects metrics via parsers, and optionally emits telemetry to Azure services. + +## Tech Stack and Key Dependencies + +- **Runtime**: .NET 9 (SDK 9.0.301 defined in `global.json`, rollForward: feature) +- **Target platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` (self-contained publish) +- **CLI parsing**: `System.CommandLine` (2.0.0-beta1) +- **Serialization**: `Newtonsoft.Json` 13.0.3 (JSON), `YamlDotNet` 15.1.1 (YAML profiles) +- **Dependency injection**: `Microsoft.Extensions.DependencyInjection` 9.0.9 +- **Logging**: `Serilog` 9.0.2 + `Serilog.Sinks.File` 6.0.0, custom `EventContext` telemetry +- **Azure integration**: `Azure.Storage.Blobs` 12.18.0, `Azure.Messaging.EventHubs` 5.11.5, `Azure.Security.KeyVault.*` 4.7.0, `Azure.Identity` 1.16.0 +- **HTTP resilience**: `Polly` 8.5.0, `Microsoft.Extensions.Http.Polly` 9.0.9 +- **SSH**: `SSH.NET` 2024.2.0 +- **REST API**: ASP.NET Core (built-in API for client/server coordination) +- **File system abstraction**: `System.IO.Abstractions` 22.0.14 +- **Testing**: `NUnit` 3.13.2, `Moq` 4.18.2, `AutoFixture` 4.18.1 +- **Code quality**: `StyleCop.Analyzers` 1.1.118, `AsyncFixer` 1.6.0 +- **Package versions**: Centrally managed via `Directory.Packages.props` at the repo root + +## Repository Structure + +``` +/ +├── src/VirtualClient/ +│ ├── VirtualClient.sln # Single solution file +│ ├── VirtualClient.Main/ # Entry point: CLI, command parsing, profiles +│ │ ├── Program.cs # Application entry point +│ │ ├── CommandLineParser.cs # CLI command definitions +│ │ ├── profiles/ # All built-in execution profiles (JSON/YAML) +│ │ └── ... +│ ├── VirtualClient.Contracts/ # Domain model and base classes +│ │ ├── VirtualClientComponent.cs # THE base class for all components +│ │ ├── VirtualClientMonitorComponent.cs # Base for monitors +│ │ ├── ExecutionProfile.cs # Profile model +│ │ ├── Metric.cs # Metric data model +│ │ ├── Exceptions.cs # Exception hierarchy +│ │ ├── Parser/ # TextParser and MetricsParser base classes +│ │ └── Extensibility/ # Data point types for telemetry +│ ├── VirtualClient.Core/ # Orchestration engine +│ │ ├── ProfileExecutor.cs # Runs profiles +│ │ ├── PackageManager.cs # Package download/install +│ │ ├── SystemManagement.cs # OS abstractions (disk, firewall, SSH) +│ │ ├── Components/ # Built-in execution components +│ │ └── Logging/ # Telemetry providers +│ ├── VirtualClient.Common/ # Low-level utilities +│ │ ├── IProcessProxy.cs # Process execution abstraction +│ │ ├── ProcessProxy.cs # Process wrapper with ConcurrentBuffer +│ │ ├── Platform/ # SupportedPlatformsAttribute, etc. +│ │ ├── Telemetry/ # EventContext +│ │ └── Rest/ # HTTP client utilities +│ ├── VirtualClient.Api/ # REST API controllers (client/server coordination) +│ ├── VirtualClient.Actions/ # ~40+ workload executors +│ │ ├── OpenSSL/ # Each subfolder = one workload +│ │ ├── FIO/ +│ │ ├── CoreMark/ +│ │ ├── DiskSpd/ +│ │ ├── Network/NetworkingWorkload/ # Client/server network benchmarks +│ │ └── ... +│ ├── VirtualClient.Dependencies/ # Prerequisite installers (GPU drivers, compilers, disks) +│ ├── VirtualClient.Monitors/ # Background monitors (perf counters, GPU stats) +│ ├── VirtualClient.TestFramework/ # Shared test infrastructure +│ ├── VirtualClient.*.UnitTests/ # Unit test projects +│ ├── VirtualClient.*.FunctionalTests/ # Functional test projects +│ ├── VirtualClient.Examples/ # Extension examples +│ └── VirtualClient.Packaging/ # NuGet packaging +├── website/ # Docusaurus documentation site +├── build.cmd / build.sh # Build scripts +├── build-test.cmd / build-test.sh # Test scripts +├── Directory.Packages.props # Central NuGet version management +├── global.json # .NET SDK version pin +└── VERSION # Build version +``` + +## Architecture Patterns + +### Component Model + +All workloads, dependencies, and monitors inherit from `VirtualClientComponent` (in `VirtualClient.Contracts`). This abstract base class provides: + +- **Constructor signature**: Always `(IServiceCollection dependencies, IDictionary parameters = null)` +- **Lifecycle methods** (called in order by the base `ExecuteAsync`): + 1. `InitializeAsync(EventContext, CancellationToken)` — virtual, setup logic + 2. `Validate()` — virtual, parameter validation + 3. `ExecuteAsync(EventContext, CancellationToken)` — **abstract**, main workload logic + 4. `CleanupAsync(EventContext, CancellationToken)` — virtual, teardown +- **Key properties**: `Platform`, `CpuArchitecture`, `Scenario`, `PackageName`, `Parameters`, `Logger`, `Dependencies`, `MetadataContract` +- Monitors extend `VirtualClientMonitorComponent` which adds `MonitorFrequency`, `MonitorIterations`, `MonitorWarmupPeriod`, `MonitorStrategy` + +### Profile-Driven Execution + +Profiles are JSON or YAML files with three sections: +- **Dependencies**: Installers to run before workloads (package downloads, GPU drivers, disk setup) +- **Actions**: Workload executors to run +- **Monitors**: Background monitoring during execution + +Each section element has a `Type` (C# class name resolved at runtime) and `Parameters` dictionary. Parameter references use `$.Parameters.Name` syntax. Expression placeholders use `{PropertyName}` syntax within command arguments. + +Example profile structure: +```json +{ + "Description": "OpenSSL CPU Performance Workload", + "Metadata": { + "RecommendedMinimumExecutionTime": "01:00:00", + "SupportedPlatforms": "linux-x64,linux-arm64,win-x64" + }, + "Parameters": { + "Duration": "00:01:40" + }, + "Actions": [ + { + "Type": "OpenSslExecutor", + "Parameters": { + "Scenario": "SHA256", + "CommandArguments": "speed -elapsed -seconds {Duration.TotalSeconds} sha256", + "Duration": "$.Parameters.Duration", + "PackageName": "openssl", + "Tags": "CPU,OpenSSL,Cryptography" + } + } + ], + "Dependencies": [ + { + "Type": "DependencyPackageInstallation", + "Parameters": { + "Scenario": "InstallOpenSSLPackage", + "BlobContainer": "packages", + "BlobName": "openssl.3.0.0.zip", + "PackageName": "openssl", + "Extract": true + } + } + ] +} +``` + +### Dependency Injection + +Services are registered in `CommandBase.InitializeDependencies()` and passed as `IServiceCollection` to every component. Components resolve services via extension methods: +```csharp +// Direct resolution +this.fileSystem = dependencies.GetService(); +this.systemManagement = dependencies.GetService(); + +// Safe resolution +if (dependencies.TryGetService(out EnvironmentLayout layout)) { ... } +``` + +Key registered services: `ISystemManagement`, `IFileSystem`, `IDiskManager`, `IFirewallManager`, `IPackageManager`, `IProfileManager`, `IApiClientManager`, `IExpressionEvaluator`, `IEnumerable`, `PlatformSpecifics`, `ILogger`. + +### Process Execution + +External workload binaries are executed through the `IProcessProxy` abstraction (wraps `System.Diagnostics.Process`). Output is captured via `ConcurrentBuffer` for both stdout and stderr. The `ISystemManagement.ProcessManager` creates process proxies. In tests, `InMemoryProcess` is used as a test double. + +### Output Parsing and Metrics + +Each workload has a parser that extracts structured `Metric` objects from raw benchmark output: +- Parsers inherit from `MetricsParser` (which extends `TextParser>`) +- Override `Parse()` (required) and optionally `Preprocess()` for text normalization +- Use regex patterns (defined as `private static readonly Regex`) to extract data +- Use `TextParsingExtensions.Sectionize()` to split output into logical sections +- Return `IList` where each `Metric` has: `Name`, `Value`, `Unit`, `Relativity`, `Tags`, `Metadata` + +Metrics are logged via: +```csharp +this.Logger.LogMetrics("ToolName", scenario, startTime, endTime, metrics, relatedContext: telemetryContext); +``` + +### Client/Server Architecture + +For network and database workloads, VirtualClient supports multi-role execution: +- One instance runs as **server**, another as **client** +- They coordinate via the built-in REST API (`VirtualClient.Api` project) for state synchronization and heartbeat +- Components use `Polly` retry policies for resilience +- The `EnvironmentLayout` defines the topology of instances + +### Error Handling + +Custom exception hierarchy rooted at `VirtualClientException`: +- `WorkloadResultsException` — parsing failures, missing results +- `MonitorException` — monitor failures +- `ApiException` — API communication failures +- `ComponentException` — general component failures +- `StartupException`, `DependencyException`, `ProcessException` + +All exceptions carry an `ErrorReason` enum value (e.g., `WorkloadResultsParsingFailed`, `DependencyNotFound`, `WorkloadFailed`). Error reasons ≥500 are fatal; 400–499 are potentially transient. + +## Coding Standards and Conventions + +### File Header +Every `.cs` file starts with: +```csharp +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +``` + +### Namespace and Using Style +- **Using statements go inside the namespace block** (not at file top) +- Ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*` +- Namespace matches folder structure: `VirtualClient.Actions`, `VirtualClient.Contracts`, etc. + +```csharp +namespace VirtualClient.Actions +{ + using System; + using System.Collections.Generic; + using Microsoft.Extensions.DependencyInjection; + using VirtualClient.Common; + using VirtualClient.Contracts; +``` + +### Naming Conventions +- **Classes**: PascalCase, suffixed by role (`OpenSslExecutor`, `DiskSpdMetricsParser`, `CoreMarkExecutor`) +- **Properties**: PascalCase (`CommandLine`, `MetricScenario`, `MonitorEnabled`) +- **Private fields**: camelCase, no prefix for instance fields, `const` fields use PascalCase + ```csharp + private IFileSystem fileSystem; + private ISystemManagement systemManagement; + private const string CoreMarkOutputFile1 = "run1.log"; + ``` +- **Parameters dictionary keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` +- **Async methods**: Suffixed with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`) +- **Test classes**: `{ComponentName}Tests` (e.g., `FioExecutorTests`, `CoreMarkExecutorTests`) +- **Test methods**: Descriptive with underscores for scenario separation: `FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario` + +### Property Pattern for Profile Parameters +Properties that read from the `Parameters` dictionary follow this pattern: +```csharp +public string CommandArguments +{ + get + { + return this.Parameters.GetValue(nameof(this.CommandArguments)); + } +} + +// With default value: +public string CompilerName +{ + get + { + return this.Parameters.GetValue(nameof(this.CompilerName), string.Empty); + } +} +``` + +### XML Documentation +All public members have XML doc comments using ``, ``, ``, ``, and `` tags: +```csharp +/// +/// Executes the OpenSSL workload. +/// +/// Provides required dependencies to the component. +/// Parameters defined in the profile or supplied on the command line. +``` + +### Platform Support Attribute +Executors declare supported platforms via a class-level attribute: +```csharp +[SupportedPlatforms("linux-arm64,linux-x64,win-x64")] +public class OpenSslExecutor : VirtualClientComponent +``` + +### Code Quality +- **StyleCop.Analyzers** enforces style rules (suppressed: SA1204 static element ordering) +- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming) +- Central package version management prevents version drift across projects + +## Executor Implementation Checklist + +When adding a new workload executor: + +1. Create a subfolder under `VirtualClient.Actions/` named after the workload +2. Create an executor class inheriting `VirtualClientComponent` +3. Add `[SupportedPlatforms("...")]` attribute +4. Define constructor with `(IServiceCollection dependencies, IDictionary parameters)` +5. Expose profile parameters as properties reading from `this.Parameters` +6. Override `InitializeAsync` for setup (locate package, set executable path) +7. Override `ExecuteAsync` for the main workload logic (execute process, capture output, parse, log metrics) +8. Optionally override `CleanupAsync` and `Validate` +9. Create a `MetricsParser` subclass to parse workload output into `IList` +10. Create an execution profile JSON in `VirtualClient.Main/profiles/` +11. Add unit tests inheriting from `MockFixture` in the corresponding `.UnitTests` project +12. Add example output files under `TestResources/` for parser tests + +## Testing Philosophy and Patterns + +### Framework +- **NUnit 3** with `[TestFixture]`, `[Test]`, `[SetUp]`, `[OneTimeSetUp]` attributes +- **Moq** for mocking interfaces +- **AutoFixture** via `MockFixture` base class for test data generation +- Tests are categorized: `[Category("Unit")]` or `[Category("Functional")]` + +### MockFixture Base Class +Test classes inherit from `MockFixture` (in `VirtualClient.TestFramework`), which provides: +- Pre-configured mock services: `ApiClient`, `DiskManager`, `FileSystem`, `File`, `Directory`, `ProcessManager` +- `Setup(PlatformID)` method to configure platform-specific behavior +- `MockFixture.ReadFile(...)` to load example output from `TestResources/Examples/` +- `InMemoryProcess`, `InMemoryFile`, `InMemoryDirectory` test doubles + +### Test Structure Pattern +```csharp +[TestFixture] +[Category("Unit")] +public class MyExecutorTests : MockFixture +{ + private IDictionary profileParameters; + private string mockResults; + + [OneTimeSetUp] + public void SetupFixture() + { + // Load example output files (one-time) + this.mockResults = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "MyWorkload", "Results.json"); + } + + [SetUp] + public void SetupTest() + { + this.Setup(PlatformID.Unix); + // Configure mocks for each test + this.ProcessManager.OnCreateProcess = (command, arguments, workingDir) => + { + return new InMemoryProcess + { + OnHasExited = () => true, + ExitCode = 0, + StartInfo = new ProcessStartInfo { FileName = command, Arguments = arguments }, + StandardOutput = new ConcurrentBuffer(new StringBuilder(this.mockResults)) + }; + }; + } + + [Test] + public async Task MyExecutorExecutesTheExpectedCommandOnLinux() + { + // Arrange, Act, Assert + } +} +``` + +### Parser Tests +Parser tests load real example output (stored in `TestResources/`), run the parser, and assert against expected metric names, values, and units. This ensures parsers remain correct as output formats evolve. + +## Build and Test Commands + +### Build +```bash +# Linux — builds solution (Debug) then publishes self-contained for all platforms (Release) +./build.sh + +# Build for specific platform only +./build.sh --linux-x64 +./build.sh --win-x64 --linux-arm64 + +# Windows +build.cmd +build.cmd --win-x64 +``` + +The build first compiles the solution in Debug configuration (for extension debugging), then publishes runtime-specific self-contained binaries in Release. + +### Test +```bash +# Linux — runs all Unit tests +./build-test.sh + +# Windows — runs Unit + Functional tests +build-test.cmd +``` + +Tests are discovered from `*Tests.csproj` files and filtered by `Category=Unit` (Linux) or `Category=Unit|Category=Functional` (Windows). + +### Direct dotnet commands +```bash +# Build solution only +dotnet build src/VirtualClient/VirtualClient.sln -c Debug + +# Run a specific test project +dotnet test src/VirtualClient/VirtualClient.Actions.UnitTests/VirtualClient.Actions.UnitTests.csproj -c Debug --filter "Category=Unit" + +# Publish for a specific runtime +dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r linux-x64 -c Release --self-contained +``` + +### Version +Build version is read from the `VERSION` file at the repo root. Override with the `VCBuildVersion` environment variable. From eee7f0bcd8653a3293d3aa298f5e60e5c9f9fa32 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 18:33:24 +0000 Subject: [PATCH 2/9] Add file/line citations to copilot-instructions.md and fix inaccuracies - Fix Serilog package name: Serilog.Extensions.Logging (not Serilog) - Fix LogMetrics example to match actual signature from OpenSslExecutor.cs - Fix profile example to use actual content from PERF-CPU-OPENSSL.json - Fix DI service list to match actual CommandBase.cs registrations - Add citation comments throughout for every claim Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/f2cfe3e1-91c6-4cbc-ad70-4a8a1e7af274 Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 218 ++++++++++++++++++++------------ 1 file changed, 138 insertions(+), 80 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 653001b146..44f1086593 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -8,16 +8,17 @@ The application is a .NET CLI tool that reads declarative execution profiles (JS ## Tech Stack and Key Dependencies + - **Runtime**: .NET 9 (SDK 9.0.301 defined in `global.json`, rollForward: feature) -- **Target platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` (self-contained publish) -- **CLI parsing**: `System.CommandLine` (2.0.0-beta1) +- **Target platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` (self-contained publish; see `build.sh` / `build.cmd`) +- **CLI parsing**: `System.CommandLine` 2.0.0-beta1 - **Serialization**: `Newtonsoft.Json` 13.0.3 (JSON), `YamlDotNet` 15.1.1 (YAML profiles) - **Dependency injection**: `Microsoft.Extensions.DependencyInjection` 9.0.9 -- **Logging**: `Serilog` 9.0.2 + `Serilog.Sinks.File` 6.0.0, custom `EventContext` telemetry +- **Logging**: `Serilog.Extensions.Logging` 9.0.2 + `Serilog.Sinks.File` 6.0.0, custom `EventContext` telemetry (`VirtualClient.Common/Telemetry/EventContext.cs`) - **Azure integration**: `Azure.Storage.Blobs` 12.18.0, `Azure.Messaging.EventHubs` 5.11.5, `Azure.Security.KeyVault.*` 4.7.0, `Azure.Identity` 1.16.0 - **HTTP resilience**: `Polly` 8.5.0, `Microsoft.Extensions.Http.Polly` 9.0.9 - **SSH**: `SSH.NET` 2024.2.0 -- **REST API**: ASP.NET Core (built-in API for client/server coordination) +- **REST API**: ASP.NET Core (built-in API for client/server coordination; see `VirtualClient.Api/`) - **File system abstraction**: `System.IO.Abstractions` 22.0.14 - **Testing**: `NUnit` 3.13.2, `Moq` 4.18.2, `AutoFixture` 4.18.1 - **Code quality**: `StyleCop.Analyzers` 1.1.118, `AsyncFixer` 1.6.0 @@ -81,16 +82,17 @@ The application is a .NET CLI tool that reads declarative execution profiles (JS ### Component Model -All workloads, dependencies, and monitors inherit from `VirtualClientComponent` (in `VirtualClient.Contracts`). This abstract base class provides: + +All workloads, dependencies, and monitors inherit from `VirtualClientComponent` (in `VirtualClient.Contracts/VirtualClientComponent.cs`). This abstract base class provides: -- **Constructor signature**: Always `(IServiceCollection dependencies, IDictionary parameters = null)` -- **Lifecycle methods** (called in order by the base `ExecuteAsync`): - 1. `InitializeAsync(EventContext, CancellationToken)` — virtual, setup logic - 2. `Validate()` — virtual, parameter validation - 3. `ExecuteAsync(EventContext, CancellationToken)` — **abstract**, main workload logic - 4. `CleanupAsync(EventContext, CancellationToken)` — virtual, teardown +- **Constructor signature**: Always `(IServiceCollection dependencies, IDictionary parameters = null)` — see line 85 of `VirtualClientComponent.cs` +- **Lifecycle methods** (called in order by the base `ExecuteAsync` at line 713): + 1. `InitializeAsync(EventContext, CancellationToken)` — virtual, setup logic (line 861) + 2. `Validate()` — virtual, parameter validation (line 929) + 3. `ExecuteAsync(EventContext, CancellationToken)` — **abstract**, main workload logic (line 856) + 4. `CleanupAsync(EventContext, CancellationToken)` — virtual, teardown (line 814) - **Key properties**: `Platform`, `CpuArchitecture`, `Scenario`, `PackageName`, `Parameters`, `Logger`, `Dependencies`, `MetadataContract` -- Monitors extend `VirtualClientMonitorComponent` which adds `MonitorFrequency`, `MonitorIterations`, `MonitorWarmupPeriod`, `MonitorStrategy` +- Monitors extend `VirtualClientMonitorComponent` (in `VirtualClient.Contracts/VirtualClientMonitorComponent.cs`) which adds `MonitorFrequency`, `MonitorIterations`, `MonitorWarmupPeriod`, `MonitorStrategy` ### Profile-Driven Execution @@ -101,13 +103,15 @@ Profiles are JSON or YAML files with three sections: Each section element has a `Type` (C# class name resolved at runtime) and `Parameters` dictionary. Parameter references use `$.Parameters.Name` syntax. Expression placeholders use `{PropertyName}` syntax within command arguments. -Example profile structure: + +Example profile structure (abbreviated from `PERF-CPU-OPENSSL.json`): ```json { "Description": "OpenSSL CPU Performance Workload", "Metadata": { "RecommendedMinimumExecutionTime": "01:00:00", - "SupportedPlatforms": "linux-x64,linux-arm64,win-x64" + "SupportedPlatforms": "linux-x64,linux-arm64,win-x64", + "SupportedOperatingSystems": "AzureLinux,CentOS,Debian,RedHat,Suse,Ubuntu,Windows" }, "Parameters": { "Duration": "00:01:40" @@ -116,8 +120,9 @@ Example profile structure: { "Type": "OpenSslExecutor", "Parameters": { - "Scenario": "SHA256", - "CommandArguments": "speed -elapsed -seconds {Duration.TotalSeconds} sha256", + "Scenario": "MD5", + "MetricScenario": "md5", + "CommandArguments": "speed -elapsed -seconds {Duration.TotalSeconds} md5", "Duration": "$.Parameters.Duration", "PackageName": "openssl", "Tags": "CPU,OpenSSL,Cryptography" @@ -141,58 +146,76 @@ Example profile structure: ### Dependency Injection -Services are registered in `CommandBase.InitializeDependencies()` and passed as `IServiceCollection` to every component. Components resolve services via extension methods: + +Services are registered in `CommandBase.InitializeDependencies()` (`VirtualClient.Main/CommandBase.cs:767`) and passed as `IServiceCollection` to every component. Components resolve services via extension methods: ```csharp -// Direct resolution +// Direct resolution (e.g. OpenSslExecutor.cs:48–49) this.fileSystem = dependencies.GetService(); this.systemManagement = dependencies.GetService(); -// Safe resolution +// Safe resolution (e.g. VirtualClientComponent.cs:118) if (dependencies.TryGetService(out EnvironmentLayout layout)) { ... } ``` -Key registered services: `ISystemManagement`, `IFileSystem`, `IDiskManager`, `IFirewallManager`, `IPackageManager`, `IProfileManager`, `IApiClientManager`, `IExpressionEvaluator`, `IEnumerable`, `PlatformSpecifics`, `ILogger`. +Key registered services (from `CommandBase.cs:851–868`): `PlatformSpecifics`, `IApiManager`, `IApiClientManager`, `IConfiguration`, `IDiskManager`, `IExpressionEvaluator`, `IEnumerable`, `IFileSystem`, `IFirewallManager`, `IPackageManager`, `IProfileManager`, `IStateManager`, `ISystemInfo`, `ISystemManagement`, `ProcessManager`, `ILogger`. ### Process Execution -External workload binaries are executed through the `IProcessProxy` abstraction (wraps `System.Diagnostics.Process`). Output is captured via `ConcurrentBuffer` for both stdout and stderr. The `ISystemManagement.ProcessManager` creates process proxies. In tests, `InMemoryProcess` is used as a test double. + +External workload binaries are executed through the `IProcessProxy` abstraction (`VirtualClient.Common/IProcessProxy.cs`), which wraps `System.Diagnostics.Process`. Output is captured via `ConcurrentBuffer` (`VirtualClient.Common/ConcurrentBuffer.cs`) for both `StandardOutput` and `StandardError` (see `ProcessProxy.cs:40–41`). The `ProcessManager` creates process proxies. In tests, `InMemoryProcess` (`VirtualClient.TestFramework/InMemoryProcess.cs`) is used as a test double. ### Output Parsing and Metrics Each workload has a parser that extracts structured `Metric` objects from raw benchmark output: -- Parsers inherit from `MetricsParser` (which extends `TextParser>`) -- Override `Parse()` (required) and optionally `Preprocess()` for text normalization -- Use regex patterns (defined as `private static readonly Regex`) to extract data -- Use `TextParsingExtensions.Sectionize()` to split output into logical sections -- Return `IList` where each `Metric` has: `Name`, `Value`, `Unit`, `Relativity`, `Tags`, `Metadata` - -Metrics are logged via: + +- Parsers inherit from `MetricsParser` (`VirtualClient.Contracts/Parser/MetricsParser.cs:15`) which extends `TextParser>` (`VirtualClient.Contracts/Parser/TextParser.cs:12`) +- Override `Parse()` (required, abstract at `TextParser.cs:48`) and optionally `Preprocess()` for text normalization (`TextParser.cs:53`) +- Use regex patterns defined as `private static readonly Regex` (e.g., `DiskSpdMetricsParser.cs:24–34`) +- Use `TextParsingExtensions.Sectionize()` to split output into logical sections (`VirtualClient.Contracts/Parser/TextParsingExtensions.cs:92`) +- Return `IList` where each `Metric` has: `Name`, `Value`, `Unit`, `Relativity`, `Tags`, `Metadata` (see `VirtualClient.Contracts/Metric.cs`) + + +Metrics are logged via (from `OpenSslExecutor.cs:225`): ```csharp -this.Logger.LogMetrics("ToolName", scenario, startTime, endTime, metrics, relatedContext: telemetryContext); +this.Logger.LogMetrics( + "OpenSSL", + this.MetricScenario ?? this.Scenario, + workloadProcess.StartTime, + workloadProcess.ExitTime, + metrics, + null, + commandArguments, + this.Tags, + telemetryContext); ``` ### Client/Server Architecture + For network and database workloads, VirtualClient supports multi-role execution: -- One instance runs as **server**, another as **client** -- They coordinate via the built-in REST API (`VirtualClient.Api` project) for state synchronization and heartbeat -- Components use `Polly` retry policies for resilience -- The `EnvironmentLayout` defines the topology of instances +- One instance runs as **server**, another as **client** (e.g., `NetworkingWorkloadExecutor.cs`) +- They coordinate via the built-in REST API (`VirtualClient.Api/` project) for state synchronization and heartbeat +- Components use `Polly` retry policies for resilience (e.g., `NetworkingWorkloadExecutor.cs:49`) +- The `EnvironmentLayout` (`VirtualClient.Contracts/EnvironmentLayout.cs`) defines the topology of instances ### Error Handling -Custom exception hierarchy rooted at `VirtualClientException`: -- `WorkloadResultsException` — parsing failures, missing results -- `MonitorException` — monitor failures -- `ApiException` — API communication failures -- `ComponentException` — general component failures -- `StartupException`, `DependencyException`, `ProcessException` + +Custom exception hierarchy rooted at `VirtualClientException` (`Exceptions.cs:12`): +- `ApiException` (`Exceptions.cs:78`) — API communication failures +- `ComponentException` (`Exceptions.cs:137`) — general component failures +- `MonitorException` (`Exceptions.cs:196`) — monitor failures +- `WorkloadResultsException` (`Exceptions.cs:373`) — parsing failures, missing results +- `DependencyException` (`Exceptions.cs:432`) — dependency resolution failures +- `ProcessException` (`Exceptions.cs:491`) — process execution failures +- `StartupException` (`Exceptions.cs:549`) — startup/initialization failures -All exceptions carry an `ErrorReason` enum value (e.g., `WorkloadResultsParsingFailed`, `DependencyNotFound`, `WorkloadFailed`). Error reasons ≥500 are fatal; 400–499 are potentially transient. +All exceptions carry an `ErrorReason` enum value (`Enumerations.cs:37`). Error reasons ≥500 are fatal (e.g., `ProfileNotFound = 500`); 400–499 are potentially transient (e.g., `InvalidResults = 400`). See the comment at `Enumerations.cs:115` and `Enumerations.cs:153`. ## Coding Standards and Conventions ### File Header + Every `.cs` file starts with: ```csharp // Copyright (c) Microsoft Corporation. @@ -200,46 +223,64 @@ Every `.cs` file starts with: ``` ### Namespace and Using Style + - **Using statements go inside the namespace block** (not at file top) - Ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*` - Namespace matches folder structure: `VirtualClient.Actions`, `VirtualClient.Contracts`, etc. ```csharp +// From OpenSslExecutor.cs:4–21 namespace VirtualClient.Actions { using System; using System.Collections.Generic; + using System.Diagnostics; + using System.IO.Abstractions; + using System.Runtime.InteropServices; + using System.Threading; + using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; + using Microsoft.Extensions.Logging; using VirtualClient.Common; + using VirtualClient.Common.Extensions; + using VirtualClient.Common.Platform; + using VirtualClient.Common.Telemetry; using VirtualClient.Contracts; + using VirtualClient.Contracts.Metadata; ``` ### Naming Conventions -- **Classes**: PascalCase, suffixed by role (`OpenSslExecutor`, `DiskSpdMetricsParser`, `CoreMarkExecutor`) -- **Properties**: PascalCase (`CommandLine`, `MetricScenario`, `MonitorEnabled`) -- **Private fields**: camelCase, no prefix for instance fields, `const` fields use PascalCase + +- **Classes**: PascalCase, suffixed by role (e.g., `OpenSslExecutor` in `OpenSSL/OpenSslExecutor.cs`, `DiskSpdMetricsParser` in `DiskSpd/DiskSpdMetricsParser.cs`, `CoreMarkExecutor` in `CoreMark/CoreMarkExecutor.cs`) +- **Properties**: PascalCase (e.g., `CommandLine`, `MetricScenario`, `MonitorEnabled`) +- **Private fields**: camelCase, no prefix for instance fields; `const` fields use PascalCase ```csharp + // From OpenSslExecutor.cs:37–38 private IFileSystem fileSystem; private ISystemManagement systemManagement; + // From CoreMarkExecutor.cs:28–29 private const string CoreMarkOutputFile1 = "run1.log"; + private const string CoreMarkOutputFile2 = "run2.log"; ``` -- **Parameters dictionary keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` +- **Parameters dictionary keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` (see `VirtualClientComponent.cs:92`) - **Async methods**: Suffixed with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`) -- **Test classes**: `{ComponentName}Tests` (e.g., `FioExecutorTests`, `CoreMarkExecutorTests`) -- **Test methods**: Descriptive with underscores for scenario separation: `FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario` +- **Test classes**: `{ComponentName}Tests` (e.g., `FioExecutorTests` in `VirtualClient.Actions.UnitTests/FIO/FioExecutorTests.cs:24`) +- **Test methods**: Descriptive with underscores for scenario separation (e.g., `FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario` at `FioExecutorTests.cs:80`) ### Property Pattern for Profile Parameters + Properties that read from the `Parameters` dictionary follow this pattern: ```csharp +// From OpenSslExecutor.cs:55–61 public string CommandArguments { get { - return this.Parameters.GetValue(nameof(this.CommandArguments)); + return this.Parameters.GetValue(nameof(OpenSslExecutor.CommandArguments)); } } -// With default value: +// With default value (from CoreMarkExecutor.cs:49–55): public string CompilerName { get @@ -250,64 +291,73 @@ public string CompilerName ``` ### XML Documentation + All public members have XML doc comments using ``, ``, ``, ``, and `` tags: ```csharp +// From OpenSslExecutor.cs:40–44 /// -/// Executes the OpenSSL workload. +/// Constructor /// /// Provides required dependencies to the component. /// Parameters defined in the profile or supplied on the command line. ``` ### Platform Support Attribute + Executors declare supported platforms via a class-level attribute: ```csharp +// From OpenSslExecutor.cs:34–35 [SupportedPlatforms("linux-arm64,linux-x64,win-x64")] public class OpenSslExecutor : VirtualClientComponent ``` ### Code Quality -- **StyleCop.Analyzers** enforces style rules (suppressed: SA1204 static element ordering) -- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming) -- Central package version management prevents version drift across projects + +- **StyleCop.Analyzers** enforces style rules (suppressed: SA1204 static element ordering — `.editorconfig:3–4`) +- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming — `.editorconfig:6–7`) +- Central package version management (`Directory.Packages.props`) prevents version drift across projects ## Executor Implementation Checklist -When adding a new workload executor: +When adding a new workload executor (pattern observed in `OpenSSL/OpenSslExecutor.cs`, `CoreMark/CoreMarkExecutor.cs`, `DiskSpd/DiskSpdExecutor.cs`): 1. Create a subfolder under `VirtualClient.Actions/` named after the workload -2. Create an executor class inheriting `VirtualClientComponent` -3. Add `[SupportedPlatforms("...")]` attribute +2. Create an executor class inheriting `VirtualClientComponent` (`VirtualClient.Contracts/VirtualClientComponent.cs`) +3. Add `[SupportedPlatforms("...")]` attribute (`VirtualClient.Common/Platform/SupportedPlatformsAttribute.cs`) 4. Define constructor with `(IServiceCollection dependencies, IDictionary parameters)` -5. Expose profile parameters as properties reading from `this.Parameters` +5. Expose profile parameters as properties reading from `this.Parameters` (e.g., `OpenSslExecutor.cs:55–61`) 6. Override `InitializeAsync` for setup (locate package, set executable path) 7. Override `ExecuteAsync` for the main workload logic (execute process, capture output, parse, log metrics) 8. Optionally override `CleanupAsync` and `Validate` -9. Create a `MetricsParser` subclass to parse workload output into `IList` -10. Create an execution profile JSON in `VirtualClient.Main/profiles/` -11. Add unit tests inheriting from `MockFixture` in the corresponding `.UnitTests` project -12. Add example output files under `TestResources/` for parser tests +9. Create a `MetricsParser` subclass to parse workload output into `IList` (e.g., `DiskSpd/DiskSpdMetricsParser.cs`) +10. Create an execution profile JSON in `VirtualClient.Main/profiles/` (e.g., `PERF-CPU-OPENSSL.json`) +11. Add unit tests inheriting from `MockFixture` in the corresponding `.UnitTests` project (e.g., `FIO/FioExecutorTests.cs`) +12. Add example output files under `Examples/` for parser tests ## Testing Philosophy and Patterns ### Framework -- **NUnit 3** with `[TestFixture]`, `[Test]`, `[SetUp]`, `[OneTimeSetUp]` attributes -- **Moq** for mocking interfaces -- **AutoFixture** via `MockFixture` base class for test data generation -- Tests are categorized: `[Category("Unit")]` or `[Category("Functional")]` + +- **NUnit 3** with `[TestFixture]`, `[Test]`, `[SetUp]`, `[OneTimeSetUp]` attributes (e.g., `FioExecutorTests.cs:22–23`) +- **Moq** for mocking interfaces (e.g., `FioExecutorTests.cs:57`) +- **AutoFixture** via `MockFixture` base class for test data generation (`MockFixture.cs:35` extends `Fixture`) +- Tests are categorized: `[Category("Unit")]` (e.g., `FioExecutorTests.cs:23`) or `[Category("Functional")]` ### MockFixture Base Class -Test classes inherit from `MockFixture` (in `VirtualClient.TestFramework`), which provides: -- Pre-configured mock services: `ApiClient`, `DiskManager`, `FileSystem`, `File`, `Directory`, `ProcessManager` -- `Setup(PlatformID)` method to configure platform-specific behavior -- `MockFixture.ReadFile(...)` to load example output from `TestResources/Examples/` -- `InMemoryProcess`, `InMemoryFile`, `InMemoryDirectory` test doubles + +Test classes inherit from `MockFixture` (in `VirtualClient.TestFramework/MockFixture.cs:35`), which provides: +- Pre-configured mock services: `ApiClient` (line 87), `DiskManager` (line 141), `FileSystem` (line 156), `File` (line 161), `Directory` (line 146), `ProcessManager` (line 235) +- `Setup(PlatformID platform, Architecture architecture = ...)` method to configure platform-specific behavior (line 466) +- `MockFixture.ReadFile(...)` to load example output from `Examples/` directory (line 278) +- `InMemoryProcess` (`InMemoryProcess.cs:20`), `InMemoryFile` (`InMemoryFile.cs:17`), `InMemoryDirectory` (`InMemoryDirectory.cs:13`) test doubles ### Test Structure Pattern + ```csharp +// From FioExecutorTests.cs [TestFixture] [Category("Unit")] -public class MyExecutorTests : MockFixture +public class FioExecutorTests : MockFixture { private IDictionary profileParameters; private string mockResults; @@ -315,29 +365,33 @@ public class MyExecutorTests : MockFixture [OneTimeSetUp] public void SetupFixture() { - // Load example output files (one-time) - this.mockResults = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "MyWorkload", "Results.json"); + this.mockResults = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "FIO", "Results_FIO.json"); } [SetUp] public void SetupTest() { this.Setup(PlatformID.Unix); - // Configure mocks for each test + // ... this.ProcessManager.OnCreateProcess = (command, arguments, workingDir) => { return new InMemoryProcess { OnHasExited = () => true, ExitCode = 0, - StartInfo = new ProcessStartInfo { FileName = command, Arguments = arguments }, + StartInfo = new ProcessStartInfo + { + FileName = command, + Arguments = arguments, + WorkingDirectory = workingDir + }, StandardOutput = new ConcurrentBuffer(new StringBuilder(this.mockResults)) }; }; } [Test] - public async Task MyExecutorExecutesTheExpectedCommandOnLinux() + public void FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario() { // Arrange, Act, Assert } @@ -345,16 +399,18 @@ public class MyExecutorTests : MockFixture ``` ### Parser Tests -Parser tests load real example output (stored in `TestResources/`), run the parser, and assert against expected metric names, values, and units. This ensures parsers remain correct as output formats evolve. + +Parser tests load real example output (stored in `Examples/` or `TestResources/` under test projects), run the parser, and assert against expected metric names, values, and units. This ensures parsers remain correct as output formats evolve. ## Build and Test Commands ### Build + ```bash # Linux — builds solution (Debug) then publishes self-contained for all platforms (Release) ./build.sh -# Build for specific platform only +# Build for specific platform only (see build.sh for all options) ./build.sh --linux-x64 ./build.sh --win-x64 --linux-arm64 @@ -363,14 +419,15 @@ build.cmd build.cmd --win-x64 ``` -The build first compiles the solution in Debug configuration (for extension debugging), then publishes runtime-specific self-contained binaries in Release. +The build first compiles the solution in Debug configuration (for extension debugging — see comment in `build.sh:124–126` and `build.cmd:69–74`), then publishes runtime-specific self-contained binaries in Release. ### Test + ```bash -# Linux — runs all Unit tests +# Linux — runs Unit tests only (build-test.sh:65 filters Category=Unit) ./build-test.sh -# Windows — runs Unit + Functional tests +# Windows — runs Unit + Functional tests (build-test.cmd:23 filters Category=Unit|Category=Functional) build-test.cmd ``` @@ -389,4 +446,5 @@ dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r ``` ### Version + Build version is read from the `VERSION` file at the repo root. Override with the `VCBuildVersion` environment variable. From dbd9b6ec09f86e44ad0a2d2128ab4f478b29d782 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 22:25:47 +0000 Subject: [PATCH 3/9] Add PR review guidelines section to copilot-instructions.md Add a new 'PR Review Guidelines' section with two subsections: - Required Fixes (10 items): things that break the build or crash at runtime, including constructor signature, base class inheritance, assembly attribute, profile Type matching, parser hierarchy, central package management, using placement, exception hierarchy, test category attributes, and copyright header. - Suggestions (9 items): convention inconsistencies that won't break but should be addressed, including parameter access pattern, platform attribute, MockFixture inheritance, using ordering, field naming, XML docs, parser test data, Validate pattern, and async naming. Every rule is cited to specific files and line numbers in the repo. Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/7e265018-d4c1-422a-8fc9-d7d0b387a0cd Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 84 +++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 44f1086593..ec4a26752b 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -448,3 +448,87 @@ dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r ### Version Build version is read from the `VERSION` file at the repo root. Override with the `VCBuildVersion` environment variable. + +## PR Review Guidelines + +When reviewing pull requests, flag items below as **Required Fix** (will break the build, crash at runtime, or violate a hard architectural constraint) or **Suggestion** (inconsistency with conventions that won't break anything but should be addressed). + +### Required Fixes (flag these — they break things) + +1. **Component must inherit `VirtualClientComponent`.** + Any class referenced by a profile `"Type"` field is resolved via `ComponentTypeCache` and instantiated by `ComponentFactory.CreateComponent` using `Activator.CreateInstance` — which casts the result to `VirtualClientComponent`. A class that does not inherit this base type will throw `InvalidCastException` → `StartupException` at runtime. + + +2. **Component constructor must be `(IServiceCollection, IDictionary)`.** + `ComponentFactory` calls `Activator.CreateInstance(componentType, dependencies, effectiveParameters)` which requires exactly this two-parameter constructor signature. A missing or differently-typed constructor throws `MissingMethodException` → `StartupException`. + + +3. **Assembly containing new components must have `[assembly: VirtualClientComponentAssembly]`.** + `ComponentTypeCache.IsComponentAssembly()` checks for this attribute; assemblies without it are skipped during type discovery. A new executor in an un-attributed assembly will cause `TypeLoadException` at profile load time. + + +4. **Profile `"Type"` value must exactly match the C# class name.** + The `ComponentTypeCache.TryGetComponentType` lookup matches on type name. A typo or wrong name throws `TypeLoadException` → `StartupException` with the message "does not exist or does not inherit from VirtualClientComponent". + + +5. **Parser must extend `MetricsParser` (which extends `TextParser>`) and implement `Parse()`.** + `Parse()` is abstract in `TextParser` — failing to override it is a compile error. Using a return type other than `IList` will break the `LogMetrics` call which iterates over the result. + + +6. **NuGet package versions must be in `Directory.Packages.props`, not in individual `.csproj` files.** + The repo uses central package management. Adding a `Version=` attribute in a `.csproj` `` will cause a build error (`NU1008`) because it conflicts with centrally-managed versions. + + +7. **`using` statements must be inside the `namespace` block.** + StyleCop.Analyzers (enabled repo-wide) enforces `SA1200` — `using` directives outside a namespace cause build warnings treated as errors in CI. + + +8. **Exception types must use the project's exception hierarchy.** + Throwing raw `Exception` or `InvalidOperationException` instead of the established types (`WorkloadException`, `DependencyException`, `ProcessException`, `MonitorException`, etc.) breaks the error-handling pipeline that routes on `ErrorReason`. The `Validate()` pattern in existing executors consistently throws `WorkloadException` with `ErrorReason.InvalidProfileDefinition`. + + +9. **Test classes must have `[TestFixture]` and `[Category("Unit")]` (or `"Functional"`).** + The build scripts filter tests by category (`--filter "Category=Unit"`). Tests without a `[Category]` attribute are silently skipped by the CI pipeline and will never run. + + +10. **Copyright header must be present on every `.cs` file.** + StyleCop rule `SA1633` requires the standard two-line header. Missing it will fail the StyleCop check. + + +### Suggestions (flag these — won't break but inconsistent) + +1. **Prefer `this.Parameters.GetValue(nameof(...))` for profile parameters.** + All existing executors expose parameters as properties backed by `this.Parameters.GetValue()` using `nameof()` for the key. Direct dictionary access (`this.Parameters["Key"]`) works but is inconsistent with the established pattern and loses the case-insensitive, typed lookup. + + +2. **Add `[SupportedPlatforms("...")]` attribute to executor classes.** + While not strictly required (the base class handles missing attributes gracefully), every existing executor uses this attribute. Omitting it means the workload will attempt to run on all platforms, which is rarely correct. + + +3. **Test classes should inherit `MockFixture`, not create mocks from scratch.** + `MockFixture` pre-configures all the standard service mocks (`FileSystem`, `DiskManager`, `ProcessManager`, etc.) and provides helpers like `ReadFile()` and `Setup(PlatformID)`. Manually recreating these mocks is verbose and diverges from the pattern used by all existing tests. + + +4. **Using-statement ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*`.** + StyleCop `SA1208`/`SA1210` enforce alphabetical ordering within groups. While reordering won't break the build (it's a warning, not an error), it's inconsistent with every file in the codebase. + + +5. **Private instance fields should use camelCase with no prefix.** + The codebase uses `private IFileSystem fileSystem;` not `_fileSystem` or `m_fileSystem`. Using a different convention won't break anything but creates visual inconsistency. + + +6. **XML doc comments on all public members.** + Existing code consistently documents every public method, property, and constructor with ``, ``, and `` tags. Missing docs won't fail the build (XML doc warnings are not currently treated as errors) but diverges from project norms. + + +7. **Parser tests should load real example output from `Examples/` directories.** + Parser tests in the codebase use `MockFixture.ReadFile(MockFixture.ExamplesDirectory, ...)` to load actual benchmark output. Inline test strings work but don't verify against real-world output formats. + + +8. **`Validate()` should check required parameters and throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.** + This is the established validation pattern. While not enforced by the compiler, skipping validation means invalid profiles fail later with obscure errors instead of clear messages at startup. + + +9. **Async methods should be suffixed with `Async`.** + The `AZCA1002` analyzer rule is suppressed (`.editorconfig:6–7`), so this won't break the build. However, every existing method follows the `Async` suffix convention (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`). + From 562e075dd0134e0e200f3016a41edb74c9fc7d2d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 22:58:48 +0000 Subject: [PATCH 4/9] Refactor copilot-instructions.md into focused instruction files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the 535-line monolithic file into 9 focused files, each ≤100 lines: - .github/copilot-instructions.md (73 lines) — overview, tech stack, architecture, build/test - .github/instructions/csharp.instructions.md (88 lines) — C# coding standards - .github/instructions/testing.instructions.md (77 lines) — test patterns - .github/instructions/documentation.instructions.md (37 lines) — doc formatting - .github/instructions/pr-review.instructions.md (59 lines) — PR review rules - .github/instructions/profile-review.instructions.md (64 lines) — profile rules - .github/instructions/workload-development.instructions.md (80 lines) — workload dev - .github/instructions/MetricsParser.instructions.md (95 lines) — parser patterns - .github/instructions/ClientServer-Workloads.instructions.md (90 lines) — client/server Each file has proper frontmatter with applyTo glob and description. No content is duplicated across files. Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/284221cb-f2b7-47a2-9538-840584055261 Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 541 ++---------------- .../ClientServer-Workloads.instructions.md | 90 +++ .../MetricsParser.instructions.md | 95 +++ .github/instructions/csharp.instructions.md | 88 +++ .../documentation.instructions.md | 37 ++ .../instructions/pr-review.instructions.md | 59 ++ .../profile-review.instructions.md | 64 +++ .github/instructions/testing.instructions.md | 77 +++ .../workload-development.instructions.md | 80 +++ 9 files changed, 630 insertions(+), 501 deletions(-) create mode 100644 .github/instructions/ClientServer-Workloads.instructions.md create mode 100644 .github/instructions/MetricsParser.instructions.md create mode 100644 .github/instructions/csharp.instructions.md create mode 100644 .github/instructions/documentation.instructions.md create mode 100644 .github/instructions/pr-review.instructions.md create mode 100644 .github/instructions/profile-review.instructions.md create mode 100644 .github/instructions/testing.instructions.md create mode 100644 .github/instructions/workload-development.instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ec4a26752b..edc01915ba 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -2,533 +2,72 @@ ## Project Overview -VirtualClient is a cross-platform benchmarking and workload execution framework built by Microsoft. It provides a standardized, profile-driven way to run performance benchmarks, stress tests, and qualification workloads on systems (particularly Azure VMs) and collect structured metrics. It supports Windows and Linux on x64 and ARM64 architectures. +VirtualClient is a cross-platform benchmarking framework by Microsoft. It runs profile-driven performance +benchmarks, stress tests, and qualification workloads on systems (particularly Azure VMs), collecting +structured metrics. Supports Windows and Linux on x64 and ARM64. -The application is a .NET CLI tool that reads declarative execution profiles (JSON or YAML), installs dependencies, runs workload executors, collects metrics via parsers, and optionally emits telemetry to Azure services. +## Tech Stack -## Tech Stack and Key Dependencies - - -- **Runtime**: .NET 9 (SDK 9.0.301 defined in `global.json`, rollForward: feature) -- **Target platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` (self-contained publish; see `build.sh` / `build.cmd`) -- **CLI parsing**: `System.CommandLine` 2.0.0-beta1 -- **Serialization**: `Newtonsoft.Json` 13.0.3 (JSON), `YamlDotNet` 15.1.1 (YAML profiles) -- **Dependency injection**: `Microsoft.Extensions.DependencyInjection` 9.0.9 -- **Logging**: `Serilog.Extensions.Logging` 9.0.2 + `Serilog.Sinks.File` 6.0.0, custom `EventContext` telemetry (`VirtualClient.Common/Telemetry/EventContext.cs`) -- **Azure integration**: `Azure.Storage.Blobs` 12.18.0, `Azure.Messaging.EventHubs` 5.11.5, `Azure.Security.KeyVault.*` 4.7.0, `Azure.Identity` 1.16.0 -- **HTTP resilience**: `Polly` 8.5.0, `Microsoft.Extensions.Http.Polly` 9.0.9 -- **SSH**: `SSH.NET` 2024.2.0 -- **REST API**: ASP.NET Core (built-in API for client/server coordination; see `VirtualClient.Api/`) -- **File system abstraction**: `System.IO.Abstractions` 22.0.14 -- **Testing**: `NUnit` 3.13.2, `Moq` 4.18.2, `AutoFixture` 4.18.1 -- **Code quality**: `StyleCop.Analyzers` 1.1.118, `AsyncFixer` 1.6.0 -- **Package versions**: Centrally managed via `Directory.Packages.props` at the repo root +- **Runtime**: .NET 9 (SDK 9.0.301, `global.json`) +- **Platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` +- **Serialization**: `Newtonsoft.Json` 13.0.3, `YamlDotNet` 15.1.1 +- **DI**: `Microsoft.Extensions.DependencyInjection` 9.0.9 +- **Logging**: `Serilog.Extensions.Logging` 9.0.2 (NOT `Serilog`) +- **Testing**: NUnit 3.13.2, Moq 4.18.2, AutoFixture 4.18.1 +- **Code quality**: StyleCop.Analyzers 1.1.118, AsyncFixer 1.6.0 +- **Package versions**: Centrally managed via `Directory.Packages.props` ## Repository Structure ``` -/ -├── src/VirtualClient/ -│ ├── VirtualClient.sln # Single solution file -│ ├── VirtualClient.Main/ # Entry point: CLI, command parsing, profiles -│ │ ├── Program.cs # Application entry point -│ │ ├── CommandLineParser.cs # CLI command definitions -│ │ ├── profiles/ # All built-in execution profiles (JSON/YAML) -│ │ └── ... -│ ├── VirtualClient.Contracts/ # Domain model and base classes -│ │ ├── VirtualClientComponent.cs # THE base class for all components -│ │ ├── VirtualClientMonitorComponent.cs # Base for monitors -│ │ ├── ExecutionProfile.cs # Profile model -│ │ ├── Metric.cs # Metric data model -│ │ ├── Exceptions.cs # Exception hierarchy -│ │ ├── Parser/ # TextParser and MetricsParser base classes -│ │ └── Extensibility/ # Data point types for telemetry -│ ├── VirtualClient.Core/ # Orchestration engine -│ │ ├── ProfileExecutor.cs # Runs profiles -│ │ ├── PackageManager.cs # Package download/install -│ │ ├── SystemManagement.cs # OS abstractions (disk, firewall, SSH) -│ │ ├── Components/ # Built-in execution components -│ │ └── Logging/ # Telemetry providers -│ ├── VirtualClient.Common/ # Low-level utilities -│ │ ├── IProcessProxy.cs # Process execution abstraction -│ │ ├── ProcessProxy.cs # Process wrapper with ConcurrentBuffer -│ │ ├── Platform/ # SupportedPlatformsAttribute, etc. -│ │ ├── Telemetry/ # EventContext -│ │ └── Rest/ # HTTP client utilities -│ ├── VirtualClient.Api/ # REST API controllers (client/server coordination) -│ ├── VirtualClient.Actions/ # ~40+ workload executors -│ │ ├── OpenSSL/ # Each subfolder = one workload -│ │ ├── FIO/ -│ │ ├── CoreMark/ -│ │ ├── DiskSpd/ -│ │ ├── Network/NetworkingWorkload/ # Client/server network benchmarks -│ │ └── ... -│ ├── VirtualClient.Dependencies/ # Prerequisite installers (GPU drivers, compilers, disks) -│ ├── VirtualClient.Monitors/ # Background monitors (perf counters, GPU stats) -│ ├── VirtualClient.TestFramework/ # Shared test infrastructure -│ ├── VirtualClient.*.UnitTests/ # Unit test projects -│ ├── VirtualClient.*.FunctionalTests/ # Functional test projects -│ ├── VirtualClient.Examples/ # Extension examples -│ └── VirtualClient.Packaging/ # NuGet packaging -├── website/ # Docusaurus documentation site -├── build.cmd / build.sh # Build scripts -├── build-test.cmd / build-test.sh # Test scripts -├── Directory.Packages.props # Central NuGet version management -├── global.json # .NET SDK version pin -└── VERSION # Build version +src/VirtualClient/ +├── VirtualClient.Main/ # CLI entry point, profiles/ +├── VirtualClient.Contracts/ # Base classes (VirtualClientComponent), Metric, Parser/ +├── VirtualClient.Core/ # ProfileExecutor, PackageManager, SystemManagement +├── VirtualClient.Common/ # IProcessProxy, ConcurrentBuffer, Telemetry/EventContext +├── VirtualClient.Api/ # REST API for client/server coordination +├── VirtualClient.Actions/ # ~40+ workload executors (OpenSSL/, FIO/, DiskSpd/, ...) +├── VirtualClient.Dependencies/ # Prerequisite installers +├── VirtualClient.Monitors/ # Background monitors +├── VirtualClient.TestFramework/ # MockFixture, InMemoryProcess, test doubles +└── VirtualClient.*.UnitTests/ # Unit test projects ``` ## Architecture Patterns ### Component Model - -All workloads, dependencies, and monitors inherit from `VirtualClientComponent` (in `VirtualClient.Contracts/VirtualClientComponent.cs`). This abstract base class provides: - -- **Constructor signature**: Always `(IServiceCollection dependencies, IDictionary parameters = null)` — see line 85 of `VirtualClientComponent.cs` -- **Lifecycle methods** (called in order by the base `ExecuteAsync` at line 713): - 1. `InitializeAsync(EventContext, CancellationToken)` — virtual, setup logic (line 861) - 2. `Validate()` — virtual, parameter validation (line 929) - 3. `ExecuteAsync(EventContext, CancellationToken)` — **abstract**, main workload logic (line 856) - 4. `CleanupAsync(EventContext, CancellationToken)` — virtual, teardown (line 814) -- **Key properties**: `Platform`, `CpuArchitecture`, `Scenario`, `PackageName`, `Parameters`, `Logger`, `Dependencies`, `MetadataContract` -- Monitors extend `VirtualClientMonitorComponent` (in `VirtualClient.Contracts/VirtualClientMonitorComponent.cs`) which adds `MonitorFrequency`, `MonitorIterations`, `MonitorWarmupPeriod`, `MonitorStrategy` - -### Profile-Driven Execution - -Profiles are JSON or YAML files with three sections: -- **Dependencies**: Installers to run before workloads (package downloads, GPU drivers, disk setup) -- **Actions**: Workload executors to run -- **Monitors**: Background monitoring during execution - -Each section element has a `Type` (C# class name resolved at runtime) and `Parameters` dictionary. Parameter references use `$.Parameters.Name` syntax. Expression placeholders use `{PropertyName}` syntax within command arguments. - - -Example profile structure (abbreviated from `PERF-CPU-OPENSSL.json`): -```json -{ - "Description": "OpenSSL CPU Performance Workload", - "Metadata": { - "RecommendedMinimumExecutionTime": "01:00:00", - "SupportedPlatforms": "linux-x64,linux-arm64,win-x64", - "SupportedOperatingSystems": "AzureLinux,CentOS,Debian,RedHat,Suse,Ubuntu,Windows" - }, - "Parameters": { - "Duration": "00:01:40" - }, - "Actions": [ - { - "Type": "OpenSslExecutor", - "Parameters": { - "Scenario": "MD5", - "MetricScenario": "md5", - "CommandArguments": "speed -elapsed -seconds {Duration.TotalSeconds} md5", - "Duration": "$.Parameters.Duration", - "PackageName": "openssl", - "Tags": "CPU,OpenSSL,Cryptography" - } - } - ], - "Dependencies": [ - { - "Type": "DependencyPackageInstallation", - "Parameters": { - "Scenario": "InstallOpenSSLPackage", - "BlobContainer": "packages", - "BlobName": "openssl.3.0.0.zip", - "PackageName": "openssl", - "Extract": true - } - } - ] -} -``` +All workloads, dependencies, monitors inherit `VirtualClientComponent`. +Constructor: `(IServiceCollection, IDictionary)`. +Lifecycle: `InitializeAsync` → `Validate` → `ExecuteAsync` → `CleanupAsync`. ### Dependency Injection - -Services are registered in `CommandBase.InitializeDependencies()` (`VirtualClient.Main/CommandBase.cs:767`) and passed as `IServiceCollection` to every component. Components resolve services via extension methods: -```csharp -// Direct resolution (e.g. OpenSslExecutor.cs:48–49) -this.fileSystem = dependencies.GetService(); -this.systemManagement = dependencies.GetService(); - -// Safe resolution (e.g. VirtualClientComponent.cs:118) -if (dependencies.TryGetService(out EnvironmentLayout layout)) { ... } -``` - -Key registered services (from `CommandBase.cs:851–868`): `PlatformSpecifics`, `IApiManager`, `IApiClientManager`, `IConfiguration`, `IDiskManager`, `IExpressionEvaluator`, `IEnumerable`, `IFileSystem`, `IFirewallManager`, `IPackageManager`, `IProfileManager`, `IStateManager`, `ISystemInfo`, `ISystemManagement`, `ProcessManager`, `ILogger`. +Services registered in `CommandBase.InitializeDependencies()`, resolved via `dependencies.GetService()`. +Key services: `IFileSystem`, `ISystemManagement`, `ProcessManager`, `IPackageManager`, `ILogger`. ### Process Execution - -External workload binaries are executed through the `IProcessProxy` abstraction (`VirtualClient.Common/IProcessProxy.cs`), which wraps `System.Diagnostics.Process`. Output is captured via `ConcurrentBuffer` (`VirtualClient.Common/ConcurrentBuffer.cs`) for both `StandardOutput` and `StandardError` (see `ProcessProxy.cs:40–41`). The `ProcessManager` creates process proxies. In tests, `InMemoryProcess` (`VirtualClient.TestFramework/InMemoryProcess.cs`) is used as a test double. - -### Output Parsing and Metrics - -Each workload has a parser that extracts structured `Metric` objects from raw benchmark output: - -- Parsers inherit from `MetricsParser` (`VirtualClient.Contracts/Parser/MetricsParser.cs:15`) which extends `TextParser>` (`VirtualClient.Contracts/Parser/TextParser.cs:12`) -- Override `Parse()` (required, abstract at `TextParser.cs:48`) and optionally `Preprocess()` for text normalization (`TextParser.cs:53`) -- Use regex patterns defined as `private static readonly Regex` (e.g., `DiskSpdMetricsParser.cs:24–34`) -- Use `TextParsingExtensions.Sectionize()` to split output into logical sections (`VirtualClient.Contracts/Parser/TextParsingExtensions.cs:92`) -- Return `IList` where each `Metric` has: `Name`, `Value`, `Unit`, `Relativity`, `Tags`, `Metadata` (see `VirtualClient.Contracts/Metric.cs`) - - -Metrics are logged via (from `OpenSslExecutor.cs:225`): -```csharp -this.Logger.LogMetrics( - "OpenSSL", - this.MetricScenario ?? this.Scenario, - workloadProcess.StartTime, - workloadProcess.ExitTime, - metrics, - null, - commandArguments, - this.Tags, - telemetryContext); -``` - -### Client/Server Architecture - - -For network and database workloads, VirtualClient supports multi-role execution: -- One instance runs as **server**, another as **client** (e.g., `NetworkingWorkloadExecutor.cs`) -- They coordinate via the built-in REST API (`VirtualClient.Api/` project) for state synchronization and heartbeat -- Components use `Polly` retry policies for resilience (e.g., `NetworkingWorkloadExecutor.cs:49`) -- The `EnvironmentLayout` (`VirtualClient.Contracts/EnvironmentLayout.cs`) defines the topology of instances +External binaries run through `IProcessProxy` (wraps `System.Diagnostics.Process`). +Output captured via `ConcurrentBuffer`. Tests use `InMemoryProcess`. ### Error Handling - -Custom exception hierarchy rooted at `VirtualClientException` (`Exceptions.cs:12`): -- `ApiException` (`Exceptions.cs:78`) — API communication failures -- `ComponentException` (`Exceptions.cs:137`) — general component failures -- `MonitorException` (`Exceptions.cs:196`) — monitor failures -- `WorkloadResultsException` (`Exceptions.cs:373`) — parsing failures, missing results -- `DependencyException` (`Exceptions.cs:432`) — dependency resolution failures -- `ProcessException` (`Exceptions.cs:491`) — process execution failures -- `StartupException` (`Exceptions.cs:549`) — startup/initialization failures - -All exceptions carry an `ErrorReason` enum value (`Enumerations.cs:37`). Error reasons ≥500 are fatal (e.g., `ProfileNotFound = 500`); 400–499 are potentially transient (e.g., `InvalidResults = 400`). See the comment at `Enumerations.cs:115` and `Enumerations.cs:153`. - -## Coding Standards and Conventions - -### File Header - -Every `.cs` file starts with: -```csharp -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -``` - -### Namespace and Using Style - -- **Using statements go inside the namespace block** (not at file top) -- Ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*` -- Namespace matches folder structure: `VirtualClient.Actions`, `VirtualClient.Contracts`, etc. - -```csharp -// From OpenSslExecutor.cs:4–21 -namespace VirtualClient.Actions -{ - using System; - using System.Collections.Generic; - using System.Diagnostics; - using System.IO.Abstractions; - using System.Runtime.InteropServices; - using System.Threading; - using System.Threading.Tasks; - using Microsoft.Extensions.DependencyInjection; - using Microsoft.Extensions.Logging; - using VirtualClient.Common; - using VirtualClient.Common.Extensions; - using VirtualClient.Common.Platform; - using VirtualClient.Common.Telemetry; - using VirtualClient.Contracts; - using VirtualClient.Contracts.Metadata; -``` - -### Naming Conventions - -- **Classes**: PascalCase, suffixed by role (e.g., `OpenSslExecutor` in `OpenSSL/OpenSslExecutor.cs`, `DiskSpdMetricsParser` in `DiskSpd/DiskSpdMetricsParser.cs`, `CoreMarkExecutor` in `CoreMark/CoreMarkExecutor.cs`) -- **Properties**: PascalCase (e.g., `CommandLine`, `MetricScenario`, `MonitorEnabled`) -- **Private fields**: camelCase, no prefix for instance fields; `const` fields use PascalCase - ```csharp - // From OpenSslExecutor.cs:37–38 - private IFileSystem fileSystem; - private ISystemManagement systemManagement; - // From CoreMarkExecutor.cs:28–29 - private const string CoreMarkOutputFile1 = "run1.log"; - private const string CoreMarkOutputFile2 = "run2.log"; - ``` -- **Parameters dictionary keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` (see `VirtualClientComponent.cs:92`) -- **Async methods**: Suffixed with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`) -- **Test classes**: `{ComponentName}Tests` (e.g., `FioExecutorTests` in `VirtualClient.Actions.UnitTests/FIO/FioExecutorTests.cs:24`) -- **Test methods**: Descriptive with underscores for scenario separation (e.g., `FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario` at `FioExecutorTests.cs:80`) - -### Property Pattern for Profile Parameters - -Properties that read from the `Parameters` dictionary follow this pattern: -```csharp -// From OpenSslExecutor.cs:55–61 -public string CommandArguments -{ - get - { - return this.Parameters.GetValue(nameof(OpenSslExecutor.CommandArguments)); - } -} - -// With default value (from CoreMarkExecutor.cs:49–55): -public string CompilerName -{ - get - { - return this.Parameters.GetValue(nameof(this.CompilerName), string.Empty); - } -} -``` - -### XML Documentation - -All public members have XML doc comments using ``, ``, ``, ``, and `` tags: -```csharp -// From OpenSslExecutor.cs:40–44 -/// -/// Constructor -/// -/// Provides required dependencies to the component. -/// Parameters defined in the profile or supplied on the command line. -``` - -### Platform Support Attribute - -Executors declare supported platforms via a class-level attribute: -```csharp -// From OpenSslExecutor.cs:34–35 -[SupportedPlatforms("linux-arm64,linux-x64,win-x64")] -public class OpenSslExecutor : VirtualClientComponent -``` - -### Code Quality - -- **StyleCop.Analyzers** enforces style rules (suppressed: SA1204 static element ordering — `.editorconfig:3–4`) -- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming — `.editorconfig:6–7`) -- Central package version management (`Directory.Packages.props`) prevents version drift across projects - -## Executor Implementation Checklist - -When adding a new workload executor (pattern observed in `OpenSSL/OpenSslExecutor.cs`, `CoreMark/CoreMarkExecutor.cs`, `DiskSpd/DiskSpdExecutor.cs`): - -1. Create a subfolder under `VirtualClient.Actions/` named after the workload -2. Create an executor class inheriting `VirtualClientComponent` (`VirtualClient.Contracts/VirtualClientComponent.cs`) -3. Add `[SupportedPlatforms("...")]` attribute (`VirtualClient.Common/Platform/SupportedPlatformsAttribute.cs`) -4. Define constructor with `(IServiceCollection dependencies, IDictionary parameters)` -5. Expose profile parameters as properties reading from `this.Parameters` (e.g., `OpenSslExecutor.cs:55–61`) -6. Override `InitializeAsync` for setup (locate package, set executable path) -7. Override `ExecuteAsync` for the main workload logic (execute process, capture output, parse, log metrics) -8. Optionally override `CleanupAsync` and `Validate` -9. Create a `MetricsParser` subclass to parse workload output into `IList` (e.g., `DiskSpd/DiskSpdMetricsParser.cs`) -10. Create an execution profile JSON in `VirtualClient.Main/profiles/` (e.g., `PERF-CPU-OPENSSL.json`) -11. Add unit tests inheriting from `MockFixture` in the corresponding `.UnitTests` project (e.g., `FIO/FioExecutorTests.cs`) -12. Add example output files under `Examples/` for parser tests - -## Testing Philosophy and Patterns - -### Framework - -- **NUnit 3** with `[TestFixture]`, `[Test]`, `[SetUp]`, `[OneTimeSetUp]` attributes (e.g., `FioExecutorTests.cs:22–23`) -- **Moq** for mocking interfaces (e.g., `FioExecutorTests.cs:57`) -- **AutoFixture** via `MockFixture` base class for test data generation (`MockFixture.cs:35` extends `Fixture`) -- Tests are categorized: `[Category("Unit")]` (e.g., `FioExecutorTests.cs:23`) or `[Category("Functional")]` - -### MockFixture Base Class - -Test classes inherit from `MockFixture` (in `VirtualClient.TestFramework/MockFixture.cs:35`), which provides: -- Pre-configured mock services: `ApiClient` (line 87), `DiskManager` (line 141), `FileSystem` (line 156), `File` (line 161), `Directory` (line 146), `ProcessManager` (line 235) -- `Setup(PlatformID platform, Architecture architecture = ...)` method to configure platform-specific behavior (line 466) -- `MockFixture.ReadFile(...)` to load example output from `Examples/` directory (line 278) -- `InMemoryProcess` (`InMemoryProcess.cs:20`), `InMemoryFile` (`InMemoryFile.cs:17`), `InMemoryDirectory` (`InMemoryDirectory.cs:13`) test doubles - -### Test Structure Pattern - -```csharp -// From FioExecutorTests.cs -[TestFixture] -[Category("Unit")] -public class FioExecutorTests : MockFixture -{ - private IDictionary profileParameters; - private string mockResults; +Custom hierarchy: `VirtualClientException` → `WorkloadException`, `DependencyException`, +`ProcessException`, `MonitorException`, etc. All carry `ErrorReason` enum. +Codes ≥500 fatal, 400–499 transient. - [OneTimeSetUp] - public void SetupFixture() - { - this.mockResults = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "FIO", "Results_FIO.json"); - } +## Build and Test - [SetUp] - public void SetupTest() - { - this.Setup(PlatformID.Unix); - // ... - this.ProcessManager.OnCreateProcess = (command, arguments, workingDir) => - { - return new InMemoryProcess - { - OnHasExited = () => true, - ExitCode = 0, - StartInfo = new ProcessStartInfo - { - FileName = command, - Arguments = arguments, - WorkingDirectory = workingDir - }, - StandardOutput = new ConcurrentBuffer(new StringBuilder(this.mockResults)) - }; - }; - } - - [Test] - public void FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario() - { - // Arrange, Act, Assert - } -} -``` - -### Parser Tests - -Parser tests load real example output (stored in `Examples/` or `TestResources/` under test projects), run the parser, and assert against expected metric names, values, and units. This ensures parsers remain correct as output formats evolve. - -## Build and Test Commands - -### Build - -```bash -# Linux — builds solution (Debug) then publishes self-contained for all platforms (Release) -./build.sh - -# Build for specific platform only (see build.sh for all options) -./build.sh --linux-x64 -./build.sh --win-x64 --linux-arm64 - -# Windows -build.cmd -build.cmd --win-x64 -``` - -The build first compiles the solution in Debug configuration (for extension debugging — see comment in `build.sh:124–126` and `build.cmd:69–74`), then publishes runtime-specific self-contained binaries in Release. - -### Test - -```bash -# Linux — runs Unit tests only (build-test.sh:65 filters Category=Unit) -./build-test.sh - -# Windows — runs Unit + Functional tests (build-test.cmd:23 filters Category=Unit|Category=Functional) -build-test.cmd -``` - -Tests are discovered from `*Tests.csproj` files and filtered by `Category=Unit` (Linux) or `Category=Unit|Category=Functional` (Windows). - -### Direct dotnet commands ```bash -# Build solution only -dotnet build src/VirtualClient/VirtualClient.sln -c Debug +# Build +./build.sh # or: dotnet build src/VirtualClient/VirtualClient.sln -c Debug -# Run a specific test project -dotnet test src/VirtualClient/VirtualClient.Actions.UnitTests/VirtualClient.Actions.UnitTests.csproj -c Debug --filter "Category=Unit" +# Test +./build-test.sh # or: dotnet test .csproj -c Debug --filter "Category=Unit" -# Publish for a specific runtime +# Publish dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r linux-x64 -c Release --self-contained ``` -### Version - -Build version is read from the `VERSION` file at the repo root. Override with the `VCBuildVersion` environment variable. - -## PR Review Guidelines - -When reviewing pull requests, flag items below as **Required Fix** (will break the build, crash at runtime, or violate a hard architectural constraint) or **Suggestion** (inconsistency with conventions that won't break anything but should be addressed). - -### Required Fixes (flag these — they break things) - -1. **Component must inherit `VirtualClientComponent`.** - Any class referenced by a profile `"Type"` field is resolved via `ComponentTypeCache` and instantiated by `ComponentFactory.CreateComponent` using `Activator.CreateInstance` — which casts the result to `VirtualClientComponent`. A class that does not inherit this base type will throw `InvalidCastException` → `StartupException` at runtime. - - -2. **Component constructor must be `(IServiceCollection, IDictionary)`.** - `ComponentFactory` calls `Activator.CreateInstance(componentType, dependencies, effectiveParameters)` which requires exactly this two-parameter constructor signature. A missing or differently-typed constructor throws `MissingMethodException` → `StartupException`. - - -3. **Assembly containing new components must have `[assembly: VirtualClientComponentAssembly]`.** - `ComponentTypeCache.IsComponentAssembly()` checks for this attribute; assemblies without it are skipped during type discovery. A new executor in an un-attributed assembly will cause `TypeLoadException` at profile load time. - - -4. **Profile `"Type"` value must exactly match the C# class name.** - The `ComponentTypeCache.TryGetComponentType` lookup matches on type name. A typo or wrong name throws `TypeLoadException` → `StartupException` with the message "does not exist or does not inherit from VirtualClientComponent". - - -5. **Parser must extend `MetricsParser` (which extends `TextParser>`) and implement `Parse()`.** - `Parse()` is abstract in `TextParser` — failing to override it is a compile error. Using a return type other than `IList` will break the `LogMetrics` call which iterates over the result. - - -6. **NuGet package versions must be in `Directory.Packages.props`, not in individual `.csproj` files.** - The repo uses central package management. Adding a `Version=` attribute in a `.csproj` `` will cause a build error (`NU1008`) because it conflicts with centrally-managed versions. - - -7. **`using` statements must be inside the `namespace` block.** - StyleCop.Analyzers (enabled repo-wide) enforces `SA1200` — `using` directives outside a namespace cause build warnings treated as errors in CI. - - -8. **Exception types must use the project's exception hierarchy.** - Throwing raw `Exception` or `InvalidOperationException` instead of the established types (`WorkloadException`, `DependencyException`, `ProcessException`, `MonitorException`, etc.) breaks the error-handling pipeline that routes on `ErrorReason`. The `Validate()` pattern in existing executors consistently throws `WorkloadException` with `ErrorReason.InvalidProfileDefinition`. - - -9. **Test classes must have `[TestFixture]` and `[Category("Unit")]` (or `"Functional"`).** - The build scripts filter tests by category (`--filter "Category=Unit"`). Tests without a `[Category]` attribute are silently skipped by the CI pipeline and will never run. - - -10. **Copyright header must be present on every `.cs` file.** - StyleCop rule `SA1633` requires the standard two-line header. Missing it will fail the StyleCop check. - - -### Suggestions (flag these — won't break but inconsistent) - -1. **Prefer `this.Parameters.GetValue(nameof(...))` for profile parameters.** - All existing executors expose parameters as properties backed by `this.Parameters.GetValue()` using `nameof()` for the key. Direct dictionary access (`this.Parameters["Key"]`) works but is inconsistent with the established pattern and loses the case-insensitive, typed lookup. - - -2. **Add `[SupportedPlatforms("...")]` attribute to executor classes.** - While not strictly required (the base class handles missing attributes gracefully), every existing executor uses this attribute. Omitting it means the workload will attempt to run on all platforms, which is rarely correct. - - -3. **Test classes should inherit `MockFixture`, not create mocks from scratch.** - `MockFixture` pre-configures all the standard service mocks (`FileSystem`, `DiskManager`, `ProcessManager`, etc.) and provides helpers like `ReadFile()` and `Setup(PlatformID)`. Manually recreating these mocks is verbose and diverges from the pattern used by all existing tests. - - -4. **Using-statement ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*`.** - StyleCop `SA1208`/`SA1210` enforce alphabetical ordering within groups. While reordering won't break the build (it's a warning, not an error), it's inconsistent with every file in the codebase. - - -5. **Private instance fields should use camelCase with no prefix.** - The codebase uses `private IFileSystem fileSystem;` not `_fileSystem` or `m_fileSystem`. Using a different convention won't break anything but creates visual inconsistency. - - -6. **XML doc comments on all public members.** - Existing code consistently documents every public method, property, and constructor with ``, ``, and `` tags. Missing docs won't fail the build (XML doc warnings are not currently treated as errors) but diverges from project norms. - - -7. **Parser tests should load real example output from `Examples/` directories.** - Parser tests in the codebase use `MockFixture.ReadFile(MockFixture.ExamplesDirectory, ...)` to load actual benchmark output. Inline test strings work but don't verify against real-world output formats. - - -8. **`Validate()` should check required parameters and throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.** - This is the established validation pattern. While not enforced by the compiler, skipping validation means invalid profiles fail later with obscure errors instead of clear messages at startup. - - -9. **Async methods should be suffixed with `Async`.** - The `AZCA1002` analyzer rule is suppressed (`.editorconfig:6–7`), so this won't break the build. However, every existing method follows the `Async` suffix convention (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`). - +Version is read from the `VERSION` file. Override with `VCBuildVersion` env var. diff --git a/.github/instructions/ClientServer-Workloads.instructions.md b/.github/instructions/ClientServer-Workloads.instructions.md new file mode 100644 index 0000000000..a19b87eed3 --- /dev/null +++ b/.github/instructions/ClientServer-Workloads.instructions.md @@ -0,0 +1,90 @@ +--- +applyTo: "VirtualClient.Actions/**/*.cs" +description: "Pattern for developing multi-VM client/server/reverseProxy workloads" +--- + +# Client/Server Workload Development + +## Overview + +For network and database workloads, VirtualClient supports multi-role execution where separate +instances run as client and server, coordinating via the built-in REST API. + +## Key Components + +- `EnvironmentLayout` — defines the topology of instances (`ClientInstance` objects with Name, Role, IPAddress) +- `IApiClientManager` — creates API clients for inter-VM communication +- `ClientRole.Client` / `ClientRole.Server` — role constants +- `ServerCancellationSource` — `CancellationTokenSource` to signal server shutdown + +## Role Determination + +Roles come from the `EnvironmentLayout` loaded into DI. The component checks its role: + +```csharp +// Defined in profile Parameters: "Role": "Client" or "Role": "Server" +// Accessed via this.IsInRole(ClientRole.Client) or this.IsInRole(ClientRole.Server) +``` + +## Executor Pattern + +```csharp +[SupportedPlatforms("linux-x64,linux-arm64")] +public class MyWorkloadExecutor : VirtualClientComponent +{ + protected IApiClientManager ApiClientManager { get; } + protected IApiClient ServerApiClient { get; set; } + protected CancellationTokenSource ServerCancellationSource { get; set; } + + public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary parameters) + : base(dependencies, parameters) + { + this.ApiClientManager = dependencies.GetService(); + } + + // Define supported roles + public List SupportedRoles = new List { ClientRole.Client, ClientRole.Server }; + + protected override async Task ExecuteAsync(EventContext context, CancellationToken ct) + { + if (this.IsInRole(ClientRole.Server)) + { + await this.ExecuteServerAsync(context, ct); + } + else + { + await this.ExecuteClientAsync(context, ct); + } + } +} +``` + +## State Synchronization + +Client and server coordinate via the REST API (`VirtualClient.Api/`): + +- Server publishes state indicating readiness +- Client polls for server state before starting workload +- Use `Polly` retry policies for resilience against transient failures +- `IApiClient` provides `GetStateAsync`/`CreateStateAsync` for state exchange + +## Profile Structure + +```json +{ + "Actions": [ + { "Type": "MyServerExecutor", "Parameters": { "Role": "Server", "Port": 5000 } }, + { "Type": "MyClientExecutor", "Parameters": { "Role": "Client", "ServerPort": "$.Parameters.Port" } } + ] +} +``` + +## Checklist + +- [ ] Define `SupportedRoles` with `ClientRole.Client` and/or `ClientRole.Server` +- [ ] Resolve `IApiClientManager` from dependencies +- [ ] Use role checks (`this.IsInRole(...)`) to branch execution logic +- [ ] Implement state synchronization via REST API +- [ ] Use `Polly` retry policies for cross-VM communication +- [ ] Handle `ServerCancellationSource` for clean server shutdown +- [ ] Test both client and server code paths independently diff --git a/.github/instructions/MetricsParser.instructions.md b/.github/instructions/MetricsParser.instructions.md new file mode 100644 index 0000000000..3d29854133 --- /dev/null +++ b/.github/instructions/MetricsParser.instructions.md @@ -0,0 +1,95 @@ +--- +applyTo: "*MetricsParser.cs" +description: "Pattern for developing metric parsers with proper units and consistency" +--- + +# MetricsParser Development Guide + +## Class Structure + +Parsers inherit from `MetricsParser` → `TextParser>`: + +```csharp +public class MyWorkloadMetricsParser : MetricsParser +{ + private static readonly Regex ValuePattern = new Regex( + @"(\d+\.?\d*)\s+(ops/sec)", RegexOptions.Compiled); + + public MyWorkloadMetricsParser(string rawText) + : base(rawText) + { + } + + public override IList Parse() + { + try + { + this.Preprocess(); + this.Sections = TextParsingExtensions.Sectionize(this.PreprocessedText, "SectionHeader"); + List metrics = new List(); + // Parse sections and extract metrics + return metrics; + } + catch (Exception exc) + { + throw new WorkloadResultsException( + "Failed to parse results.", exc, ErrorReason.WorkloadResultsParsingFailed); + } + } + + protected override void Preprocess() + { + this.PreprocessedText = TextParsingExtensions.RemoveRows(this.RawText, somePattern); + } +} +``` + +## Standard Units (from `MetricUnit` constants) + +Always use `MetricUnit.*` constants — never raw strings: + +- **Throughput**: `KilobytesPerSecond`, `MegabytesPerSecond`, `OperationsPerSec`, + `RequestsPerSec`, `TransactionsPerSec` +- **Latency**: `Nanoseconds`, `Milliseconds`, `Seconds` +- **Count**: `Count`, `Bytes`, `Kilobytes`, `Megabytes` + +## MetricRelativity + +Set relativity correctly on every metric: + +- `MetricRelativity.HigherIsBetter` — throughput, bandwidth, operations/sec +- `MetricRelativity.LowerIsBetter` — latency, time, error counts +- `MetricRelativity.Undefined` — informational metrics (default) + +```csharp +new Metric("throughput", value, MetricUnit.OperationsPerSec, MetricRelativity.HigherIsBetter) +new Metric("latency_p99", value, MetricUnit.Milliseconds, MetricRelativity.LowerIsBetter) +``` + +## Regex Patterns + +Define as `private static readonly Regex` with `RegexOptions.Compiled`: + +```csharp +private static readonly Regex ThroughputRegex = new Regex( + @"Throughput:\s+(\d+\.?\d*)\s+ops/sec", RegexOptions.Compiled); +``` + +## Error Handling + +Wrap `Parse()` body in try-catch, throwing `WorkloadResultsException`: + +```csharp +catch (Exception exc) +{ + throw new WorkloadResultsException( + "Failed to parse MyWorkload results.", exc, ErrorReason.WorkloadResultsParsingFailed); +} +``` + +## Text Parsing Utilities + +- `TextParsingExtensions.Sectionize(text, pattern)` — split text into named sections +- `TextParsingExtensions.RemoveRows(text, pattern)` — strip unwanted lines in `Preprocess()` +- `this.PreprocessedText` — normalized text for parsing (set in `Preprocess()`) +- `this.Sections` — dictionary of parsed sections (set in `Parse()`) diff --git a/.github/instructions/csharp.instructions.md b/.github/instructions/csharp.instructions.md new file mode 100644 index 0000000000..f33c9701e7 --- /dev/null +++ b/.github/instructions/csharp.instructions.md @@ -0,0 +1,88 @@ +--- +applyTo: "**/*.cs" +description: "C# coding standards and conventions for VirtualClient" +--- + +# C# Coding Standards + +## File Header + +Every `.cs` file must start with: +```csharp +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +``` + +## Namespace and Using Style + +- `using` statements go **inside** the namespace block (not at file top) — enforced by StyleCop SA1200 +- Ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*` +- Namespace matches folder structure: `VirtualClient.Actions`, `VirtualClient.Contracts`, etc. + +```csharp +namespace VirtualClient.Actions +{ + using System; + using System.Collections.Generic; + using System.Threading; + using System.Threading.Tasks; + using Microsoft.Extensions.DependencyInjection; + using Microsoft.Extensions.Logging; + using VirtualClient.Common; + using VirtualClient.Contracts; +} +``` + +## Naming Conventions + +- **Classes**: PascalCase, suffixed by role (`OpenSslExecutor`, `DiskSpdMetricsParser`) +- **Properties**: PascalCase (`CommandLine`, `MetricScenario`) +- **Private fields**: camelCase, no prefix (`private IFileSystem fileSystem;` — not `_fileSystem`) +- **Constants**: PascalCase (`private const string CoreMarkOutputFile1 = "run1.log";`) +- **Parameters keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` +- **Async methods**: Suffix with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`) + +## Profile Parameter Properties + +Properties reading from the `Parameters` dictionary use this pattern: + +```csharp +public string CommandArguments +{ + get { return this.Parameters.GetValue(nameof(this.CommandArguments)); } +} + +// With default value: +public string CompilerName +{ + get { return this.Parameters.GetValue(nameof(this.CompilerName), string.Empty); } +} +``` + +## XML Documentation + +All public members require XML doc comments with ``, ``, `` tags: + +```csharp +/// +/// Constructor +/// +/// Provides required dependencies to the component. +/// Parameters defined in the profile or supplied on the command line. +``` + +## Code Quality Rules + +- **StyleCop.Analyzers** enforces style (suppressed: SA1204 static element ordering) +- **AsyncFixer** validates async patterns (suppressed: AZCA1002 async method naming) +- NuGet versions must be in `Directory.Packages.props`, never in individual `.csproj` files + +## Exception Handling + +Use the project's exception hierarchy — never throw raw `Exception` or `InvalidOperationException`: + +- `WorkloadException` — workload failures, validation errors (with `ErrorReason.InvalidProfileDefinition`) +- `DependencyException` — dependency resolution failures +- `ProcessException` — process execution failures +- `MonitorException` — monitor failures +- `WorkloadResultsException` — parsing failures diff --git a/.github/instructions/documentation.instructions.md b/.github/instructions/documentation.instructions.md new file mode 100644 index 0000000000..b490c83412 --- /dev/null +++ b/.github/instructions/documentation.instructions.md @@ -0,0 +1,37 @@ +--- +applyTo: "**/*.md" +description: "Documentation formatting and style guidelines" +--- + +# Documentation Guidelines + +## Markdown Formatting + +- Use ATX-style headers (`#`, `##`, `###`) with a blank line before and after +- Use fenced code blocks with language identifiers (` ```csharp `, ` ```bash `, ` ```json `) +- Use relative links for cross-references within the repository +- Keep lines at a reasonable length for readability + +## Code Examples in Documentation + +- Code examples must be syntactically correct and representative of actual codebase patterns +- Include the source file reference when citing existing code patterns +- Use comments to explain non-obvious behavior + +## Profile Documentation + +- When documenting execution profiles, show the complete JSON structure +- Include descriptions of all parameters and their expected types/values +- Note supported platforms and minimum execution times + +## API Documentation + +- Document all public APIs with clear input/output descriptions +- Include error codes and their meanings from the `ErrorReason` enum +- Provide example request/response payloads where applicable + +## README and Getting Started + +- Keep setup instructions current with the actual build process +- Test all documented commands before committing +- Include prerequisites (e.g., .NET SDK version from `global.json`: currently 9.0.301) diff --git a/.github/instructions/pr-review.instructions.md b/.github/instructions/pr-review.instructions.md new file mode 100644 index 0000000000..ef4cb738a7 --- /dev/null +++ b/.github/instructions/pr-review.instructions.md @@ -0,0 +1,59 @@ +--- +applyTo: "**/*.cs" +description: "PR review rules: required fixes vs suggestions for C# code changes" +--- + +# PR Review Guidelines + +## Required Fixes (flag these — they break things) + +1. **Component must inherit `VirtualClientComponent`.** Profile `"Type"` resolution casts via + `Activator.CreateInstance` to `VirtualClientComponent`. Wrong base class → `InvalidCastException`. + +2. **Constructor must be `(IServiceCollection, IDictionary)`.** `ComponentFactory` + uses reflection with this exact signature. Wrong constructor → `MissingMethodException`. + +3. **Assembly must have `[assembly: VirtualClientComponentAssembly]`.** Without this attribute, + `ComponentTypeCache` skips the assembly during type discovery → `TypeLoadException`. + +4. **Profile `"Type"` must exactly match the C# class name.** `ComponentTypeCache.TryGetComponentType` + matches on type name. Typo → `TypeLoadException` at profile load. + +5. **Parser must extend `MetricsParser` and implement `Parse()`.** `Parse()` is abstract — missing + override is a compile error. Wrong return type breaks `LogMetrics`. + +6. **NuGet versions in `Directory.Packages.props` only.** Adding `Version=` in a `.csproj` causes + build error `NU1008` due to central package management. + +7. **`using` statements inside `namespace` block.** StyleCop SA1200 enforced repo-wide — top-level + usings fail CI. + +8. **Use project exception hierarchy.** Raw `Exception`/`InvalidOperationException` breaks error + routing on `ErrorReason`. Use `WorkloadException`, `DependencyException`, `ProcessException`, etc. + +9. **Tests must have `[TestFixture]` and `[Category("Unit")]`.** Build scripts filter by category. + Missing category → tests silently never run in CI. + +10. **Copyright header on every `.cs` file.** StyleCop SA1633 requires the standard two-line header. + +## Suggestions (flag these — won't break but inconsistent) + +1. **Use `this.Parameters.GetValue(nameof(...))` for profile parameters** — not direct + dictionary access. + +2. **Add `[SupportedPlatforms("...")]` to executor classes** — omitting means workload attempts + to run on all platforms. + +3. **Test classes should inherit `MockFixture`** — not create mocks from scratch. + +4. **Using ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*`.** + +5. **Private fields: camelCase, no prefix** (`fileSystem` not `_fileSystem`). + +6. **XML doc comments on all public members.** + +7. **Parser tests should load real output from `Examples/`** — not inline strings. + +8. **`Validate()` should throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.** + +9. **Async methods suffixed with `Async`.** diff --git a/.github/instructions/profile-review.instructions.md b/.github/instructions/profile-review.instructions.md new file mode 100644 index 0000000000..1763a2411d --- /dev/null +++ b/.github/instructions/profile-review.instructions.md @@ -0,0 +1,64 @@ +--- +applyTo: "**/*.json" +description: "Execution profile review rules for VirtualClient JSON profiles" +--- + +# Profile Review Guidelines + +## Required Structure + +Every profile must contain: +- `"Description"` — human-readable description of the workload +- `"Metadata"` — with `SupportedPlatforms`, `SupportedOperatingSystems`, `RecommendedMinimumExecutionTime` +- `"Parameters"` — global parameters referenced by actions/dependencies +- `"Actions"` — array of workload executors to run +- `"Dependencies"` — array of prerequisite installers + +## Action Definition + +Each action must have: +- `"Type"` — exact C# class name (e.g., `"OpenSslExecutor"`, `"FioExecutor"`) +- `"Parameters"` with at minimum: + - `"Scenario"` — unique identifier for this action step + - `"PackageName"` — name of the dependency package (if applicable) + +## Parameter Referencing + +- Global parameters referenced via JSONPath: `"Duration": "$.Parameters.Duration"` +- Expression placeholders in commands: `"speed -seconds {Duration.TotalSeconds} md5"` +- Calculated expressions: `"{calculate({LogicalCoreCount}/2)}"` +- Conditional expressions: `"{calculate(\"{Platform}\".StartsWith(\"linux\") ? \"libaio\" : \"windowsaio\")}"` + +## Metadata Fields + +```json +"Metadata": { + "RecommendedMinimumExecutionTime": "01:00:00", + "SupportedPlatforms": "linux-x64,linux-arm64,win-x64", + "SupportedOperatingSystems": "AzureLinux,CentOS,Debian,RedHat,Suse,Ubuntu,Windows" +} +``` + +## Client/Server Profiles + +For multi-VM workloads, actions specify roles: + +```json +{ "Type": "ServerExecutor", "Parameters": { "Scenario": "Start", "Role": "Server", "Port": 6379 } }, +{ "Type": "ClientExecutor", "Parameters": { "Scenario": "Bench", "Role": "Client", "ServerPort": "$.Parameters.ServerPort" } } +``` + +## Dependencies + +- Must run before actions; define package installations, compiler setup, disk initialization +- Common types: `DependencyPackageInstallation`, `LinuxPackageInstallation`, `CompilerInstallation` +- Use `"Extract": true` for ZIP packages, `"BlobContainer": "packages"` for blob storage + +## Review Checklist + +- [ ] `"Type"` values match actual C# class names +- [ ] All `$.Parameters.*` references resolve to defined global parameters +- [ ] `{Placeholder}` expressions reference valid properties +- [ ] `SupportedPlatforms` lists only platforms the executor actually supports +- [ ] `Scenario` values are unique within the profile +- [ ] Dependencies are ordered correctly (base packages before workload packages) diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md new file mode 100644 index 0000000000..6a58bd41c1 --- /dev/null +++ b/.github/instructions/testing.instructions.md @@ -0,0 +1,77 @@ +--- +applyTo: "**/*Tests.cs" +description: "Unit test patterns, naming conventions, mock setup, and assertion rules" +--- + +# Unit Testing Patterns + +## Framework and Attributes + +- **NUnit 3** with `[TestFixture]`, `[Test]`, `[SetUp]`, `[OneTimeSetUp]` +- **Moq** for mocking interfaces +- **AutoFixture** via `MockFixture` base class +- **Required**: `[Category("Unit")]` or `[Category("Functional")]` — tests without a category + are silently skipped by CI (`build-test.sh` filters on `Category=Unit`) + +## Test Class Structure + +- Test classes **must inherit `MockFixture`** (from `VirtualClient.TestFramework`) +- Class name: `{ComponentName}Tests` (e.g., `FioExecutorTests`) +- Method names: Descriptive with underscores (e.g., `FioExecutorSelectsTheExpectedDisks_RemoteDiskScenario`) + +```csharp +[TestFixture] +[Category("Unit")] +public class FioExecutorTests : MockFixture +{ + private IDictionary profileParameters; + private string mockResults; + + [OneTimeSetUp] + public void SetupFixture() + { + this.mockResults = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "FIO", "Results_FIO.json"); + } + + [SetUp] + public void SetupTest() + { + this.Setup(PlatformID.Unix); + this.ProcessManager.OnCreateProcess = (command, arguments, workingDir) => + { + return new InMemoryProcess + { + OnHasExited = () => true, + ExitCode = 0, + StartInfo = new ProcessStartInfo { FileName = command, Arguments = arguments }, + StandardOutput = new ConcurrentBuffer(new StringBuilder(this.mockResults)) + }; + }; + } + + [Test] + public void FioExecutorSelectsTheExpectedDisksForTest_RemoteDiskScenario() + { + // Arrange, Act, Assert + } +} +``` + +## MockFixture Provides + +- Pre-configured mocks: `ApiClient`, `DiskManager`, `FileSystem`, `File`, `Directory`, `ProcessManager` +- `Setup(PlatformID platform, Architecture arch)` — configure platform-specific behavior +- `MockFixture.ReadFile(...)` — load example output from `Examples/` directories +- Test doubles: `InMemoryProcess`, `InMemoryFile`, `InMemoryDirectory` + +## Parser Tests + +- Load real example output from `Examples/` directories (not inline strings) +- Run the parser against actual benchmark output +- Assert specific metric names, values, and units + +## Process Mocking + +Use `InMemoryProcess` via `ProcessManager.OnCreateProcess`: +- Set `ExitCode`, `OnHasExited`, `StandardOutput`, `StandardError` +- `StandardOutput` uses `ConcurrentBuffer(new StringBuilder(content))` diff --git a/.github/instructions/workload-development.instructions.md b/.github/instructions/workload-development.instructions.md new file mode 100644 index 0000000000..4f243c6cd6 --- /dev/null +++ b/.github/instructions/workload-development.instructions.md @@ -0,0 +1,80 @@ +--- +applyTo: "VirtualClient.Actions/**/*.cs" +description: "Pattern for developing new workload executors" +--- + +# Workload Development Guide + +## New Executor Checklist + +1. Create subfolder under `VirtualClient.Actions/` named after the workload +2. Create executor class inheriting `VirtualClientComponent` +3. Add `[SupportedPlatforms("linux-x64,linux-arm64,win-x64")]` attribute +4. Define constructor: `(IServiceCollection dependencies, IDictionary parameters)` +5. Expose profile parameters as properties using `this.Parameters.GetValue(nameof(...))` +6. Override `InitializeAsync` — locate package, set executable path +7. Override `ExecuteAsync` — run process, capture output, parse metrics, log telemetry +8. Override `Validate` — check required parameters, throw `WorkloadException` +9. Create a `MetricsParser` subclass (see MetricsParser.instructions.md) +10. Create profile JSON in `VirtualClient.Main/profiles/` +11. Add unit tests inheriting `MockFixture` in `.UnitTests` project +12. Add example output files in `Examples/` for parser tests + +## Class Structure + +```csharp +[SupportedPlatforms("linux-arm64,linux-x64,win-x64")] +public class MyWorkloadExecutor : VirtualClientComponent +{ + private IFileSystem fileSystem; + private ISystemManagement systemManagement; + + public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary parameters) + : base(dependencies, parameters) + { + this.fileSystem = dependencies.GetService(); + this.systemManagement = dependencies.GetService(); + } + + public string CommandArguments + { + get { return this.Parameters.GetValue(nameof(this.CommandArguments)); } + } + + protected override async Task InitializeAsync(EventContext telemetryContext, CancellationToken ct) + { + DependencyPath package = await this.GetPackageAsync(this.PackageName, ct); + this.ExecutablePath = this.Combine(package.Path, "bin", "my-tool"); + } + + protected override async Task ExecuteAsync(EventContext telemetryContext, CancellationToken ct) + { + using (IProcessProxy process = await this.ExecuteCommandAsync( + this.ExecutablePath, this.CommandArguments, this.WorkingDirectory, telemetryContext, ct)) + { + await this.LogProcessDetailsAsync(process, telemetryContext, "MyWorkload"); + process.ThrowIfWorkloadFailed(); + + MyWorkloadParser parser = new MyWorkloadParser(process.StandardOutput.ToString()); + IList metrics = parser.Parse(); + + this.Logger.LogMetrics( + "MyWorkload", this.MetricScenario ?? this.Scenario, + process.StartTime, process.ExitTime, + metrics, null, this.CommandArguments, this.Tags, telemetryContext); + } + } +} +``` + +## Process Execution + +- Use `this.ExecuteCommandAsync()` or `ProcessManager.CreateProcess()` +- Always call `process.ThrowIfWorkloadFailed()` to check exit code +- Capture output via `process.StandardOutput.ToString()` + +## Telemetry + +- Log metrics via `this.Logger.LogMetrics(toolName, scenario, start, end, metrics, ...)` +- Use `EventContext` for structured telemetry throughout the lifecycle +- Log process details via `this.LogProcessDetailsAsync(process, context, toolName)` From 505eedf8c73f5e9fac0ca2cf8ab35c3de344578f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 22:59:31 +0000 Subject: [PATCH 5/9] Fix glob patterns for MetricsParser and testing instruction files Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/284221cb-f2b7-47a2-9538-840584055261 Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/instructions/MetricsParser.instructions.md | 2 +- .github/instructions/testing.instructions.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/instructions/MetricsParser.instructions.md b/.github/instructions/MetricsParser.instructions.md index 3d29854133..671e86466d 100644 --- a/.github/instructions/MetricsParser.instructions.md +++ b/.github/instructions/MetricsParser.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: "*MetricsParser.cs" +applyTo: "**/*MetricsParser.cs" description: "Pattern for developing metric parsers with proper units and consistency" --- diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index 6a58bd41c1..df50c7b80a 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: "**/*Tests.cs" +applyTo: "**/*Tests/**/*.cs" description: "Unit test patterns, naming conventions, mock setup, and assertion rules" --- From cca2b207a5cb1e24a517b842050ba231b9eb4fd1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 23:31:34 +0000 Subject: [PATCH 6/9] Fix factual errors, remove duplication, rewrite weak instruction files - copilot-instructions.md: fix lifecycle order (add IsSupported), remove error handling and DI service list duplication - csharp.instructions.md: add this. prefix convention - pr-review.instructions.md: remove suggestions that duplicate csharp conventions - profile-review.instructions.md: narrow applyTo glob to profiles/, remove client/server section duplicated in ClientServer-Workloads - MetricsParser.instructions.md: replace incomplete unit list with full list from source, fix placeholder in Preprocess example - ClientServer-Workloads.instructions.md: rewrite to fix SupportedRoles field shadowing base property, add proper constructor deps matching ExampleClientServerExecutor, add concrete SetServerOnline/PollForHeartbeat sync patterns - workload-development.instructions.md: add Validate override to template - testing.instructions.md: add assertion examples with MetricAssert.Exists - documentation.instructions.md: rewrite with VirtualClient-specific rules (Docusaurus site structure, workload doc template, metric tables) Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/5e869f39-3937-4d40-a653-352d7f90de4f Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 13 +-- .../ClientServer-Workloads.instructions.md | 96 ++++++++++--------- .../MetricsParser.instructions.md | 16 ++-- .github/instructions/csharp.instructions.md | 1 + .../documentation.instructions.md | 51 +++++----- .../instructions/pr-review.instructions.md | 21 +--- .../profile-review.instructions.md | 11 +-- .github/instructions/testing.instructions.md | 16 +++- .../workload-development.instructions.md | 6 ++ 9 files changed, 118 insertions(+), 113 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index edc01915ba..8bac47543d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -39,24 +39,13 @@ src/VirtualClient/ All workloads, dependencies, monitors inherit `VirtualClientComponent`. Constructor: `(IServiceCollection, IDictionary)`. -Lifecycle: `InitializeAsync` → `Validate` → `ExecuteAsync` → `CleanupAsync`. - -### Dependency Injection - -Services registered in `CommandBase.InitializeDependencies()`, resolved via `dependencies.GetService()`. -Key services: `IFileSystem`, `ISystemManagement`, `ProcessManager`, `IPackageManager`, `ILogger`. +Lifecycle: `IsSupported` → `InitializeAsync` → `Validate` → `ExecuteAsync` → `CleanupAsync`. ### Process Execution External binaries run through `IProcessProxy` (wraps `System.Diagnostics.Process`). Output captured via `ConcurrentBuffer`. Tests use `InMemoryProcess`. -### Error Handling - -Custom hierarchy: `VirtualClientException` → `WorkloadException`, `DependencyException`, -`ProcessException`, `MonitorException`, etc. All carry `ErrorReason` enum. -Codes ≥500 fatal, 400–499 transient. - ## Build and Test ```bash diff --git a/.github/instructions/ClientServer-Workloads.instructions.md b/.github/instructions/ClientServer-Workloads.instructions.md index a19b87eed3..ccf24b9d8e 100644 --- a/.github/instructions/ClientServer-Workloads.instructions.md +++ b/.github/instructions/ClientServer-Workloads.instructions.md @@ -5,68 +5,74 @@ description: "Pattern for developing multi-VM client/server/reverseProxy workloa # Client/Server Workload Development -## Overview - For network and database workloads, VirtualClient supports multi-role execution where separate instances run as client and server, coordinating via the built-in REST API. ## Key Components -- `EnvironmentLayout` — defines the topology of instances (`ClientInstance` objects with Name, Role, IPAddress) +- `EnvironmentLayout` — topology of instances (`ClientInstance` with Name, Role, IPAddress) - `IApiClientManager` — creates API clients for inter-VM communication -- `ClientRole.Client` / `ClientRole.Server` — role constants -- `ServerCancellationSource` — `CancellationTokenSource` to signal server shutdown - -## Role Determination - -Roles come from the `EnvironmentLayout` loaded into DI. The component checks its role: +- `ClientRole.Client` / `ClientRole.Server` / `ClientRole.ReverseProxy` — role constants +- `this.SetServerOnline(bool)` — extension method to signal server readiness +- `serverApiClient.PollForHeartbeatAsync(timeout, ct)` / `PollForServerOnlineAsync(timeout, ct)` -```csharp -// Defined in profile Parameters: "Role": "Client" or "Role": "Server" -// Accessed via this.IsInRole(ClientRole.Client) or this.IsInRole(ClientRole.Server) -``` +## Base Executor Pattern -## Executor Pattern +See `VirtualClient.Actions/Examples/ClientServer/ExampleClientServerExecutor.cs` for the canonical +implementation. The base class resolves dependencies and defines supported roles in the constructor: ```csharp [SupportedPlatforms("linux-x64,linux-arm64")] public class MyWorkloadExecutor : VirtualClientComponent { - protected IApiClientManager ApiClientManager { get; } - protected IApiClient ServerApiClient { get; set; } - protected CancellationTokenSource ServerCancellationSource { get; set; } - - public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary parameters) + public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary parameters = null) : base(dependencies, parameters) { + this.SystemManagement = dependencies.GetService(); this.ApiClientManager = dependencies.GetService(); - } - - // Define supported roles - public List SupportedRoles = new List { ClientRole.Client, ClientRole.Server }; + this.FileSystem = this.SystemManagement.FileSystem; + this.PackageManager = this.SystemManagement.PackageManager; + this.ProcessManager = this.SystemManagement.ProcessManager; + this.StateManager = this.SystemManagement.StateManager; - protected override async Task ExecuteAsync(EventContext context, CancellationToken ct) - { - if (this.IsInRole(ClientRole.Server)) - { - await this.ExecuteServerAsync(context, ct); - } - else - { - await this.ExecuteClientAsync(context, ct); - } + // Set the base class property — do NOT declare a new field + this.SupportedRoles = new List { ClientRole.Client, ClientRole.Server }; } } ``` -## State Synchronization +## Client-Side Sync Flow + +Clients poll the server before starting the workload (see `ExampleClientExecutor.cs`): + +```csharp +IApiClient serverApiClient = this.ApiClientManager.GetOrCreateApiClient(server.Name, server); +await serverApiClient.PollForHeartbeatAsync(this.PollingTimeout, cancellationToken); +await serverApiClient.PollForServerOnlineAsync(TimeSpan.FromSeconds(30), cancellationToken); +// Server confirmed online — execute workload +``` + +## Server-Side Signal Flow -Client and server coordinate via the REST API (`VirtualClient.Api/`): +Servers signal readiness after starting (see `ExampleServerExecutor.cs`): + +```csharp +this.SetServerOnline(true); // Signal to clients +await webHostProcess.WaitForExitAsync(cancellationToken); +// In finally block: +this.SetServerOnline(false); // Always signal offline before exiting +``` -- Server publishes state indicating readiness -- Client polls for server state before starting workload -- Use `Polly` retry policies for resilience against transient failures -- `IApiClient` provides `GetStateAsync`/`CreateStateAsync` for state exchange +## Validation + +Override `Validate()` to check layout and roles: +```csharp +protected override void Validate() +{ + base.Validate(); + this.ThrowIfLayoutNotDefined(); +} +``` ## Profile Structure @@ -81,10 +87,10 @@ Client and server coordinate via the REST API (`VirtualClient.Api/`): ## Checklist -- [ ] Define `SupportedRoles` with `ClientRole.Client` and/or `ClientRole.Server` -- [ ] Resolve `IApiClientManager` from dependencies -- [ ] Use role checks (`this.IsInRole(...)`) to branch execution logic -- [ ] Implement state synchronization via REST API -- [ ] Use `Polly` retry policies for cross-VM communication -- [ ] Handle `ServerCancellationSource` for clean server shutdown +- [ ] Set `this.SupportedRoles` in constructor (use base class property, not a new field) +- [ ] Resolve `IApiClientManager` and `ISystemManagement` from dependencies +- [ ] Use `this.IsInRole(ClientRole.Client/Server)` to branch execution +- [ ] Server calls `this.SetServerOnline(true/false)` for handshake +- [ ] Client calls `PollForHeartbeatAsync` then `PollForServerOnlineAsync` +- [ ] Use `Polly` retry policies for cross-VM communication resilience - [ ] Test both client and server code paths independently diff --git a/.github/instructions/MetricsParser.instructions.md b/.github/instructions/MetricsParser.instructions.md index 671e86466d..c7439ec979 100644 --- a/.github/instructions/MetricsParser.instructions.md +++ b/.github/instructions/MetricsParser.instructions.md @@ -39,19 +39,21 @@ public class MyWorkloadMetricsParser : MetricsParser protected override void Preprocess() { - this.PreprocessedText = TextParsingExtensions.RemoveRows(this.RawText, somePattern); + // Remove unwanted lines before parsing + this.PreprocessedText = TextParsingExtensions.RemoveRows(this.RawText, @"^\s*$"); } } ``` ## Standard Units (from `MetricUnit` constants) -Always use `MetricUnit.*` constants — never raw strings: - -- **Throughput**: `KilobytesPerSecond`, `MegabytesPerSecond`, `OperationsPerSec`, - `RequestsPerSec`, `TransactionsPerSec` -- **Latency**: `Nanoseconds`, `Milliseconds`, `Seconds` -- **Count**: `Count`, `Bytes`, `Kilobytes`, `Megabytes` +Always use `MetricUnit.*` constants from `VirtualClient.Contracts/MetricUnit.cs` — never raw strings. +Full list includes: `Bytes`, `Kilobytes`, `Megabytes`, `Gigabytes`, `Terabytes`, `Petabytes`, +`BytesPerSecond`, `KilobytesPerSecond`, `MegabytesPerSecond`, `GigabytesPerSecond`, +`KibibytesPerSecond`, `MebibytesPerSecond`, `GibibytesPerSecond`, +`OperationsPerSec`, `RequestsPerSec`, `TransactionsPerSec`, +`Nanoseconds`, `Microseconds`, `Milliseconds`, `Seconds`, `Minutes`, +`Count`, `Operations`, `Watts`, `Amps`, `Celcius`, `Megahertz`, `BytesPerConnection`. ## MetricRelativity diff --git a/.github/instructions/csharp.instructions.md b/.github/instructions/csharp.instructions.md index f33c9701e7..fc4dfa5147 100644 --- a/.github/instructions/csharp.instructions.md +++ b/.github/instructions/csharp.instructions.md @@ -38,6 +38,7 @@ namespace VirtualClient.Actions - **Classes**: PascalCase, suffixed by role (`OpenSslExecutor`, `DiskSpdMetricsParser`) - **Properties**: PascalCase (`CommandLine`, `MetricScenario`) - **Private fields**: camelCase, no prefix (`private IFileSystem fileSystem;` — not `_fileSystem`) +- **Member access**: Always use `this.` prefix (`this.fileSystem`, `this.Parameters`, `this.Logger`) - **Constants**: PascalCase (`private const string CoreMarkOutputFile1 = "run1.log";`) - **Parameters keys**: PascalCase, accessed case-insensitively via `StringComparer.OrdinalIgnoreCase` - **Async methods**: Suffix with `Async` (`ExecuteAsync`, `InitializeAsync`, `CleanupAsync`) diff --git a/.github/instructions/documentation.instructions.md b/.github/instructions/documentation.instructions.md index b490c83412..33928b8044 100644 --- a/.github/instructions/documentation.instructions.md +++ b/.github/instructions/documentation.instructions.md @@ -1,37 +1,44 @@ --- applyTo: "**/*.md" -description: "Documentation formatting and style guidelines" +description: "Documentation formatting and style guidelines for VirtualClient" --- # Documentation Guidelines -## Markdown Formatting +## Website Documentation (`website/docs/`) -- Use ATX-style headers (`#`, `##`, `###`) with a blank line before and after -- Use fenced code blocks with language identifiers (` ```csharp `, ` ```bash `, ` ```json `) -- Use relative links for cross-references within the repository -- Keep lines at a reasonable length for readability +The documentation site uses **Docusaurus** (`website/docusaurus.config.js`). Workload docs live +under `website/docs/workloads/{workload-name}/`. -## Code Examples in Documentation +### Workload Documentation Structure -- Code examples must be syntactically correct and representative of actual codebase patterns -- Include the source file reference when citing existing code patterns -- Use comments to explain non-obvious behavior +Every new workload should have a matching doc page. Follow the existing pattern (e.g., `openssl/openssl.md`): -## Profile Documentation +1. **Title** — the workload/tool name as an H1 +2. **Overview** — what the tool does, with links to upstream project/docs +3. **What is Being Measured?** — metrics produced and what they represent +4. **Workload Metrics** — table of metric names, units, and relativity +5. **Supported Platforms** — which `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` are supported +6. **Profile Parameters** — table of all profile parameters with types, defaults, descriptions + +### Metric Tables + +When documenting metrics, use consistent columns: -- When documenting execution profiles, show the complete JSON structure -- Include descriptions of all parameters and their expected types/values -- Note supported platforms and minimum execution times +| Metric Name | Unit | Relativity | Description | +|---|---|---|---| +| `throughput` | `operations/sec` | HigherIsBetter | Operations per second | -## API Documentation +Use the exact `MetricUnit.*` constant string values (e.g., `kilobytes/sec`, `milliseconds`). -- Document all public APIs with clear input/output descriptions -- Include error codes and their meanings from the `ErrorReason` enum -- Provide example request/response payloads where applicable +## Code Examples in Docs -## README and Getting Started +- Code examples must compile and match actual codebase patterns +- Use fenced blocks with language identifiers (` ```csharp `, ` ```json `, ` ```bash `) +- Reference the source file when citing existing patterns + +## Profile Documentation -- Keep setup instructions current with the actual build process -- Test all documented commands before committing -- Include prerequisites (e.g., .NET SDK version from `global.json`: currently 9.0.301) +- Show the complete JSON profile structure with all parameters +- Include descriptions of parameters and their expected types/values +- Note `SupportedPlatforms` and `RecommendedMinimumExecutionTime` from profile metadata diff --git a/.github/instructions/pr-review.instructions.md b/.github/instructions/pr-review.instructions.md index ef4cb738a7..68d5c07199 100644 --- a/.github/instructions/pr-review.instructions.md +++ b/.github/instructions/pr-review.instructions.md @@ -36,24 +36,13 @@ description: "PR review rules: required fixes vs suggestions for C# code changes 10. **Copyright header on every `.cs` file.** StyleCop SA1633 requires the standard two-line header. -## Suggestions (flag these — won't break but inconsistent) +## Suggestions (flag these — coding conventions defined in csharp.instructions.md) -1. **Use `this.Parameters.GetValue(nameof(...))` for profile parameters** — not direct - dictionary access. - -2. **Add `[SupportedPlatforms("...")]` to executor classes** — omitting means workload attempts +1. **Add `[SupportedPlatforms("...")]` to executor classes** — omitting means workload attempts to run on all platforms. -3. **Test classes should inherit `MockFixture`** — not create mocks from scratch. - -4. **Using ordering: `System.*` → `Microsoft.*` → `Newtonsoft.*` → `VirtualClient.*`.** - -5. **Private fields: camelCase, no prefix** (`fileSystem` not `_fileSystem`). +2. **`Validate()` should throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.** -6. **XML doc comments on all public members.** - -7. **Parser tests should load real output from `Examples/`** — not inline strings. - -8. **`Validate()` should throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.** +3. **Test classes should inherit `MockFixture`** — not create mocks from scratch. -9. **Async methods suffixed with `Async`.** +4. **Parser tests should load real output from `Examples/`** — not inline strings. diff --git a/.github/instructions/profile-review.instructions.md b/.github/instructions/profile-review.instructions.md index 1763a2411d..bc1152fca8 100644 --- a/.github/instructions/profile-review.instructions.md +++ b/.github/instructions/profile-review.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: "**/*.json" +applyTo: "**/profiles/**/*.json" description: "Execution profile review rules for VirtualClient JSON profiles" --- @@ -39,15 +39,6 @@ Each action must have: } ``` -## Client/Server Profiles - -For multi-VM workloads, actions specify roles: - -```json -{ "Type": "ServerExecutor", "Parameters": { "Scenario": "Start", "Role": "Server", "Port": 6379 } }, -{ "Type": "ClientExecutor", "Parameters": { "Scenario": "Bench", "Role": "Client", "ServerPort": "$.Parameters.ServerPort" } } -``` - ## Dependencies - Must run before actions; define package installations, compiler setup, disk initialization diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index df50c7b80a..b855a032e0 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -68,7 +68,21 @@ public class FioExecutorTests : MockFixture - Load real example output from `Examples/` directories (not inline strings) - Run the parser against actual benchmark output -- Assert specific metric names, values, and units +- Assert specific metric names, values, and units: + +```csharp +[Test] +public void MyParserParsesMetricsCorrectly() +{ + string output = MockFixture.ReadFile(MockFixture.ExamplesDirectory, "MyWorkload", "results.txt"); + MyWorkloadMetricsParser parser = new MyWorkloadMetricsParser(output); + IList metrics = parser.Parse(); + + Assert.IsNotEmpty(metrics); + MetricAssert.Exists(metrics, "throughput", 12345.67, MetricUnit.OperationsPerSec); + MetricAssert.Exists(metrics, "latency_p99", 1.23, MetricUnit.Milliseconds); +} +``` ## Process Mocking diff --git a/.github/instructions/workload-development.instructions.md b/.github/instructions/workload-development.instructions.md index 4f243c6cd6..7a5a85d9f9 100644 --- a/.github/instructions/workload-development.instructions.md +++ b/.github/instructions/workload-development.instructions.md @@ -64,6 +64,12 @@ public class MyWorkloadExecutor : VirtualClientComponent metrics, null, this.CommandArguments, this.Tags, telemetryContext); } } + + protected override void Validate() + { + base.Validate(); + this.ThrowIfParameterNotDefined(nameof(this.CommandArguments)); + } } ``` From a6b354b493b016c0d2283d012198521c216e7c0d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 23:37:59 +0000 Subject: [PATCH 7/9] Normalize instruction file names to lowercase-with-hyphens, fix cross-references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename ClientServer-Workloads.instructions.md → client-server-workloads.instructions.md - Rename MetricsParser.instructions.md → metrics-parser.instructions.md - Update cross-reference in workload-development.instructions.md - Deduplicate exception handling: pr-review now references csharp for the detailed list Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/166d6345-47e7-4cc6-a97a-80b517ed55cb Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- ....instructions.md => client-server-workloads.instructions.md} | 0 ...icsParser.instructions.md => metrics-parser.instructions.md} | 0 .github/instructions/pr-review.instructions.md | 2 +- .github/instructions/workload-development.instructions.md | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename .github/instructions/{ClientServer-Workloads.instructions.md => client-server-workloads.instructions.md} (100%) rename .github/instructions/{MetricsParser.instructions.md => metrics-parser.instructions.md} (100%) diff --git a/.github/instructions/ClientServer-Workloads.instructions.md b/.github/instructions/client-server-workloads.instructions.md similarity index 100% rename from .github/instructions/ClientServer-Workloads.instructions.md rename to .github/instructions/client-server-workloads.instructions.md diff --git a/.github/instructions/MetricsParser.instructions.md b/.github/instructions/metrics-parser.instructions.md similarity index 100% rename from .github/instructions/MetricsParser.instructions.md rename to .github/instructions/metrics-parser.instructions.md diff --git a/.github/instructions/pr-review.instructions.md b/.github/instructions/pr-review.instructions.md index 68d5c07199..3d71ae584e 100644 --- a/.github/instructions/pr-review.instructions.md +++ b/.github/instructions/pr-review.instructions.md @@ -29,7 +29,7 @@ description: "PR review rules: required fixes vs suggestions for C# code changes usings fail CI. 8. **Use project exception hierarchy.** Raw `Exception`/`InvalidOperationException` breaks error - routing on `ErrorReason`. Use `WorkloadException`, `DependencyException`, `ProcessException`, etc. + routing on `ErrorReason`. See exception list in csharp.instructions.md. 9. **Tests must have `[TestFixture]` and `[Category("Unit")]`.** Build scripts filter by category. Missing category → tests silently never run in CI. diff --git a/.github/instructions/workload-development.instructions.md b/.github/instructions/workload-development.instructions.md index 7a5a85d9f9..af0bab91ab 100644 --- a/.github/instructions/workload-development.instructions.md +++ b/.github/instructions/workload-development.instructions.md @@ -15,7 +15,7 @@ description: "Pattern for developing new workload executors" 6. Override `InitializeAsync` — locate package, set executable path 7. Override `ExecuteAsync` — run process, capture output, parse metrics, log telemetry 8. Override `Validate` — check required parameters, throw `WorkloadException` -9. Create a `MetricsParser` subclass (see MetricsParser.instructions.md) +9. Create a `MetricsParser` subclass (see metrics-parser.instructions.md) 10. Create profile JSON in `VirtualClient.Main/profiles/` 11. Add unit tests inheriting `MockFixture` in `.UnitTests` project 12. Add example output files in `Examples/` for parser tests From cca55e1f5ff674553e0182c40a6fd0b585951726 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 23:41:19 +0000 Subject: [PATCH 8/9] Remove documentation.instructions.md as requested Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/7c69f96c-828d-4b13-85ea-9e9aef0c01bb Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .../documentation.instructions.md | 44 ------------------- 1 file changed, 44 deletions(-) delete mode 100644 .github/instructions/documentation.instructions.md diff --git a/.github/instructions/documentation.instructions.md b/.github/instructions/documentation.instructions.md deleted file mode 100644 index 33928b8044..0000000000 --- a/.github/instructions/documentation.instructions.md +++ /dev/null @@ -1,44 +0,0 @@ ---- -applyTo: "**/*.md" -description: "Documentation formatting and style guidelines for VirtualClient" ---- - -# Documentation Guidelines - -## Website Documentation (`website/docs/`) - -The documentation site uses **Docusaurus** (`website/docusaurus.config.js`). Workload docs live -under `website/docs/workloads/{workload-name}/`. - -### Workload Documentation Structure - -Every new workload should have a matching doc page. Follow the existing pattern (e.g., `openssl/openssl.md`): - -1. **Title** — the workload/tool name as an H1 -2. **Overview** — what the tool does, with links to upstream project/docs -3. **What is Being Measured?** — metrics produced and what they represent -4. **Workload Metrics** — table of metric names, units, and relativity -5. **Supported Platforms** — which `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` are supported -6. **Profile Parameters** — table of all profile parameters with types, defaults, descriptions - -### Metric Tables - -When documenting metrics, use consistent columns: - -| Metric Name | Unit | Relativity | Description | -|---|---|---|---| -| `throughput` | `operations/sec` | HigherIsBetter | Operations per second | - -Use the exact `MetricUnit.*` constant string values (e.g., `kilobytes/sec`, `milliseconds`). - -## Code Examples in Docs - -- Code examples must compile and match actual codebase patterns -- Use fenced blocks with language identifiers (` ```csharp `, ` ```json `, ` ```bash `) -- Reference the source file when citing existing patterns - -## Profile Documentation - -- Show the complete JSON profile structure with all parameters -- Include descriptions of parameters and their expected types/values -- Note `SupportedPlatforms` and `RecommendedMinimumExecutionTime` from profile metadata From 8a7f37e6359f3a7ddd29ad8803910ae90a1e5e6c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 23:57:35 +0000 Subject: [PATCH 9/9] Remove explicit version numbers from library references in copilot-instructions.md Agent-Logs-Url: https://github.com/microsoft/VirtualClient/sessions/e6210446-9382-456c-ad55-a4744cf27408 Co-authored-by: nchapagain001 <165215502+nchapagain001@users.noreply.github.com> --- .github/copilot-instructions.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8bac47543d..900db45d6e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -8,13 +8,13 @@ structured metrics. Supports Windows and Linux on x64 and ARM64. ## Tech Stack -- **Runtime**: .NET 9 (SDK 9.0.301, `global.json`) +- **Runtime**: .NET 9 (`global.json`) - **Platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64` -- **Serialization**: `Newtonsoft.Json` 13.0.3, `YamlDotNet` 15.1.1 -- **DI**: `Microsoft.Extensions.DependencyInjection` 9.0.9 -- **Logging**: `Serilog.Extensions.Logging` 9.0.2 (NOT `Serilog`) -- **Testing**: NUnit 3.13.2, Moq 4.18.2, AutoFixture 4.18.1 -- **Code quality**: StyleCop.Analyzers 1.1.118, AsyncFixer 1.6.0 +- **Serialization**: `Newtonsoft.Json`, `YamlDotNet` +- **DI**: `Microsoft.Extensions.DependencyInjection` +- **Logging**: `Serilog.Extensions.Logging` (NOT `Serilog`) +- **Testing**: NUnit, Moq, AutoFixture +- **Code quality**: StyleCop.Analyzers, AsyncFixer - **Package versions**: Centrally managed via `Directory.Packages.props` ## Repository Structure