From 234b5a744a2677a9a88affdaec2cd7c6c9e2bb63 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Fri, 8 Nov 2024 13:13:49 -0800 Subject: [PATCH 1/3] [pkg/stanza] Pass buffer by reference so buffer changes are not lost (#36252) The root of the crash reported in #36179 was the fact that the `Buffer` struct being passed by value in recursive calls made it allocate the needed amount, but, after the return of the recursive call it attempted to read a buffer that was larger than the allocated buffer on the recursive call. The crash was going to be hit whenever an XML was larger than the default size of the buffer (16KiB). The code is a bit hard to test because its fully usage actually happens on the receiver where the buffer and its components are not visible. I'll look to add a test in a follow-up. cc @djaglowski Skipping changelog since #36179 covers it. --------- Co-authored-by: Daniel Jaglowski --- pkg/stanza/operator/input/windows/bookmark.go | 2 +- pkg/stanza/operator/input/windows/buffer.go | 4 ++-- pkg/stanza/operator/input/windows/buffer_test.go | 11 ----------- pkg/stanza/operator/input/windows/event.go | 6 +++--- pkg/stanza/operator/input/windows/input.go | 2 +- 5 files changed, 7 insertions(+), 18 deletions(-) diff --git a/pkg/stanza/operator/input/windows/bookmark.go b/pkg/stanza/operator/input/windows/bookmark.go index 83ee23f665b9..23a7e8d2f939 100644 --- a/pkg/stanza/operator/input/windows/bookmark.go +++ b/pkg/stanza/operator/input/windows/bookmark.go @@ -54,7 +54,7 @@ func (b *Bookmark) Update(event Event) error { } // Render will render the bookmark as xml. -func (b *Bookmark) Render(buffer Buffer) (string, error) { +func (b *Bookmark) Render(buffer *Buffer) (string, error) { if b.handle == 0 { return "", fmt.Errorf("bookmark handle is not open") } diff --git a/pkg/stanza/operator/input/windows/buffer.go b/pkg/stanza/operator/input/windows/buffer.go index 3d083bb76693..3f939b38f42a 100644 --- a/pkg/stanza/operator/input/windows/buffer.go +++ b/pkg/stanza/operator/input/windows/buffer.go @@ -75,8 +75,8 @@ func (b *Buffer) FirstByte() *byte { } // NewBuffer creates a new buffer with the default buffer size -func NewBuffer() Buffer { - return Buffer{ +func NewBuffer() *Buffer { + return &Buffer{ buffer: make([]byte, defaultBufferSize), } } diff --git a/pkg/stanza/operator/input/windows/buffer_test.go b/pkg/stanza/operator/input/windows/buffer_test.go index 78f654742495..f8376706ec78 100644 --- a/pkg/stanza/operator/input/windows/buffer_test.go +++ b/pkg/stanza/operator/input/windows/buffer_test.go @@ -21,17 +21,6 @@ func TestBufferReadBytes(t *testing.T) { require.Equal(t, utf8, bytes) } -func TestBufferReadBytesOverflow(t *testing.T) { - buffer := NewBuffer() - utf8 := []byte("test") - utf16, _ := unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewEncoder().Bytes(utf8) - copy(buffer.buffer, utf16) - offset := uint32(len(utf16)) - bytes, err := buffer.ReadBytes(offset * 2) - require.NoError(t, err) - require.Equal(t, utf8, bytes) -} - func TestBufferReadWideBytes(t *testing.T) { buffer := NewBuffer() utf8 := []byte("test") diff --git a/pkg/stanza/operator/input/windows/event.go b/pkg/stanza/operator/input/windows/event.go index c55703648d25..9fc2bc28b071 100644 --- a/pkg/stanza/operator/input/windows/event.go +++ b/pkg/stanza/operator/input/windows/event.go @@ -27,7 +27,7 @@ type Event struct { } // GetPublisherName will get the publisher name of the event. -func (e *Event) GetPublisherName(buffer Buffer) (string, error) { +func (e *Event) GetPublisherName(buffer *Buffer) (string, error) { if e.handle == 0 { return "", fmt.Errorf("event handle does not exist") } @@ -77,7 +77,7 @@ func NewEvent(handle uintptr) Event { } // RenderSimple will render the event as EventXML without formatted info. -func (e *Event) RenderSimple(buffer Buffer) (*EventXML, error) { +func (e *Event) RenderSimple(buffer *Buffer) (*EventXML, error) { if e.handle == 0 { return nil, fmt.Errorf("event handle does not exist") } @@ -100,7 +100,7 @@ func (e *Event) RenderSimple(buffer Buffer) (*EventXML, error) { } // RenderDeep will render the event as EventXML with all available formatted info. -func (e *Event) RenderDeep(buffer Buffer, publisher Publisher) (*EventXML, error) { +func (e *Event) RenderDeep(buffer *Buffer, publisher Publisher) (*EventXML, error) { if e.handle == 0 { return nil, fmt.Errorf("event handle does not exist") } diff --git a/pkg/stanza/operator/input/windows/input.go b/pkg/stanza/operator/input/windows/input.go index 5ad0db5c4fb9..a7cace3e7363 100644 --- a/pkg/stanza/operator/input/windows/input.go +++ b/pkg/stanza/operator/input/windows/input.go @@ -24,7 +24,7 @@ import ( type Input struct { helper.InputOperator bookmark Bookmark - buffer Buffer + buffer *Buffer channel string maxReads int startAt string From 68f39ff0e623fcebe8a2ac9129edb63c2c5e4317 Mon Sep 17 00:00:00 2001 From: Argannor <4489279+Argannor@users.noreply.github.com> Date: Fri, 8 Nov 2024 22:15:02 +0100 Subject: [PATCH 2/3] [fix][prometheusexporter] Race condition between start and shutdown (#36164) #### Description Adjusted the Start and Shutdown sequence of the prometheusexporter to prevent a race condition causing the `close tcp 127.0.0.1:8999: use of closed network connection` error as observed in #36139. The behaviour was changed in the following ways: - If an error occurs during the creation of the server, close the listener immediately, leaving shutdown a noop - Since `srv.Shutdown` will close all open listeners, the explicit call to `ln.Close` in the shutdown routine was removed #### Link to tracking issue Fixes #36139 #### Testing Unit testing, I temporarily increased the number of iterations on `TestPrometheusExporter` to 2000. The error did no longer occur. However sporadically another error occured: ``` === RUN TestPrometheusExporter prometheus_test.go:103: Error Trace: C:/development/code/opentelemetry-collector-contrib/exporter/prometheusexporter/prometheus_test.go:84 Error: Received unexpected error: listen tcp 127.0.0.1:8999: bind: Only one usage of each socket address (protocol/network address/port) is normally permitted. Test: TestPrometheusExporter --- FAIL: TestPrometheusExporter (1.16s) ``` I assume that this is because the OS (in my case Windows) won't always close the underlying sockets immediately, blocking it for some time afterwards. I'm not sure there is anything we can do about that. --------- Signed-off-by: Argannor --- ..._prometheusexporter-shutdown-server-2.yaml | 27 +++++++++++++++++++ exporter/prometheusexporter/prometheus.go | 14 ++++------ 2 files changed, 32 insertions(+), 9 deletions(-) create mode 100644 .chloggen/fix_prometheusexporter-shutdown-server-2.yaml diff --git a/.chloggen/fix_prometheusexporter-shutdown-server-2.yaml b/.chloggen/fix_prometheusexporter-shutdown-server-2.yaml new file mode 100644 index 000000000000..7b01af232225 --- /dev/null +++ b/.chloggen/fix_prometheusexporter-shutdown-server-2.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: prometheusexporter + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Fixes a race condition between the exporter start and shutdown functions." + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [36139] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] \ No newline at end of file diff --git a/exporter/prometheusexporter/prometheus.go b/exporter/prometheusexporter/prometheus.go index 438fb3acdd07..8de4fa367e88 100644 --- a/exporter/prometheusexporter/prometheus.go +++ b/exporter/prometheusexporter/prometheus.go @@ -66,16 +66,12 @@ func (pe *prometheusExporter) Start(ctx context.Context, host component.Host) er mux := http.NewServeMux() mux.Handle("/metrics", pe.handler) srv, err := pe.config.ToServer(ctx, host, pe.settings, mux) - pe.shutdownFunc = func(ctx context.Context) error { - errLn := ln.Close() - if srv == nil { - return errLn - } - errSrv := srv.Shutdown(ctx) - return errors.Join(errLn, errSrv) - } if err != nil { - return err + lnerr := ln.Close() + return errors.Join(err, lnerr) + } + pe.shutdownFunc = func(ctx context.Context) error { + return srv.Shutdown(ctx) } go func() { _ = srv.Serve(ln) From 5618c7cc43bfc3614ab30e5eb0b75cad50be2bb2 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Fri, 8 Nov 2024 13:15:27 -0800 Subject: [PATCH 3/3] [test] Fix opampsupervisor test on Windows (#36282) Fix #36278 The issue is that the temporary path used in the test is passed as text to an yaml file so the Windows dir separator ends up as a escape char on the yaml. --- cmd/opampsupervisor/e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/opampsupervisor/e2e_test.go b/cmd/opampsupervisor/e2e_test.go index c0e080faa1e2..8e106b2b65f6 100644 --- a/cmd/opampsupervisor/e2e_test.go +++ b/cmd/opampsupervisor/e2e_test.go @@ -188,7 +188,7 @@ func getSupervisorConfig(t *testing.T, configType string, extraConfigData map[st "goos": runtime.GOOS, "goarch": runtime.GOARCH, "extension": extension, - "storage_dir": t.TempDir(), + "storage_dir": strings.ReplaceAll(t.TempDir(), "\\", "\\\\"), } for key, val := range extraConfigData {