Skip to content

Conversation

@rishabhmalikMS
Copy link
Contributor

Context

This PR addresses Azure Pipelines Agent container node selection process and implements a comprehensive strategy pattern for Node.js version selection in container environments.

AB#2343104
AB#2343431
AB#2345319
AB#2345320
AB#2345321


Description

  • Updated strategy pattern for Node.js version selection with container support
  • Added CanHandleInContainer methods to Node24Strategy, Node20Strategy, and Node16Strategy for container-specific logic
  • Updated NodeVersionOrchestrator with dual selection paths: SelectNodeVersionForHostAsync() and SelectNodeVersionForContainer()
  • Integrated container support in ContainerOperationProvider with proper node path detection
  • Removed node --version dependency for container command in ContainerOperationProvider.
  • Enhanced NodeRunnerInfo with ContainerDefaultNode support for cross-platform scenarios
  • Restructured test infrastructure for container tests with comprehensive container test scenarios and improved organization

Risk Assessment (Low / Medium / High)

Medium Risk - This change affects the core node selection logic for container execution. However, risk is mitigated by:

  • Comprehensive strategy pattern with clear fallback chains
  • Extensive test coverage (~15 container-specific tests)
  • Feature can be controlled via existing feature flag (AGENT_USE_NODE_STRATEGY)

Unit Tests Added or Updated (Yes / No)

  • Unit tests were already added in previous PRs.
  • Updated the tests name and description to be more menaingful for container scenarios

Additional Testing Performed

List manual or automated tests performed beyond unit tests (e.g., integration, scenario, regression).


Change Behind Feature Flag (Yes / No)

Yes - The change can be controlled via existing feature mechanisms:

  • AGENT_USE_NODE_STRATEGY controls strategy vs legacy behavior

Tech Design / Approach

  • Design has been written and reviewed.
  • Any architectural decisions, trade-offs, and alternatives are captured.

Documentation Changes Required (Yes/No)

Indicate whether related documentation needs to be updated.

  • User guides, API specs, system diagrams, or runbooks are updated.

Logging Added/Updated (Yes/No)

  • Appropriate log statements are added with meaningful messages.
  • Logging does not expose sensitive data.
  • Log levels are used correctly (e.g., info, warn, error).

Telemetry Added/Updated (Yes/No)

  • Custom telemetry (e.g., counters, timers, error tracking) is added as needed.
  • Events are tagged with proper metadata for filtering and analysis.
  • Telemetry is validated in staging or test environments.

Rollback Scenario and Process (Yes/No)

  • Rollback plan is documented.

Dependency Impact Assessed and Regression Tested (Yes/No)

  • All impacted internal modules, APIs, services, and third-party libraries are analyzed.
  • Results are reviewed and confirmed to not break existing functionality.

…ner support

	- Added CanHandleInContainer methods to Node24Strategy, Node20Strategy, and Node16Strategy for container-specific logic
	- Updated NodeVersionOrchestrator with dual selection paths: SelectNodeVersionForHostAsync() and SelectNodeVersionForContainer()
	- Integrated container support in ContainerOperationProvider with proper node path detection
	- Removed node --version dependency for container command in ContainerOperationProvider.
	- Enhanced NodeRunnerInfo with ContainerDefaultNode support for cross-platform scenarios
	- Restructured test infrastructure for container tests with comprehensive container test scenarios and improved organization
@rishabhmalikMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rishabhmalikMS rishabhmalikMS marked this pull request as ready for review January 14, 2026 05:10
@rishabhmalikMS rishabhmalikMS requested review from a team as code owners January 14, 2026 05:10
private BaseNodeHandlerData GetJobContainerHandlerData(IExecutionContext executionContext, ContainerInfo container)
{
// Custom node takes highest priority
if (!string.IsNullOrEmpty(container.CustomNodePath))
Copy link
Contributor

Choose a reason for hiding this comment

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

if label in dockerfile considered as customnode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, label is considered as custom node

),

new TestScenario(
name: "Container_GlobalNode24Knob_OverridesContainerDefaults",
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a test case if knobs for node24/20 is enabled still custom node will be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is updated.

@rishabhmalikMS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).


// Skip container tests on macOS since they always use cross-platform logic
// This is expected behavior - macOS agent binaries cannot run in typical Linux containers
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
Copy link
Contributor

Choose a reason for hiding this comment

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

Skip macOS logic can we move it to start of this method like this?
[Trait("SkipOn", "darwin")]
This approach skips the entire method for macOS, and I believe this aligns with the pattern we follow in the agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants