-
Notifications
You must be signed in to change notification settings - Fork 914
Update L1 for Container Label #5449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| SetupL1(); | ||
| var message = LoadTemplateMessage(); | ||
|
|
||
| var containerResource = new Microsoft.TeamFoundation.DistributedTask.Pipelines.ContainerResource() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating object of type Microsoft.TeamFoundation.DistributedTask.Pipelines.ContainerResource, you can import it and create object of just ContainerResource. This would keep code clean.
Same for Code in line 82 for AgentJobRequestMessage
| private async Task CreateTestContainerImageWindows() | ||
| { | ||
| string dockerfile = $@"FROM mcr.microsoft.com/windows/servercore/insider:10.0.20348.1 | ||
| LABEL ""{ContainerLabelKey}""=""{CustomNodePathWindows}"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the indentation here, same for below code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| var steps = GetSteps(); | ||
| Assert.NotNull(steps); | ||
| Assert.True(steps.Count() >= 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests don't actually verify that the container label is being read and used. The assertions only check that steps exist or not. Can we validate that the custom node via label is actually used vs the agent's build in node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are more like verifying that the agent doesn't crash when running containerized jobs with labels and without labels, as we don't have any container L1 tests for the agent. The label checking logic is mostly covered in the L0 PR that I have raised. Though I'll try to check if it's possible to validate custom node via label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if label checking logic is part of L0 tests, how this validates worker crashed or not?
what are the scenario in which worker will crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the ValidateNodePathUsage method to validate that the custom node path from the Docker label appears in the container execution logs.
| private const string TestImageNameLinux = "azure-pipelines-agent-test-container-label-linux"; | ||
| private const string TestImageNameWindowsNoLabel = "azure-pipelines-agent-test-container-nolabel-windows"; | ||
| private const string TestImageNameLinuxNoLabel = "azure-pipelines-agent-test-container-nolabel-linux"; | ||
| private const string CustomNodePathWindows = "C:\\Program Files\\nodejs\\node.exe"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we validate this path in ci/cd pipelines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI/CD validates the path by running a real container job and asserting that pathe appears in the execution logs. in method ValidateNodePathUsage
|
|
||
| var steps = GetSteps(); | ||
| Assert.NotNull(steps); | ||
| Assert.True(steps.Count() >= 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if label checking logic is part of L0 tests, how this validates worker crashed or not?
what are the scenario in which worker will crash
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azo run |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Context
This change implements L1 tests for containers using Docker images, both with and without labels.
Work-Item [AB#2346693]- https://dev.azure.com/mseng/AzureDevOps/_workitems/edit/2346693
Description
Integration tests validating that the Azure Pipelines Agent reads the com.azure.dev.pipelines.agent.handler.node.path container label to resolve custom Node.js paths. Includes four platform-specific tests (Windows and Linux) that verify both label-based resolution and fallback behavior when the label is absent.
Risk Assessment (Low / Medium / High)
Low
Unit Tests Added or Updated (Yes / No)
Yes
Additional Testing Performed
NA
Change Behind Feature Flag (Yes / No)
No
Tech Design / Approach
Documentation Changes Required (Yes/No)
Indicate whether related documentation needs to be updated.
Logging Added/Updated (Yes/No)
Telemetry Added/Updated (Yes/No)
Rollback Scenario and Process (Yes/No)
Dependency Impact Assessed and Regression Tested (Yes/No)