PMM-4879 Adding support for defaults-file in new mysql service. #1068

pkadej · 2022-04-12T21:30:18Z

pmm

it-percona-cla · 2022-04-12T21:30:22Z

All committers have signed the CLA.

BupycHuk · 2022-04-20T10:36:04Z

services/agents/parse_defaults_file.go

+)
+
+// ParseDefaultsFile requests from agent to parse defaultsFile.
+type ParseDefaultsFile struct {


maybe DefaultsFileParser will be better naming for this struct?

Sure, fixed.

BupycHuk · 2022-04-20T10:43:31Z

services/agents/parse_defaults_file.go

+	if !ok {
+		return nil, errors.New("wrong response from agent (not ParseDefaultsFileResponse model)")
+	}
+	if len(parserResponse.GetError()) != 0 {


let's use field instead of method, we had to use methods in some places to not duplicate code

in a cases below, please use fields too

BupycHuk · 2022-04-20T10:45:00Z

services/management/mysql.go

@@ -78,6 +83,30 @@ func (s *MySQLService) Add(ctx context.Context, req *managementpb.AddMySQLReques
 		if err != nil {
 			return err
 		}
+
+		if len(req.DefaultsFile) != 0 {


Suggested change

if len(req.DefaultsFile) != 0 {

if req.DefaultsFile != "" {

the same for cases below

BupycHuk · 2022-04-20T19:16:48Z

services/management/mysql.go

+			if len(result.Username) != 0 {
+				req.Username = result.Username
+			}
+			if len(result.Password) != 0 {
+				req.Password = result.Password
+			}
+
+			if len(result.Host) != 0 {
+				req.Address = result.Host
+			}
+
+			if result.Port > 0 {
+				req.Port = result.Port
+			}


based on discussion in Jira ticket we should set values from defaults file only if they weren't passed as a separate field in request.

BupycHuk · 2022-04-20T19:17:15Z

services/management/mysql.go

@@ -39,15 +42,17 @@ type MySQLService struct {
 	state agentsStateUpdater
 	cc    connectionChecker
 	vc    versionCache
+	pfd   defaultsFileParser


Suggested change

pfd defaultsFileParser

dfp defaultsFileParser

Refactored.

BupycHuk · 2022-04-23T20:21:29Z

@pkadej could you please commit changes in go modules, so we can see result of CI.

pkadej · 2022-04-24T15:40:09Z

@pkadej could you please commit changes in go modules, so we can see result of CI.
@BupycHuk Did you mean temporary updating mod.go with pmm replaced with my fork or with version after percona/pmm#863 being merged?

BupycHuk · 2022-04-25T08:47:30Z

@pkadej yes, updated go.mod with replace directive for now and once PR in pmm is merged update go.mod to use that version.

pkadej · 2022-04-25T19:18:43Z

@pkadej yes, updated go.mod with replace directive for now and once PR in pmm is merged update go.mod to use that version.

@BupycHuk
Done. Unfortunately i don't have permission to trigger runflow.

codecov · 2022-04-26T19:17:12Z

Codecov Report

Merging #1068 (5267092) into main (05124a1) will increase coverage by 0.30%.
The diff coverage is 42.62%.

@@            Coverage Diff             @@
##             main    #1068      +/-   ##
==========================================
+ Coverage   48.67%   48.98%   +0.30%     
==========================================
  Files         181      182       +1     
  Lines       21264    21323      +59     
==========================================
+ Hits        10351    10445      +94     
+ Misses       9775     9723      -52     
- Partials     1138     1155      +17

Flag	Coverage Δ
all	`48.64% <42.62%> (+0.30%)`	⬆️
cover	`49.14% <42.62%> (+0.33%)`	⬆️
crosscover	`48.98% <42.62%> (+0.30%)`	⬆️
update	`12.84% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
main.go	`0.00% <0.00%> (ø)`
services/agents/parse_defaults_file.go	`20.00% <20.00%> (ø)`
services/agents/channel/channel.go	`62.28% <100.00%> (+0.43%)`	⬆️
services/management/mysql.go	`54.03% <100.00%> (+54.03%)`	⬆️
services/management/management.go	`28.26% <0.00%> (+18.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05124a1...5267092. Read the comment docs.

BupycHuk · 2022-04-28T21:40:49Z

services/agents/channel/channel_test.go

@@ -356,3 +356,50 @@ func TestUnexpectedResponsePayloadFromAgent(t *testing.T) {
 	close(stopServer)
 	<-stop
 }
+
+func TestChannelForDefaultsFileParser(t *testing.T) {


I'm not sure what are we testing here

ritbl · 2022-05-02T09:33:39Z

services/agents/parse_defaults_file.go

+		}
+		return request, nil
+	} else {
+		return nil, errors.Errorf("unhandled service type %s", serviceType)


Suggested change

return nil, errors.Errorf("unhandled service type %s", serviceType)

return nil, errors.Errorf("unsupported service type %s", serviceType)

ritbl · 2022-05-02T09:36:51Z

services/agents/parse_defaults_file.go

+	start := time.Now()
+	defer func() {
+		if dur := time.Since(start); dur > 5*time.Second {
+			l.Warnf("ParseDefaultsFile took %s.", dur)
+		}
+	}()


Is it realistic? Reading a file with few lines should not be a problem. And, I think, it should be handed in agent

That can be written like this:

defer func(start time.Time) { if dur := time.Since(start); dur > 5*time.Second { l.Warnf("ParseDefaultsFile took %s.", dur) } }(time.Now())

But yes, I also don't see benefit of this log

ritbl · 2022-05-02T09:37:56Z

services/agents/parse_defaults_file.go

+	l.Infof("ParseDefaultsFile response from agent: %+v.", resp)
+	parserResponse, ok := resp.(*agentpb.ParseDefaultsFileResponse)
+	if !ok {
+		return nil, errors.New("wrong response from agent (not ParseDefaultsFileResponse model)")


Can we add info regarding actual type?

ritbl · 2022-05-02T09:42:25Z

services/management/mysql.go

+				return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))
+			}
+
+			// set username and password from parsed defaults file by agent


nit: it would be good to know that value has been overridden, can we log.debug that?

artemgavrilov

LGTM with comments

artemgavrilov · 2022-05-05T11:30:38Z

services/agents/parse_defaults_file.go

+	start := time.Now()
+	defer func() {
+		if dur := time.Since(start); dur > 5*time.Second {
+			l.Warnf("ParseDefaultsFile took %s.", dur)
+		}
+	}()


That can be written like this:

defer func(start time.Time) { if dur := time.Since(start); dur > 5*time.Second { l.Warnf("ParseDefaultsFile took %s.", dur) } }(time.Now())

But yes, I also don't see benefit of this log

artemgavrilov · 2022-05-05T14:10:09Z

services/agents/parse_defaults_file.go

+		return nil, err
+	}
+
+	l.Infof("ParseDefaultsFile response from agent: %+v.", resp)


I think Debug level fits better here.

artemgavrilov · 2022-05-05T14:30:19Z

services/management/mysql.go

+		if req.DefaultsFile != "" {
+			result, err := s.dfp.ParseDefaultsFile(ctx, req.PmmAgentId, req.DefaultsFile, models.MySQLServiceType)
+			if err != nil {
+				return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))


Suggested change

return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))

return status.Errorf(codes.FailedPrecondition, "Defaults file error: %s.", err)

artemgavrilov · 2022-05-05T14:36:33Z

services/management/mysql_test.go

+	versionCache := &mockVersionCache{}
+	versionCache.Test(t)
+
+	teardown := func(t *testing.T) {


You can use t.Cleanup instead.

PMM-4879 Adding support for defaults-file in new mysql service.

860c132

pkadej and others added 3 commits April 15, 2022 02:50

PMM-4879 defaults-file adjustments.

2f7f34d

Merge branch 'main' into PMM-4879-defaults-file

2168c85

PMM-4879 Linter & tests fix.

f6c7690

pkadej marked this pull request as ready for review April 20, 2022 09:44

pkadej requested review from BupycHuk and artemgavrilov as code owners April 20, 2022 09:44

BupycHuk suggested changes Apr 20, 2022

View reviewed changes

pkadej added 2 commits April 21, 2022 06:42

PMM-4879 Adding support for configuration entry: socket.

ea8e53e

PMM-4879 Code review adjustments.

67e20e5

pkadej requested a review from BupycHuk April 23, 2022 13:03

Merge branch 'main' into PMM-4879-defaults-file

6964cfd

pkadej and others added 2 commits April 25, 2022 20:23

PMM-4879 Code review adjustments: replacing API dependencies.

5f78aa2

Merge branch 'main' into PMM-4879-defaults-file

53bb457

pkadej and others added 3 commits April 26, 2022 20:20

PMM-4879 Code review adjustments: replacing API dependencies.

868c96b

PMM-4879 Code review adjustments: fixing code for CI.

1d179ca

Merge branch 'main' into PMM-4879-defaults-file

031c091

pkadej added 2 commits April 27, 2022 19:26

Merge branch 'main' into PMM-4879-defaults-file

5cb6346

PMM-4879 Added coverage tests.

64538ce

pkadej force-pushed the PMM-4879-defaults-file branch from b4c8c0a to 64538ce Compare April 27, 2022 17:56

pkadej added 3 commits April 27, 2022 23:04

PMM-4879 Added coverage tests.

2b875c8

PMM-4879 Code review adjustments.

9889453

Merge branch 'main' into PMM-4879-defaults-file

bdd11c2

BupycHuk approved these changes Apr 28, 2022

View reviewed changes

ritbl approved these changes May 2, 2022

View reviewed changes

Merge branch 'main' into PMM-4879-defaults-file

469b7bc

pkadej mentioned this pull request May 3, 2022

PMM-4879 Adding support for defaults-file in new mysql service. #1103

Closed

PMM-4879 Updating api-tests.

f0d0326

artemgavrilov approved these changes May 5, 2022

View reviewed changes

artemgavrilov assigned pkadej May 5, 2022

pkadej mentioned this pull request Jun 9, 2022

PMM-4879 mysql add defaults file percona/pmm#928

Merged

Merge branch 'main' into PMM-4879-defaults-file

5267092

pkadej merged commit 1011fab into percona:main Jun 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMM-4879 Adding support for defaults-file in new mysql service. #1068

PMM-4879 Adding support for defaults-file in new mysql service. #1068

pkadej commented Apr 12, 2022 •

edited

Loading

it-percona-cla commented Apr 12, 2022 •

edited

Loading

BupycHuk Apr 20, 2022

pkadej Apr 22, 2022

BupycHuk Apr 20, 2022

BupycHuk Apr 20, 2022

pkadej Apr 22, 2022

BupycHuk Apr 20, 2022

BupycHuk Apr 20, 2022

pkadej Apr 22, 2022

BupycHuk Apr 20, 2022

pkadej Apr 22, 2022

BupycHuk Apr 20, 2022

pkadej Apr 22, 2022

BupycHuk commented Apr 23, 2022

pkadej commented Apr 24, 2022 •

edited

Loading

BupycHuk commented Apr 25, 2022 •

edited

Loading

pkadej commented Apr 25, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

BupycHuk Apr 28, 2022

ritbl May 2, 2022

ritbl May 2, 2022

artemgavrilov May 5, 2022

ritbl May 2, 2022

ritbl May 2, 2022

artemgavrilov left a comment

artemgavrilov May 5, 2022

artemgavrilov May 5, 2022

artemgavrilov May 5, 2022

artemgavrilov May 5, 2022

	return nil, errors.Errorf("unhandled service type %s", serviceType)
	return nil, errors.Errorf("unsupported service type %s", serviceType)

	return status.Error(codes.FailedPrecondition, fmt.Sprintf("Defaults file error: %s.", err))
	return status.Errorf(codes.FailedPrecondition, "Defaults file error: %s.", err)

PMM-4879 Adding support for defaults-file in new mysql service. #1068

PMM-4879 Adding support for defaults-file in new mysql service. #1068

Conversation

pkadej commented Apr 12, 2022 • edited Loading

it-percona-cla commented Apr 12, 2022 • edited Loading

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

BupycHuk commented Apr 23, 2022

pkadej commented Apr 24, 2022 • edited Loading

BupycHuk commented Apr 25, 2022 • edited Loading

pkadej commented Apr 25, 2022 • edited Loading

codecov bot commented Apr 26, 2022 • edited Loading

Codecov Report

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

artemgavrilov 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

pkadej commented Apr 12, 2022 •

edited

Loading

it-percona-cla commented Apr 12, 2022 •

edited

Loading

pkadej commented Apr 24, 2022 •

edited

Loading

BupycHuk commented Apr 25, 2022 •

edited

Loading

pkadej commented Apr 25, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading