Refactor copilot-instructions.md into focused instruction files with PR review guidelines

.github/copilot-instructions.md

@@ -0,0 +1,62 @@
+# Copilot Instructions for VirtualClient
+
+## Project Overview
+
+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.
+
+## Tech Stack
+
+- **Runtime**: .NET 9 (`global.json`)
+- **Platforms**: `linux-x64`, `linux-arm64`, `win-x64`, `win-arm64`
+- **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
+
+```
+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, monitors inherit `VirtualClientComponent`.
+Constructor: `(IServiceCollection, IDictionary<string, IConvertible>)`.
+Lifecycle: `IsSupported` → `InitializeAsync` → `Validate` → `ExecuteAsync` → `CleanupAsync`.
+
+### Process Execution
+
+External binaries run through `IProcessProxy` (wraps `System.Diagnostics.Process`).
+Output captured via `ConcurrentBuffer`. Tests use `InMemoryProcess`.
+
+## Build and Test
+
+```bash
+# Build
+./build.sh                    # or: dotnet build src/VirtualClient/VirtualClient.sln -c Debug
+
+# Test
+./build-test.sh               # or: dotnet test <project>.csproj -c Debug --filter "Category=Unit"
+
+# Publish
+dotnet publish src/VirtualClient/VirtualClient.Main/VirtualClient.Main.csproj -r linux-x64 -c Release --self-contained
+```
+
+Version is read from the `VERSION` file. Override with `VCBuildVersion` env var.

.github/instructions/client-server-workloads.instructions.md

@@ -0,0 +1,96 @@
+---
+applyTo: "VirtualClient.Actions/**/*.cs"
+description: "Pattern for developing multi-VM client/server/reverseProxy workloads"
+---
+
+# Client/Server Workload Development
+
+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` — topology of instances (`ClientInstance` with Name, Role, IPAddress)
+- `IApiClientManager` — creates API clients for inter-VM communication
+- `ClientRole.Client` / `ClientRole.Server` / `ClientRole.ReverseProxy` — role constants
+- `this.SetServerOnline(bool)` — extension method to signal server readiness
+- `serverApiClient.PollForHeartbeatAsync(timeout, ct)` / `PollForServerOnlineAsync(timeout, ct)`
+
+## Base 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
+{
+    public MyWorkloadExecutor(IServiceCollection dependencies, IDictionary<string, IConvertible> parameters = null)
+        : base(dependencies, parameters)
+    {
+        this.SystemManagement = dependencies.GetService<ISystemManagement>();
+        this.ApiClientManager = dependencies.GetService<IApiClientManager>();
+        this.FileSystem = this.SystemManagement.FileSystem;
+        this.PackageManager = this.SystemManagement.PackageManager;
+        this.ProcessManager = this.SystemManagement.ProcessManager;
+        this.StateManager = this.SystemManagement.StateManager;
+
+        // Set the base class property — do NOT declare a new field
+        this.SupportedRoles = new List<string> { ClientRole.Client, ClientRole.Server };
+    }
+}
+```
+
+## 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
+
+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
+```
+
+## Validation
+
+Override `Validate()` to check layout and roles:
+```csharp
+protected override void Validate()
+{
+    base.Validate();
+    this.ThrowIfLayoutNotDefined();
+}
+```
+
+## Profile Structure
+
+```json
+{
+    "Actions": [
+        { "Type": "MyServerExecutor", "Parameters": { "Role": "Server", "Port": 5000 } },
+        { "Type": "MyClientExecutor", "Parameters": { "Role": "Client", "ServerPort": "$.Parameters.Port" } }
+    ]
+}
+```
+
+## Checklist
+
+- [ ] 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

.github/instructions/csharp.instructions.md

@@ -0,0 +1,89 @@
+---
+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`)
+- **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`)
+
+## Profile Parameter Properties
+
+Properties reading from the `Parameters` dictionary use this pattern:
+
+```csharp
+public string CommandArguments
+{
+    get { return this.Parameters.GetValue<string>(nameof(this.CommandArguments)); }
+}
+
+// With default value:
+public string CompilerName
+{
+    get { return this.Parameters.GetValue<string>(nameof(this.CompilerName), string.Empty); }
+}
+```
+
+## XML Documentation
+
+All public members require XML doc comments with `<summary>`, `<param>`, `<returns>` tags:
+
+```csharp
+/// <summary>
+/// Constructor
+/// </summary>
+/// <param name="dependencies">Provides required dependencies to the component.</param>
+/// <param name="parameters">Parameters defined in the profile or supplied on the command line.</param>
+```
+
+## 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

.github/instructions/metrics-parser.instructions.md

@@ -0,0 +1,97 @@
+---
+applyTo: "**/*MetricsParser.cs"
+description: "Pattern for developing metric parsers with proper units and consistency"
+---
+
+# MetricsParser Development Guide
+
+## Class Structure
+
+Parsers inherit from `MetricsParser` → `TextParser<IList<Metric>>`:
+
+```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<Metric> Parse()
+    {
+        try
+        {
+            this.Preprocess();
+            this.Sections = TextParsingExtensions.Sectionize(this.PreprocessedText, "SectionHeader");
+            List<Metric> metrics = new List<Metric>();
+            // Parse sections and extract metrics
+            return metrics;
+        }
+        catch (Exception exc)
+        {
+            throw new WorkloadResultsException(
+                "Failed to parse results.", exc, ErrorReason.WorkloadResultsParsingFailed);
+        }
+    }
+
+    protected override void Preprocess()
+    {
+        // Remove unwanted lines before parsing
+        this.PreprocessedText = TextParsingExtensions.RemoveRows(this.RawText, @"^\s*$");
+    }
+}
+```
+
+## Standard Units (from `MetricUnit` constants)
+
+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
+
+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()`)

.github/instructions/pr-review.instructions.md

@@ -0,0 +1,48 @@
+---
+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<string, IConvertible>)`.** `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`. 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.
+
+10. **Copyright header on every `.cs` file.** StyleCop SA1633 requires the standard two-line header.
+
+## Suggestions (flag these — coding conventions defined in csharp.instructions.md)
+
+1. **Add `[SupportedPlatforms("...")]` to executor classes** — omitting means workload attempts
+   to run on all platforms.
+
+2. **`Validate()` should throw `WorkloadException` with `ErrorReason.InvalidProfileDefinition`.**
+
+3. **Test classes should inherit `MockFixture`** — not create mocks from scratch.
+
+4. **Parser tests should load real output from `Examples/`** — not inline strings.