DAOS-10625 control: Create the tool to collect the logs/config for support purpose #11094

ravalsam · 2022-12-20T21:49:11Z

1> Adding dmg support collectlog option collect the logs/configs from servers.
2> Adding daos_server support collectlog option to get the system and server side information in case dmg Management layer is not working for some reason.
3> Adding daos_admin support collectlog option to get the client side logs and configs.
4> Added unit test for support lib
5> Added functional tests for dmg,daos_agent and daos_server support options

Signed-off-by: Samir Raval [email protected]

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

github-actions · 2022-12-20T21:49:31Z

Bug-tracker data:
Ticket title is 'Provide tool which can collect the logs/configuration/matric from the customer sites for support purpose.'
Status is 'In Review'
Labels: 'sustaining_internal'
Errors are Title of PR is too long
https://daosio.atlassian.net/browse/DAOS-10625

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2022-12-21T05:54:12Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11094/1/execution/node/1092/log

…pport purpose. *** WORK IN PROGRESS *** 1> Adding dmg support collectlog option collect the logs/configs from servers. 2> Adding daos_server support collectlog option to get the system and server side information incase dmg Management layer is not working for some reason. 3> Adding daos_admin support collectlog option to get the client side logs and configs. --- To BE DONE --- 1> Unit testing still in progress. 2> All command which collect the system related information still needs to be added. 3> Functional testing to be done with this PR. Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2022-12-21T21:44:48Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-11094/3/testReport/(root)/

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

tanabarr

some initial observations (most points refer to multiple occurrences in the PR):

if b == true can be shortened to if b

initialising to an empty value i.e. LogCollection["..."] = []string{""} it is rarely necessary as zero values are usually good enough un-initialised (one of the useful things of go)

If using string constants as keys, define them e.g. const CopyServerConfigCmd = "CopyServerConfig", not convinced this is the best use of the map though, you might be able to define enum for the keys

progress.Total = progress.Total + 1 can be shortened to progress.Total++

The code for the daos_server and dmg command variants should be consolidated into shared helper functions to reduce duplication.

tanabarr · 2023-01-11T12:18:42Z

src/control/cmd/dmg/support.go

+			}
+			if len(resp.GetHostErrors()) > 0 {
+				var bld strings.Builder
+				_ = pretty.PrintResponseErrors(resp, &bld)


errors should always be handled

tanabarr · 2023-01-11T12:19:04Z

src/control/cmd/dmg/support.go

+	}
+
+	// Rsync the logs from servers
+	hostName, _ := support.GetHostName()


errors should always be handled

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

kjacque

A lot of code! Most of my comments are Go language tips and/or naming suggestions, and can be applied in multiple places.

I haven't tried running it so can't speak to any oddities there.

src/control/cmd/daos_agent/support.go

kjacque · 2023-01-12T00:40:01Z

src/control/cmd/daos_agent/support.go

+type collectLogCmd struct {
+	configCmd
+	cmdutil.LogCmd
+	Stop         bool   `short:"s" long:"Stop" description:"Stop the collectlog command on very first error"`


suggestion - Maybe "stop-on-error" for the long name? As it is, this option feels a bit confusing.

kjacque · 2023-01-12T00:40:30Z

src/control/cmd/daos_agent/support.go

+	configCmd
+	cmdutil.LogCmd
+	Stop         bool   `short:"s" long:"Stop" description:"Stop the collectlog command on very first error"`
+	TargetFolder string `short:"t" long:"loglocation" description:"Folder location where log is going to be copied"`


suggestion - "dest" or "target" for the long name

kjacque · 2023-01-12T00:41:42Z

src/control/cmd/daos_agent/support.go

+	Stop         bool   `short:"s" long:"Stop" description:"Stop the collectlog command on very first error"`
+	TargetFolder string `short:"t" long:"loglocation" description:"Folder location where log is going to be copied"`
+	Archive      bool   `short:"z" long:"archive" description:"Archive the log/config files"`
+	CustomLogs   string `short:"c" long:"custom-logs" description:"Collect the Logs from given directory"`


Not sure I fully understand this option. Is this an extra source dir that the logs will be copied from? "extra-logs-dir" may be a clearer name in that case

kjacque · 2023-01-12T00:50:02Z

src/control/cmd/daos_agent/support.go

+	}
+
+	// Default 3 steps of log/conf collection.
+	progress := support.ProgressBar{1, 3, 0, false}


