internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2523

andrewkroh · 2023-04-19T22:30:46Z

What does this PR do?

Prior to this change sub-directories within logs/ were not created properly and resulted in unexpected directories within diagnostics files. A path separator was missing. Before the change the new test was failing with

-  Name: (string) (len=13) "logs/sub-dir/",
+  Name: (string) (len=12) "logssub-dir/",

Why is it important?

It makes the diag zip file layout match what's on the agent host filesystem. The diag file should be correct to be trustworthy.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

Prior to this change sub-directories within logs/ were not created properly and resulted in unexpected directories within diagnostics files. A path separator was missing. Before the change the new test was failing with - Name: (string) (len=13) "logs/sub-dir/", + Name: (string) (len=12) "logssub-dir/",

mergify · 2023-04-19T22:31:22Z

This pull request does not have a backport label. Could you fix it @andrewkroh? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2023-04-19T22:33:57Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-04-20T13:57:25.640+0000
Duration: 16 min 20 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5539
Skipped	19
Total	5558

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

ycombinator

LGTM. Thanks for the fix and the tests.

elasticmachine · 2023-04-19T22:53:53Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.551% (`68/69`)	👍 0.021
Files	68.354% (`162/237`)	👍 0.134
Classes	67.857% (`304/448`)	👍 0.072
Methods	54.029% (`932/1725`)	👎 -0.082
Lines	39.434% (`10567/26797`)	👎 -0.176
Conditionals	100.0% (`0/0`)	💚

michalpristas

LGTM

michalpristas · 2023-04-20T09:37:21Z

can you pull latest main, I made some changes in this area and want to avoid some silent conflicts

…p-logs-dirs

I made the test pass, but this these exposes a problem with the newly added elastic-agent-{commit} directory in that the zip file is missing a directory entry for it.

andrewkroh · 2023-04-20T13:12:45Z

internal/pkg/diagnostics/dianostics_test.go

+
+	// Verify the results.
+	expected := []zippedItem{
+		{"logs/", true},


@michalpristas I pulled in the latest code and this test points out a problem with the newly added elastic-agent-{commit} directory in that it is missing a directory entry within the Zip file.

These directory entries may not be required, but IIRC depending on how robust a particular unzip tool used is, this can lead to problems when unzipping. Sometimes a tool will try to write files into a directory that doesn't exist and fail (or worse, silently ignore the problem and not write any of the files under that dir).

The test is passing as is (at least on posix), ~~so this could be followed up in a separate change.~~ Fixed everything I found here.

andrewkroh · 2023-04-20T13:44:13Z

On Windows the test appears to have detected another issue:

-  Name: (string) (len=44) "logs/elastic-agent-unknow/sub-dir/log.ndjson",
+  Name: (string) (len=44) "logs/elastic-agent-unknow\\sub-dir\\log.ndjson",

I think this value needs to be filepath.ToSlash()ed for Windows when it is used to construct names within the zip. (also occurs in saveLogs)

elastic-agent/internal/pkg/diagnostics/diagnostics.go

Line 338 in 383752b

name = filepath.Join(commitName, name)

That's based on the comment and code at

elastic-agent/internal/pkg/diagnostics/diagnostics.go

Lines 332 to 333 in 383752b

    
           // this will clean log names on windows machines and will nop on *nix 
        
           name := filepath.ToSlash(strings.TrimPrefix(path, logPath))

And according to https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

All slashes MUST be forward slashes '/' as opposed to backwards slashes '\'

The zip file did not contain a directory entry for the elastic-agent-{commit} directory. Relates elastic#2518

This calls filepath.ToSlash on any names used in zip entries. Relates elastic#2518

michalpristas

thanks again for handling the issues

…2523) * internal/pkg/diagnostics - Fix creation of sub-directories in logs/ Prior to this change sub-directories within logs/ were not created properly and resulted in unexpected directories within diagnostics files. A path separator was missing. Before the change the new test was failing with - Name: (string) (len=13) "logs/sub-dir/", + Name: (string) (len=12) "logssub-dir/", * Update test to include the versioned sub-directory I made the test pass, but this these exposes a problem with the newly added elastic-agent-{commit} directory in that the zip file is missing a directory entry for it. * Add missing directory entry for logs/elastic-agent-{commit} The zip file did not contain a directory entry for the elastic-agent-{commit} directory. Relates #2518 * All files within the zip must use forward slash This calls filepath.ToSlash on any names used in zip entries. Relates #2518 (cherry picked from commit 91a3122)

