Skip to content
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

Netdump enhancements #3307

Merged
merged 2 commits into from
Jul 6, 2023
Merged

Conversation

milan-zededa
Copy link
Contributor

Two small commits with netdump enhancements that could be useful for troubleshooting of customer issues.

netdump: Add option to capture HTTP field values for download requests
This can be useful to troubleshoot download failures caused by MITM
changing the HTTP header fields (proxy, CDN, etc.).
The option is disabled by default becase HTTP header field values may contain
sensitive information, such as datastore credentials.

Ensure we get at least one netdump in each topic for tested EVE upgrade
If upgrade fails, the recorded netdumps will remain persisted and
available for retrieval from the older EVE version.

This can be useful to troubleshoot download failures caused by MITM
changing the HTTP header fields (proxy, CDN, etc.).
The option is disabled by default becase HTTP header field values may contain
sensitive information, such as datastore credentials.

Signed-off-by: Milan Lenco <milan@zededa.com>
Copy link
Member

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

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

LGTM, like this size of PRs way more than 9k+ LOC :D

lastNetdumpAge := time.Since(ctx.lastInfoNetdumpPub)
// Ensure we get at least one netdump for the currently tested EVE upgrade.
if zboot.IsCurrentPartitionStateInProgress() &&
(ctx.lastInfoNetdumpPub.IsZero() || lastNetdumpAge > uptime) {
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if EVE will reboot during download in process?

Copy link
Contributor Author

@milan-zededa milan-zededa Jul 3, 2023

Choose a reason for hiding this comment

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

Netdump is collected after download completes. So if interrupted, EVE would restart download and netdump would be collected afterwards. Note that for downloader microservice we always collect netdumps - this change is for zedagent and nim to collect at least one netdump for requests towards the controller during upgrade. With reboot during upgrade we may end up collecting more, but that is OK. Not having any netdump for failed upgrade would be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uncleDecart Will you please re-approve? Somehow adding comments makes eden tests skipped :/

Copy link
Member

Choose a reason for hiding this comment

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

Great and in case of reboot it'll be 2 different logs which we will be able to differentiate

@milan-zededa
Copy link
Contributor Author

LGTM, like this size of PRs way more than 9k+ LOC :D

Don't worry, I have two larger PRs coming up ;)

Copy link
Member

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

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

Let's run eden!

@milan-zededa
Copy link
Contributor Author

@uncleDecart Eden really does not like you ಥ‿ಥ

@@ -1087,8 +1088,15 @@ func traceNextConfigReq(ctx *zedagentContext) bool {
if !isNettraceEnabled(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this and the corresponding info function only differ in the last*NetdumpPub they check and the functions are getting more complicated. Would it make sense to introduce a common function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - done

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM but some nits.

@milan-zededa milan-zededa force-pushed the netdump-enhancements branch from 3fb6ac6 to ff66cd5 Compare July 4, 2023 15:24
If upgrade fails, the recorded netdumps will remain persisted and
available for retrieval from the older EVE version.

Signed-off-by: Milan Lenco <milan@zededa.com>
@milan-zededa milan-zededa force-pushed the netdump-enhancements branch from ff66cd5 to eb1e506 Compare July 4, 2023 15:29
@milan-zededa
Copy link
Contributor Author

@uncleDecart Could you please hit approve again to trigger eden tests :)

Copy link
Member

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

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

Let the test begin

@milan-zededa milan-zededa merged commit f6de6ea into lf-edge:master Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants