Skip to content
This repository was archived by the owner on Jun 21, 2022. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ type gRPCServerDeps struct {
actions *agents.ActionsService
agentsStateUpdater *agents.StateUpdater
connectionCheck *agents.ConnectionChecker
parseDefaultsFile *agents.ParseDefaultsFile
grafanaClient *grafana.Client
checksService *checks.Service
dbaasClient *dbaas.Client
Expand Down Expand Up @@ -188,7 +189,7 @@ func runGRPCServer(ctx context.Context, deps *gRPCServerDeps) {

nodeSvc := management.NewNodeService(deps.db)
serviceSvc := management.NewServiceService(deps.db, deps.agentsStateUpdater, deps.vmdb)
mysqlSvc := management.NewMySQLService(deps.db, deps.agentsStateUpdater, deps.connectionCheck, deps.versionCache)
mysqlSvc := management.NewMySQLService(deps.db, deps.agentsStateUpdater, deps.connectionCheck, deps.versionCache, deps.parseDefaultsFile)
mongodbSvc := management.NewMongoDBService(deps.db, deps.agentsStateUpdater, deps.connectionCheck)
postgresqlSvc := management.NewPostgreSQLService(deps.db, deps.agentsStateUpdater, deps.connectionCheck)
proxysqlSvc := management.NewProxySQLService(deps.db, deps.agentsStateUpdater, deps.connectionCheck)
Expand Down Expand Up @@ -726,6 +727,7 @@ func main() {
schedulerService := scheduler.New(db, backupService)
versionCache := versioncache.New(db, versioner)
emailer := alertmanager.NewEmailer(logrus.WithField("component", "alertmanager-emailer").Logger)
parseDefaultsFile := agents.NewParseDefaultsFile(agentsRegistry)

serverParams := &server.Params{
DB: db,
Expand Down Expand Up @@ -926,6 +928,7 @@ func main() {
versionCache: versionCache,
supervisord: supervisord,
config: &cfg.Config,
parseDefaultsFile: parseDefaultsFile,
})
}()

Expand Down
2 changes: 2 additions & 0 deletions services/agents/channel/channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,8 @@ func (c *Channel) runReceiver() {
c.publish(msg.Id, msg.Status, p.GetVersions)
case *agentpb.AgentMessage_PbmSwitchPitr:
c.publish(msg.Id, msg.Status, p.PbmSwitchPitr)
case *agentpb.AgentMessage_ParseDefaultsFile:
c.publish(msg.Id, msg.Status, p.ParseDefaultsFile)

case nil:
c.cancel(msg.Id, errors.Errorf("unimplemented: failed to handle received message %s", msg))
Expand Down
105 changes: 105 additions & 0 deletions services/agents/parse_defaults_file.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// pmm-managed
// Copyright (C) 2017 Percona LLC
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

package agents

import (
"context"
"time"

"github.com/percona/pmm/api/agentpb"
"github.com/percona/pmm/api/inventorypb"
"github.com/pkg/errors"

"github.com/percona/pmm-managed/models"
"github.com/percona/pmm-managed/utils/logger"
)

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe DefaultsFileParser will be better naming for this struct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, fixed.

r *Registry
}

// NewParseDefaultsFile creates new ParseDefaultsFile request.
func NewParseDefaultsFile(r *Registry) *ParseDefaultsFile {
return &ParseDefaultsFile{
r: r,
}
}

// ParseDefaultsFile sends request (with file path) to pmm-agent to parse defaults file.
func (p *ParseDefaultsFile) ParseDefaultsFile(ctx context.Context, pmmAgentID, filePath string, serviceType models.ServiceType) (*models.ParseDefaultsFileResult, error) {
l := logger.Get(ctx)

pmmAgent, err := p.r.get(pmmAgentID)
if err != nil {
return nil, err
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


request, err := createRequest(filePath, serviceType)
if err != nil {
l.Debugf("can't create ParseDefaultsFileRequest %s", err)
return nil, err
}

resp, err := pmmAgent.channel.SendAndWaitResponse(request)
if err != nil {
return nil, err
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Debug level fits better here.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add info regarding actual type?

}
if len(parserResponse.GetError()) != 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in a cases below, please use fields too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed.

return nil, errors.New(parserResponse.GetError())
}

return &models.ParseDefaultsFileResult{
Username: parserResponse.GetUsername(),
Password: parserResponse.GetPassword(),
Host: parserResponse.GetHost(),
Port: parserResponse.GetPort(),
}, nil
}

func createRequest(configPath string, serviceType models.ServiceType) (*agentpb.ParseDefaultsFileRequest, error) {
var request *agentpb.ParseDefaultsFileRequest

switch serviceType {
case models.MySQLServiceType:
request = &agentpb.ParseDefaultsFileRequest{
ServiceType: inventorypb.ServiceType_MYSQL_SERVICE,
ConfigPath: configPath,
}
case models.ExternalServiceType:
case models.HAProxyServiceType:
case models.MongoDBServiceType:
case models.PostgreSQLServiceType:
case models.ProxySQLServiceType:
default:
return nil, errors.Errorf("unhandled service type %s", serviceType)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

}
return request, nil
}
7 changes: 7 additions & 0 deletions services/management/deps.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
//go:generate mockery -name=grafanaClient -case=snake -inpkg -testonly
//go:generate mockery -name=jobsService -case=snake -inpkg -testonly
//go:generate mockery -name=connectionChecker -case=snake -inpkg -testonly
//go:generate mockery -name=defaultsFileParser -case=snake -inpkg -testonly

// agentsRegistry is a subset of methods of agents.Registry used by this package.
// We use it instead of real type for testing and to avoid dependency cycle.
Expand Down Expand Up @@ -93,3 +94,9 @@ type connectionChecker interface {
type versionCache interface {
RequestSoftwareVersionsUpdate()
}

// defaultsFileParser is a subset of methods of agents.ParseDefaultsFile.
// We use it instead of real type for testing and to avoid dependency cycle.
type defaultsFileParser interface {
ParseDefaultsFile(ctx context.Context, pmmAgentID, filePath string, serviceType models.ServiceType) (*models.ParseDefaultsFileResult, error)
}
42 changes: 42 additions & 0 deletions services/management/mock_defaults_file_parser_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 30 additions & 1 deletion services/management/mysql.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ package management

import (
"context"
"fmt"

"github.com/AlekSi/pointer"
"github.com/percona/pmm/api/inventorypb"
"github.com/percona/pmm/api/managementpb"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"gopkg.in/reform.v1"

"github.com/percona/pmm-managed/models"
Expand All @@ -39,15 +42,17 @@ type MySQLService struct {
state agentsStateUpdater
cc connectionChecker
vc versionCache
pfd defaultsFileParser

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
pfd defaultsFileParser
dfp defaultsFileParser

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactored.

}

// NewMySQLService creates new MySQL Management Service.
func NewMySQLService(db *reform.DB, state agentsStateUpdater, cc connectionChecker, vc versionCache) *MySQLService {
func NewMySQLService(db *reform.DB, state agentsStateUpdater, cc connectionChecker, vc versionCache, pfd defaultsFileParser) *MySQLService {
return &MySQLService{
db: db,
state: state,
cc: cc,
vc: vc,
pfd: pfd,
}
}

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

if len(req.DefaultsFile) != 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(req.DefaultsFile) != 0 {
if req.DefaultsFile != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the same for cases below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

}

// set username and password from parsed defaults file by agent

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed.

}

service, err := models.AddNewService(tx.Querier, models.MySQLServiceType, &models.AddDBMSServiceParams{
ServiceName: req.ServiceName,
NodeID: nodeID,
Expand Down