andrewkroh added the bug Something isn't working label Apr 19, 2023

andrewkroh requested a review from a team as a code owner April 19, 2023 22:30

andrewkroh requested review from AndersonQ and michalpristas and removed request for a team April 19, 2023 22:30

andrewkroh force-pushed the bugfix/diagnostic-zip-logs-dirs branch from dcf6a92 to f83ea6c Compare April 19, 2023 22:31

mergify bot assigned andrewkroh Apr 19, 2023

mergify bot added the backport-skip label Apr 19, 2023

andrewkroh added backport-v8.7.0 Automated backport with mergify and removed backport-skip labels Apr 19, 2023

ycombinator approved these changes Apr 19, 2023

View reviewed changes

michalpristas approved these changes Apr 20, 2023

View reviewed changes

andrewkroh added 2 commits April 20, 2023 08:51

Merge remote-tracking branch 'elastic/main' into bugfix/diagnostic-zi…

19dc073

…p-logs-dirs

Update test to include the versioned sub-directory

eea8a87

I made the test pass, but this these exposes a problem with the newly added elastic-agent-{commit} directory in that the zip file is missing a directory entry for it.

andrewkroh force-pushed the bugfix/diagnostic-zip-logs-dirs branch from 48d6ca1 to eea8a87 Compare April 20, 2023 13:04

andrewkroh commented Apr 20, 2023

View reviewed changes

andrewkroh added 2 commits April 20, 2023 09:53

Add missing directory entry for logs/elastic-agent-{commit}

f72186f

The zip file did not contain a directory entry for the elastic-agent-{commit} directory. Relates elastic#2518

All files within the zip must use forward slash

204912f

This calls filepath.ToSlash on any names used in zip entries. Relates elastic#2518

andrewkroh added backport-skip backport-v8.7.0 Automated backport with mergify and removed backport-v8.7.0 Automated backport with mergify backport-skip labels Apr 20, 2023

michalpristas approved these changes Apr 21, 2023

View reviewed changes

michalpristas merged commit 91a3122 into elastic:main Apr 21, 2023

mergify bot mentioned this pull request Apr 21, 2023

[8.7](backport #2523) internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2538

Merged

This was referenced Apr 24, 2023

Add Fleet & Agent 8.7.1 release notes elastic/ingest-docs#150

Merged

Fix up 8.7.1 release notes #2577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2523

internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2523

andrewkroh commented Apr 19, 2023 •

edited

Loading

mergify bot commented Apr 19, 2023

elasticmachine commented Apr 19, 2023 •

edited

Loading

Build stats

Test stats 🧪

ycombinator left a comment

elasticmachine commented Apr 19, 2023 •

edited

Loading

michalpristas left a comment

michalpristas commented Apr 20, 2023

andrewkroh Apr 20, 2023 •

edited

Loading

andrewkroh commented Apr 20, 2023

michalpristas left a comment

internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2523

internal/pkg/diagnostics - Fix creation of sub-directories in logs/ #2523

Conversation

andrewkroh commented Apr 19, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

mergify bot commented Apr 19, 2023

elasticmachine commented Apr 19, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

ycombinator left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 19, 2023 • edited Loading

🌐 Coverage report

michalpristas left a comment

Choose a reason for hiding this comment

michalpristas commented Apr 20, 2023

andrewkroh Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

andrewkroh commented Apr 20, 2023

michalpristas left a comment

Choose a reason for hiding this comment

andrewkroh commented Apr 19, 2023 •

edited

Loading

elasticmachine commented Apr 19, 2023 •

edited

Loading

elasticmachine commented Apr 19, 2023 •

edited

Loading

andrewkroh Apr 20, 2023 •

edited

Loading