From 7b79dd61bd541f492d00dfe8140a09c17ba9ba5c Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Mon, 2 Oct 2023 10:55:07 -0700 Subject: [PATCH] Ignore regression update when the work item is in some states (#3532) * Ignore regression update when the work item is in some states * format * formatting * don't hide messages in the poison queue * fix typo * update regression logic update test_template to support regression * build fix * mypy fix * build fix * move regression ignore state under ADODuplicateTemplate * replace extend with append * update set_tcp_keepalive * mke mypy happy * copy ADODuplicateTemplate.OnDuplicate.RegressionIgnoreStates --- docs/webhook_events.md | 8 +++ .../ApiService/Functions/QueueFileChanges.cs | 24 ++++--- .../ApiService/OneFuzzTypes/Model.cs | 5 +- .../ApiService/OneFuzzTypes/Requests.cs | 2 +- .../ApiService/onefuzzlib/Reports.cs | 20 ++++++ .../onefuzzlib/notifications/Ado.cs | 67 +++++++++++-------- src/cli/onefuzz/cli.py | 13 +++- src/cli/onefuzz/debug.py | 41 +++++++++--- src/pytypes/onefuzztypes/models.py | 2 + src/pytypes/onefuzztypes/requests.py | 5 +- 10 files changed, 132 insertions(+), 55 deletions(-) diff --git a/docs/webhook_events.md b/docs/webhook_events.md index a417b7465f..cd8c5932f6 100644 --- a/docs/webhook_events.md +++ b/docs/webhook_events.md @@ -2033,6 +2033,10 @@ If webhook is set to have Event Grid message format then the payload will look a }, "original_crash_test_result": { "$ref": "#/definitions/CrashTestResult" + }, + "report_url": { + "title": "Report Url", + "type": "string" } }, "required": [ @@ -6427,6 +6431,10 @@ If webhook is set to have Event Grid message format then the payload will look a }, "original_crash_test_result": { "$ref": "#/definitions/CrashTestResult" + }, + "report_url": { + "title": "Report Url", + "type": "string" } }, "required": [ diff --git a/src/ApiService/ApiService/Functions/QueueFileChanges.cs b/src/ApiService/ApiService/Functions/QueueFileChanges.cs index f1c4711f9d..9e22f113ad 100644 --- a/src/ApiService/ApiService/Functions/QueueFileChanges.cs +++ b/src/ApiService/ApiService/Functions/QueueFileChanges.cs @@ -128,20 +128,22 @@ private async Async.Task RequeueMessage(string msg, TimeSpan? visibilityTimeout newCustomDequeueCount = json["data"]!["customDequeueCount"]!.GetValue(); } - var queueName = QueueFileChangesQueueName; if (newCustomDequeueCount > MAX_DEQUEUE_COUNT) { _log.LogWarning("Message retried more than {MAX_DEQUEUE_COUNT} times with no success: {msg}", MAX_DEQUEUE_COUNT, msg); - queueName = QueueFileChangesPoisonQueueName; + await _context.Queue.QueueObject( + QueueFileChangesPoisonQueueName, + json, + StorageType.Config) + .IgnoreResult(); + } else { + json!["data"]!["customDequeueCount"] = newCustomDequeueCount + 1; + await _context.Queue.QueueObject( + QueueFileChangesQueueName, + json, + StorageType.Config, + visibilityTimeout ?? CalculateExponentialBackoff(newCustomDequeueCount)) + .IgnoreResult(); } - - json!["data"]!["customDequeueCount"] = newCustomDequeueCount + 1; - - await _context.Queue.QueueObject( - queueName, - json, - StorageType.Config, - visibilityTimeout ?? CalculateExponentialBackoff(newCustomDequeueCount)) - .IgnoreResult(); } // Possible return values: diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs index 424669899a..3d67de106d 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Model.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Model.cs @@ -678,7 +678,8 @@ public record ADODuplicateTemplate( Dictionary SetState, Dictionary AdoFields, string? Comment = null, - List>? Unless = null + List>? Unless = null, + List? RegressionIgnoreStates = null ); public record AdoTemplate( @@ -707,7 +708,7 @@ public record RenderedAdoTemplate( ADODuplicateTemplate OnDuplicate, Dictionary? AdoDuplicateFields = null, string? Comment = null - ) : AdoTemplate(BaseUrl, AuthToken, Project, Type, UniqueFields, AdoFields, OnDuplicate, AdoDuplicateFields, Comment); + ) : AdoTemplate(BaseUrl, AuthToken, Project, Type, UniqueFields, AdoFields, OnDuplicate, AdoDuplicateFields, Comment) { } public record TeamsTemplate(SecretData Url) : NotificationTemplate { public Task Validate() { diff --git a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs index 8f3d16aa63..f3cc407b15 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs @@ -131,7 +131,7 @@ public record NotificationSearch( public record NotificationTest( - [property: Required] Report Report, + [property: Required] IReport Report, [property: Required] Notification Notification ) : BaseRequest; diff --git a/src/ApiService/ApiService/onefuzzlib/Reports.cs b/src/ApiService/ApiService/onefuzzlib/Reports.cs index c1c4aad3be..fdda7259e9 100644 --- a/src/ApiService/ApiService/onefuzzlib/Reports.cs +++ b/src/ApiService/ApiService/onefuzzlib/Reports.cs @@ -1,7 +1,10 @@ using System.Text.Json; +using System.Text.Json.Serialization; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.OneFuzz.Service.OneFuzzLib.Orm; + + namespace Microsoft.OneFuzz.Service; public interface IReports { @@ -85,6 +88,7 @@ public static IReport ParseReportOrRegression(string content, Uri reportUrl) { } } +[JsonConverter(typeof(ReportConverter))] public interface IReport { Uri? ReportUrl { init; @@ -95,3 +99,19 @@ public string FileName() { return string.Concat(segments); } }; + +public class ReportConverter : JsonConverter { + + public override IReport? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { + using var templateJson = JsonDocument.ParseValue(ref reader); + + if (templateJson.RootElement.TryGetProperty("crash_test_result", out _)) { + return templateJson.Deserialize(options); + } + return templateJson.Deserialize(options); + } + + public override void Write(Utf8JsonWriter writer, IReport value, JsonSerializerOptions options) { + throw new NotImplementedException(); + } +} diff --git a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs index b1442851ba..2b01afb37f 100644 --- a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs +++ b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs @@ -19,6 +19,7 @@ public class Ado : NotificationsBase, IAdo { // https://github.com/MicrosoftDocs/azure-devops-docs/issues/5890#issuecomment-539632059 private const int MAX_SYSTEM_TITLE_LENGTH = 128; private const string TITLE_FIELD = "System.Title"; + private static List DEFAULT_REGRESSION_IGNORE_STATES = new() { "New", "Commited", "Active" }; public Ado(ILogger logTracer, IOnefuzzContext context) : base(logTracer, context) { } @@ -56,7 +57,7 @@ public async Async.Task NotifyAdo(AdoTemplate config, Contain _logTracer.LogEvent(adoEventType); try { - await ProcessNotification(_context, container, filename, config, report, _logTracer, notificationInfo); + await ProcessNotification(_context, container, filename, config, report, _logTracer, notificationInfo, isRegression: reportable is RegressionReport); } catch (Exception e) when (e is VssUnauthorizedException || e is VssAuthenticationException || e is VssServiceException) { if (config.AdoFields.TryGetValue("System.AssignedTo", out var assignedTo)) { @@ -298,7 +299,7 @@ private static async Async.Task> GetValidFiel .ToDictionary(field => field.ReferenceName.ToLowerInvariant()); } - private static async Async.Task ProcessNotification(IOnefuzzContext context, Container container, string filename, AdoTemplate config, Report report, ILogger logTracer, IList<(string, string)> notificationInfo, Renderer? renderer = null) { + private static async Async.Task ProcessNotification(IOnefuzzContext context, Container container, string filename, AdoTemplate config, Report report, ILogger logTracer, IList<(string, string)> notificationInfo, Renderer? renderer = null, bool isRegression = false) { if (!config.AdoFields.TryGetValue(TITLE_FIELD, out var issueTitle)) { issueTitle = "{{ report.crash_site }} - {{ report.executable }}"; } @@ -311,7 +312,7 @@ private static async Async.Task ProcessNotification(IOnefuzzContext context, Con var renderedConfig = RenderAdoTemplate(logTracer, renderer, config, instanceUrl); var ado = new AdoConnector(renderedConfig, project!, client, instanceUrl, logTracer, await GetValidFields(client, project)); - await ado.Process(notificationInfo); + await ado.Process(notificationInfo, isRegression); } public static RenderedAdoTemplate RenderAdoTemplate(ILogger logTracer, Renderer renderer, AdoTemplate original, Uri instanceUrl) { @@ -352,7 +353,8 @@ public static RenderedAdoTemplate RenderAdoTemplate(ILogger logTracer, Renderer original.OnDuplicate.SetState, onDuplicateAdoFields, original.OnDuplicate.Comment != null ? Render(renderer, original.OnDuplicate.Comment, instanceUrl, logTracer) : null, - onDuplicateUnless + onDuplicateUnless, + original.OnDuplicate.RegressionIgnoreStates ); return new RenderedAdoTemplate( @@ -598,7 +600,7 @@ private async Async.Task CreateNew() { return (taskType, document); } - public async Async.Task Process(IList<(string, string)> notificationInfo) { + public async Async.Task Process(IList<(string, string)> notificationInfo, bool isRegression) { var updated = false; WorkItem? oldestWorkItem = null; await foreach (var workItem in ExistingWorkItems(notificationInfo)) { @@ -612,6 +614,13 @@ public async Async.Task Process(IList<(string, string)> notificationInfo) { continue; } + var regressionStatesToIgnore = _config.OnDuplicate.RegressionIgnoreStates != null ? _config.OnDuplicate.RegressionIgnoreStates : DEFAULT_REGRESSION_IGNORE_STATES; + if (isRegression) { + var state = (string)workItem.Fields["System.State"]; + if (regressionStatesToIgnore.Contains(state, StringComparer.InvariantCultureIgnoreCase)) + continue; + } + using (_logTracer.BeginScope("Non-duplicate work item")) { _logTracer.AddTags(new List<(string, string)> { ("NonDuplicateWorkItemId", $"{workItem.Id}") }); _logTracer.LogInformation("Found matching non-duplicate work item"); @@ -621,30 +630,32 @@ public async Async.Task Process(IList<(string, string)> notificationInfo) { updated = true; } - if (!updated) { - if (oldestWorkItem != null) { - // We have matching work items but all are duplicates - _logTracer.AddTags(notificationInfo); - _logTracer.LogInformation($"All matching work items were duplicates, re-opening the oldest one"); - var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo); - if (stateChanged) { - // add a comment if we re-opened the bug - _ = await _client.AddCommentAsync( - new CommentCreate() { - Text = - "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate." - }, - _project, - (int)oldestWorkItem.Id!); - } - } else { - // We never saw a work item like this before, it must be new - var entry = await CreateNew(); - var adoEventType = "AdoNewItem"; - _logTracer.AddTags(notificationInfo); - _logTracer.AddTag("WorkItemId", entry.Id.HasValue ? entry.Id.Value.ToString() : ""); - _logTracer.LogEvent(adoEventType); + if (updated || isRegression) { + return; + } + + if (oldestWorkItem != null) { + // We have matching work items but all are duplicates + _logTracer.AddTags(notificationInfo); + _logTracer.LogInformation($"All matching work items were duplicates, re-opening the oldest one"); + var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo); + if (stateChanged) { + // add a comment if we re-opened the bug + _ = await _client.AddCommentAsync( + new CommentCreate() { + Text = + "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate." + }, + _project, + (int)oldestWorkItem.Id!); } + } else { + // We never saw a work item like this before, it must be new + var entry = await CreateNew(); + var adoEventType = "AdoNewItem"; + _logTracer.AddTags(notificationInfo); + _logTracer.AddTag("WorkItemId", entry.Id.HasValue ? entry.Id.Value.ToString() : ""); + _logTracer.LogEvent(adoEventType); } } diff --git a/src/cli/onefuzz/cli.py b/src/cli/onefuzz/cli.py index 4456f52d49..2ae55046cf 100644 --- a/src/cli/onefuzz/cli.py +++ b/src/cli/onefuzz/cli.py @@ -28,6 +28,7 @@ Type, TypeVar, Union, + cast, ) from uuid import UUID @@ -551,8 +552,16 @@ def set_tcp_keepalive() -> None: # Azure Load Balancer default timeout (4 minutes) # # https://urllib3.readthedocs.io/en/stable/reference/urllib3.connection.html?highlight=keep-alive#:~:text=For%20example%2C%20if,socket.SO_KEEPALIVE%2C%201)%2C%0A%5D - if value not in urllib3.connection.HTTPConnection.default_socket_options: - urllib3.connection.HTTPConnection.default_socket_options.extend((value,)) + + default_socket_options = cast( + List[Tuple[int, int, int]], + urllib3.connection.HTTPConnection.default_socket_options, + ) + + if value not in default_socket_options: + default_socket_options + [ + value, + ] def execute_api(api: Any, api_types: List[Any], version: str) -> int: diff --git a/src/cli/onefuzz/debug.py b/src/cli/onefuzz/debug.py index cde03adf57..0182cb19a1 100644 --- a/src/cli/onefuzz/debug.py +++ b/src/cli/onefuzz/debug.py @@ -21,7 +21,15 @@ from azure.storage.blob import ContainerClient from onefuzztypes import models, requests, responses from onefuzztypes.enums import ContainerType, TaskType -from onefuzztypes.models import BlobRef, Job, NodeAssignment, Report, Task, TaskConfig +from onefuzztypes.models import ( + BlobRef, + Job, + NodeAssignment, + RegressionReport, + Report, + Task, + TaskConfig, +) from onefuzztypes.primitives import Container, Directory, PoolName from onefuzztypes.responses import TemplateValidationResponse @@ -633,35 +641,50 @@ def test_template( self, notificationConfig: models.NotificationConfig, task_id: Optional[UUID] = None, - report: Optional[Report] = None, + report: Optional[str] = None, ) -> responses.NotificationTestResponse: """Test a notification template""" + the_report: Union[Report, RegressionReport, None] = None + + if report is not None: + try: + the_report = RegressionReport.parse_raw(report) + print("testing regression report") + except Exception: + the_report = Report.parse_raw(report) + print("testing normal report") + if task_id is not None: task = self.onefuzz.tasks.get(task_id) - if report is None: + if the_report is None: input_blob_ref = BlobRef( account="dummy-storage-account", container="test-notification-crashes", name="fake-crash-sample", ) - report = self._create_report( + the_report = self._create_report( task.job_id, task.task_id, "fake_target.exe", input_blob_ref ) + elif isinstance(the_report, RegressionReport): + if the_report.crash_test_result.crash_report is None: + raise Exception("invalid regression report: no crash report") + the_report.crash_test_result.crash_report.task_id = task.task_id + the_report.crash_test_result.crash_report.job_id = task.job_id else: - report.task_id = task.task_id - report.job_id = task.job_id - elif report is None: + the_report.task_id = task.task_id + the_report.job_id = task.job_id + elif the_report is None: raise Exception("must specify either task_id or report") - report.report_url = "https://dummy-container.blob.core.windows.net/dummy-reports/dummy-report.json" + the_report.report_url = "https://dummy-container.blob.core.windows.net/dummy-reports/dummy-report.json" endpoint = Endpoint(self.onefuzz) return endpoint._req_model( "POST", responses.NotificationTestResponse, data=requests.NotificationTest( - report=report, + report=the_report, notification=models.Notification( container=Container("test-notification-reports"), notification_id=uuid.uuid4(), diff --git a/src/pytypes/onefuzztypes/models.py b/src/pytypes/onefuzztypes/models.py index c888621600..4b115a3c79 100644 --- a/src/pytypes/onefuzztypes/models.py +++ b/src/pytypes/onefuzztypes/models.py @@ -256,6 +256,7 @@ class CrashTestResult(BaseModel): class RegressionReport(BaseModel): crash_test_result: CrashTestResult original_crash_test_result: Optional[CrashTestResult] + report_url: Optional[str] class ADODuplicateTemplate(BaseModel): @@ -263,6 +264,7 @@ class ADODuplicateTemplate(BaseModel): comment: Optional[str] set_state: Dict[str, str] ado_fields: Dict[str, str] + regression_ignore_states: Optional[List[str]] class ADOTemplate(BaseModel): diff --git a/src/pytypes/onefuzztypes/requests.py b/src/pytypes/onefuzztypes/requests.py index ae6da006e0..d284fb416d 100644 --- a/src/pytypes/onefuzztypes/requests.py +++ b/src/pytypes/onefuzztypes/requests.py @@ -3,7 +3,7 @@ # Copyright (c) Microsoft Corporation. # Licensed under the MIT License. -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Union from uuid import UUID from pydantic import AnyHttpUrl, BaseModel, Field, root_validator @@ -26,6 +26,7 @@ AutoScaleConfig, InstanceConfig, NotificationConfig, + RegressionReport, Report, TemplateRenderContext, ) @@ -280,7 +281,7 @@ class EventsGet(BaseModel): class NotificationTest(BaseModel): - report: Report + report: Union[Report, RegressionReport] notification: models.Notification