A more readable model with Go is to use the struct member names inline.

Suggested change

progress := support.ProgressBar{1, 3, 0, false}

progress := support.ProgressBar{

Start: 1,

Total: 3,

}

No need to initialize the zeroes. All the members are automatically initialized to 0 unless otherwise specified.

An aside, I am not sure what the 1 means here. Shouldn't we always start at 0?

Done and remove the zeros.

kjacque · 2023-01-12T01:44:29Z

src/control/lib/support/log.go

+	Start      int  // start int number
+	Total      int  // end int number
+	Steps      int  // Int number be increased per steps
+	JsonOutput bool // Json option to skip progress bar if it's enabled


name suggestion - "NoDisplay"

kjacque · 2023-01-12T01:44:58Z

src/control/lib/support/log.go

+	JsonOutput bool // Json option to skip progress bar if it's enabled
+}
+
+type Params struct {


name suggestion - CollectLogsParams

Considering we may someday add other support commands.

kjacque · 2023-01-12T01:46:02Z

src/control/lib/support/log.go

+	LogCmd       string
+}
+
+type copy struct {


copy of what? I'd suggest a different name.

kjacque · 2023-01-12T01:48:33Z

src/control/lib/support/log.go

+}
+
+// Print the progress bar during log collect command
+func PrintProgress(progBar *ProgressBar) {


suggestion - could make these member functions of ProgressBar.A function signature like:

func (p *ProgressBar) Display(out io.Writer)

It would be easy enough to decide within the function whether to print progress or end status.

kjacque · 2023-01-12T01:54:46Z

src/control/lib/support/log.go

+func CollectSupportLog(log logging.Logger, opts ...Params) error {
+	switch opts[0].LogFunction {
+	case "CopyServerConfig":
+		return CopyServerConfig(log, opts...)


Do all of these functions need to be exported? Starting with a capital letter in Go means the symbol is exported. If they are solely called from this function I'd suggest unexporting them.

ravalsam · 2023-01-12T05:13:44Z

Thank you and appreciated for the review comments. I will update the PR.

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Required-githooks: true

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1

Style warning(s) for job https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-11094/5/
Please review https://wiki.hpdd.intel.com/display/DC/Coding+Rules

daosbuild1 · 2023-01-20T04:58:24Z

src/tests/ftest/control/dmg_support_collectlog.py

+        :avocado: tags=hw,medium
+        :avocado: tags=basic,control,dmg
+        :avocado: tags=test_dmg_support_collect_log
+        """        


(style) trailing whitespace

src/tests/ftest/control/dmg_support_collectlog.py

daosbuild1 · 2023-01-20T04:58:27Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11094/5/execution/node/145/log

Updated patch

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-05-18T09:25:19Z

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11094/43/execution/node/1034/log

Test-tag: pr control Required-githooks: true

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-05-31T04:17:07Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11094/45/execution/node/1236/log

mjmac

While this is a huge PR, it seems low risk in that the changes are adding new functionality rather than changing existing code. I'm in support of getting it landed so that we can continue to iterate on it.

kjacque · 2023-06-05T23:07:49Z

src/control/lib/support/log.go

+}
+
+// Get the system hostname
+func GetHostName() (string, error) {


not blocking on this, but there is a Go function os.Hostname() that can fetch the hostname without calling out to the shell.

Thanks for review. I will test and updated when will work on second iteration.

If I recall, I was trying to use that os.Hostname() but it was giving the full name including the domain name. So another option is to use the function and remove the domain from it.

Test-tag: pr control Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-06-08T21:49:59Z

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-11094/47/execution/node/1114/log

mjmac · 2023-06-13T16:10:18Z

@tanabarr: Has @ravalsam addressed your concerns? If so, please approve so we can get this landed, thanks.

tanabarr

I spent a while trying to work out whether the changes requested were addressed but tbh it's quite hard to tell, some of them definitely have been so I will approve.

Although this port copies the code from master to 2.4, there are issues related to permissions making it non-functional. This is a partial cherry-pick of the following PR's on master: DAOS-10625 control: Create the tool to collect the logs/config for support purpose (#11094) DAOS-13759 control: Update support collect-log tool. (#12906) DAOS-13763 control: Fix daos_metrics collection for support collect-log. (#12555) DAOS-13936 support: Collect the specific logs and Time range log for support (#13325) Change-Id: I168c14e177a5003c4e315595b1bf154e84cef473

Although this port copies the code from master to 2.4, there are issues related to permissions making it non-functional. This is a partial cherry-pick of the following PR's on master: DAOS-10625 control: Create the tool to collect the logs/config for support purpose (#11094) DAOS-13759 control: Update support collect-log tool. (#12906) DAOS-13763 control: Fix daos_metrics collection for support collect-log. (#12555) DAOS-13936 support: Collect the specific logs and Time range log for support (#13325) Signed-off-by: Chris Davis <[email protected]>

ravalsam requested a review from a team as a code owner December 20, 2022 21:49

ravalsam requested review from mjmac and removed request for a team December 20, 2022 21:49

daosbuild1 reviewed Dec 20, 2022

View reviewed changes

ravalsam force-pushed the samirrav/Support/DAOS-10625-Final branch from 727f0fd to 8353d18 Compare December 21, 2022 16:59

daosbuild1 reviewed Dec 21, 2022

View reviewed changes

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

59ba3a0

daosbuild1 reviewed Dec 21, 2022

View reviewed changes

ravalsam requested review from tanabarr and kjacque January 3, 2023 16:59

Samir Raval added 2 commits January 10, 2023 17:07

Adding UNit test code.. (Still WORK IN PROGRESS)

a1a57f9

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

e2360fd

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

tanabarr requested changes Jan 11, 2023

View reviewed changes

Adding option to collect daoe Agent log and config file.

f83ba36

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

kjacque requested changes Jan 12, 2023

View reviewed changes

Samir Raval added 6 commits January 12, 2023 23:59

Added Avocado test for support collectlog command.

51b59e8

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Updating code based on review comments.

dc1c5f7

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

2> Updaing code based on Review comments.

b0b2b77

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

8a39560

Required-githooks: true

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

d24f1a9

3> Updating code based on review comments.

6b4964b

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam requested a review from a team as a code owner January 20, 2023 04:56

daosbuild1 previously requested changes Jan 20, 2023

View reviewed changes

daosbuild1 reviewed May 17, 2023

View reviewed changes

phender previously approved these changes May 18, 2023

View reviewed changes

ravalsam requested a review from mjmac May 19, 2023 15:41

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

f4f0dc8

Test-tag: pr control Required-githooks: true

ravalsam dismissed phender’s stale review via f4f0dc8 May 30, 2023 16:02

daosbuild1 reviewed May 30, 2023

View reviewed changes

ravalsam requested a review from phender May 31, 2023 19:31

mjmac previously approved these changes Jun 5, 2023

View reviewed changes

kjacque previously approved these changes Jun 5, 2023

View reviewed changes

ravalsam requested a review from a team June 5, 2023 23:48

Merge branch 'master' into samirrav/Support/DAOS-10625-Final

d09903b

Test-tag: pr control Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam dismissed stale reviews from kjacque and mjmac via d09903b June 8, 2023 15:33

daosbuild1 reviewed Jun 8, 2023

View reviewed changes

ravalsam requested review from mjmac and kjacque June 8, 2023 15:36

kjacque approved these changes Jun 8, 2023

View reviewed changes

mjmac approved these changes Jun 13, 2023

View reviewed changes

ravalsam requested a review from a team June 13, 2023 16:08

tanabarr approved these changes Jun 13, 2023

View reviewed changes

mjmac merged commit 21693c0 into master Jun 14, 2023

mjmac deleted the samirrav/Support/DAOS-10625-Final branch June 14, 2023 19:53

DAOS-10625 control: Create the tool to collect the logs/config for support purpose #11094

DAOS-10625 control: Create the tool to collect the logs/config for support purpose #11094

Conversation

ravalsam commented Dec 20, 2022 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Dec 20, 2022 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Dec 21, 2022

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Dec 21, 2022

tanabarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjacque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravalsam commented Jan 12, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 20, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 18, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 31, 2023

mjmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 8, 2023

mjmac commented Jun 13, 2023

tanabarr left a comment

Choose a reason for hiding this comment

ravalsam commented Dec 20, 2022 •

edited

Loading

github-actions bot commented Dec 20, 2022 •

edited

Loading