Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
10 changes: 10 additions & 0 deletions pkg/app/api/grpcapi/piped_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -933,6 +933,16 @@ func (a *PipedAPI) GetDesiredVersion(ctx context.Context, _ *pipedservice.GetDes
}, nil
}

func (a *PipedAPI) UpdateApplicationConfigurations(ctx context.Context, req *pipedservice.UpdateApplicationConfigurationsRequest) (*pipedservice.UpdateApplicationConfigurationsResponse, error) {
Comment thread
nakabonne marked this conversation as resolved.
Comment thread
nakabonne marked this conversation as resolved.
// TODO: Update the given application configurations
return nil, status.Errorf(codes.Unimplemented, "UpdateApplicationConfigurations is not implemented yet")
}

func (a *PipedAPI) ReportUnregisteredApplicationConfigurations(ctx context.Context, req *pipedservice.ReportUnregisteredApplicationConfigurationsRequest) (*pipedservice.ReportUnregisteredApplicationConfigurationsResponse, error) {
Comment thread
nakabonne marked this conversation as resolved.
Comment thread
nakabonne marked this conversation as resolved.
// TODO: Make the unused application configurations cache up-to-date
return nil, status.Errorf(codes.Unimplemented, "ReportUnregisteredApplicationConfigurations is not implemented yet")
}

// validateAppBelongsToPiped checks if the given application belongs to the given piped.
// It gives back an error unless the application belongs to the piped.
func (a *PipedAPI) validateAppBelongsToPiped(ctx context.Context, appID, pipedID string) error {
Expand Down
23 changes: 22 additions & 1 deletion pkg/app/api/service/pipedservice/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,10 @@ service PipedService {
// GetDesiredVersion returns the desired version of the given Piped.
rpc GetDesiredVersion(GetDesiredVersionRequest) returns (GetDesiredVersionResponse) {}

// TODO: Add an rpc to update application info based on one defined in the application config
// UpdateApplicationConfigurations updates application configurations.
rpc UpdateApplicationConfigurations(UpdateApplicationConfigurationsRequest) returns (UpdateApplicationConfigurationsResponse) {}
// ReportLatestUnusedApplicationConfigurations puts the latest configurations of applications that isn't registered yet.
rpc ReportUnregisteredApplicationConfigurations(ReportUnregisteredApplicationConfigurationsRequest) returns (ReportUnregisteredApplicationConfigurationsResponse) {}
}

enum ListOrder {
Expand Down Expand Up @@ -445,3 +448,21 @@ message GetDesiredVersionRequest {
message GetDesiredVersionResponse {
string version = 1;
}


message UpdateApplicationConfigurationsRequest {
// The application configurations that should be updated.
repeated pipe.model.ApplicationInfo applications = 1;
}

message UpdateApplicationConfigurationsResponse {
}

message ReportUnregisteredApplicationConfigurationsRequest {
// All the latest application configurations that isn't registered yet.
// Note that ALL configs are always be contained every time.
repeated pipe.model.ApplicationInfo applications = 1;
}

message ReportUnregisteredApplicationConfigurationsResponse {
}
18 changes: 18 additions & 0 deletions pkg/app/piped/appconfigreporter/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "go_default_library",
srcs = ["appconfigreporter.go"],
importpath = "github.com/pipe-cd/pipe/pkg/app/piped/appconfigreporter",
visibility = ["//visibility:public"],
deps = [
"//pkg/app/api/service/pipedservice:go_default_library",
"//pkg/cache:go_default_library",
"//pkg/cache/memorycache:go_default_library",
"//pkg/config:go_default_library",
"//pkg/git:go_default_library",
"//pkg/model:go_default_library",
"@org_golang_google_grpc//:go_default_library",
"@org_uber_go_zap//:go_default_library",
],
)
279 changes: 279 additions & 0 deletions pkg/app/piped/appconfigreporter/appconfigreporter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,279 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package appconfigreporter

import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"time"

"go.uber.org/zap"
"google.golang.org/grpc"

"github.com/pipe-cd/pipe/pkg/app/api/service/pipedservice"
"github.com/pipe-cd/pipe/pkg/config"
"github.com/pipe-cd/pipe/pkg/git"
"github.com/pipe-cd/pipe/pkg/model"
)

type apiClient interface {
UpdateApplicationConfigurations(ctx context.Context, in *pipedservice.UpdateApplicationConfigurationsRequest, opts ...grpc.CallOption) (*pipedservice.UpdateApplicationConfigurationsResponse, error)
ReportUnregisteredApplicationConfigurations(ctx context.Context, in *pipedservice.ReportUnregisteredApplicationConfigurationsRequest, opts ...grpc.CallOption) (*pipedservice.ReportUnregisteredApplicationConfigurationsResponse, error)
}

type gitClient interface {
Clone(ctx context.Context, repoID, remote, branch, destination string) (git.Repo, error)
}

type applicationLister interface {
List() []*model.Application
}

type Reporter struct {
apiClient apiClient
gitClient gitClient
applicationLister applicationLister
config *config.PipedSpec
gitRepos map[string]git.Repo
gracePeriod time.Duration
// Cache for the last scanned commit for each repository.
// Not goroutine safe.
lastScannedCommits map[string]string
logger *zap.Logger
}

func NewReporter(
apiClient apiClient,
gitClient gitClient,
appLister applicationLister,
cfg *config.PipedSpec,
gracePeriod time.Duration,
logger *zap.Logger,
) *Reporter {
return &Reporter{
apiClient: apiClient,
gitClient: gitClient,
applicationLister: appLister,
config: cfg,
gracePeriod: gracePeriod,
lastScannedCommits: make(map[string]string),
logger: logger.Named("app-config-reporter"),
}
}

func (r *Reporter) Run(ctx context.Context) error {
r.logger.Info("start running app-config-reporter")

// Pre-clone to cache the registered git repositories.
r.gitRepos = make(map[string]git.Repo, len(r.config.Repositories))
for _, repoCfg := range r.config.Repositories {
repo, err := r.gitClient.Clone(ctx, repoCfg.RepoID, repoCfg.Remote, repoCfg.Branch, "")
if err != nil {
r.logger.Error("failed to clone repository",
zap.String("repo-id", repoCfg.RepoID),
zap.Error(err),
)
return err
}
r.gitRepos[repoCfg.RepoID] = repo
}

// FIXME: Think about sync interval of app config reporter
ticker := time.NewTicker(r.config.SyncInterval.Duration())
defer ticker.Stop()

for {
select {
case <-ticker.C:
if err := r.scanAppConfigs(ctx); err != nil {
r.logger.Error("failed to check application configurations defined in Git", zap.Error(err))
}
case <-ctx.Done():
r.logger.Info("app-config-reporter has been stopped")
return nil
}
}
}

// scanAppConfigs checks and reports two types of applications.
// One is applications registered in Control-plane already, and another is ones that aren't registered yet.
func (r *Reporter) scanAppConfigs(ctx context.Context) error {
if len(r.gitRepos) == 0 {
r.logger.Info("no repositories were configured for this piped")
return nil
}

// Make all repos up-to-date.
for repoID, repo := range r.gitRepos {
if err := repo.Pull(ctx, repo.GetClonedBranch()); err != nil {
r.logger.Error("failed to update repo to latest",
zap.String("repo-id", repoID),
zap.Error(err),
)
return err
}
}

// Create a map to determine from GitPath if the application is registered.
registeredAppPaths := make(map[string]struct{})
for _, app := range r.applicationLister.List() {
id := model.BuildGitPathID(app.GitPath.Repo.Id, app.GitPath.Path, app.GitPath.ConfigFilename)
registeredAppPaths[id] = struct{}{}
}

if err := r.updateUnregisteredApps(ctx, registeredAppPaths); err != nil {
return fmt.Errorf("failed to update unregistered applications: %w", err)
}
if err := r.updateRegisteredApps(ctx, registeredAppPaths); err != nil {
return fmt.Errorf("failed to update registered applications: %w", err)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Registered applications are more important than unregistered ones so let's change the order of updating.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For sure 👍


return nil
}

// updateUnregisteredApps sends all unregistered application configurations to the control-plane.
func (r *Reporter) updateUnregisteredApps(ctx context.Context, registeredAppPaths map[string]struct{}) error {
apps := make([]*model.ApplicationInfo, 0)
for repoID, repo := range r.gitRepos {
err := filepath.Walk(repo.GetPath(), func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
appInfo, skip := r.readApplicationInfo(repoID, repo.GetPath(), filepath.Base(path), registeredAppPaths, false)
if skip {
return nil
}
apps = append(apps, appInfo)
return nil
})
if err != nil {
return fmt.Errorf("failed to inspect files under %s: %w", repo.GetPath(), err)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Add a log message that could help for debugging by our users.

found out %d unregistered applications in repository %s

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice. It is definite that currently it's quite hard to see what's going on.

}
if len(apps) == 0 {
return nil

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think that we should send this empty list to the control plane to delete the cache?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or we can have a simple mark in this Reporter to avoid sending continuous this kind of request.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To avoid sending meaningless requests simply, I applied this way. Cache in Control-plane gets periodically removed, so I thought it's fine. But it's worth considering.

}

_, err := r.apiClient.ReportUnregisteredApplicationConfigurations(
ctx,
&pipedservice.ReportUnregisteredApplicationConfigurationsRequest{
Applications: apps,
},
)
if err != nil {
return fmt.Errorf("failed to put the latest unregistered application configurations: %w", err)
}
return nil
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Separating the finding part and the reporting part to have some tests for finding logic.


// updateRegisteredApps sends application configurations that have changed since the last time to the control-plane.
func (r *Reporter) updateRegisteredApps(ctx context.Context, registeredAppPaths map[string]struct{}) (err error) {
apps := make([]*model.ApplicationInfo, 0)
for repoID, repo := range r.gitRepos {
var headCommit git.Commit
headCommit, err = repo.GetLatestCommit(ctx)
if err != nil {
return fmt.Errorf("failed to get the latest commit of %s: %w", repoID, err)
}
lastScannedCommit, ok := r.lastScannedCommits[repoID]
if ok && headCommit.Hash == lastScannedCommit {
continue
}

var files []string
files, err = repo.ChangedFiles(ctx, lastScannedCommit, headCommit.Hash)
if err != nil {
return fmt.Errorf("failed to get files those were touched between two commits: %w", err)
}
if len(files) == 0 {
// The case where all changes have been fully reverted.
continue
}
for _, filename := range files {
appInfo, skip := r.readApplicationInfo(repoID, repo.GetPath(), filename, registeredAppPaths, true)
if skip {
continue
}
apps = append(apps, appInfo)
}

id := repoID
defer func() {
if err == nil {
r.lastScannedCommits[id] = headCommit.Hash
}
}()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think to be simple we should avoid defer in the loop.
So instead of that, we can mark the commit list in a variable and save them in a single defer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Cool. It will reduce the amount of code.

}
if len(apps) == 0 {
return
}

_, err = r.apiClient.UpdateApplicationConfigurations(
ctx,
&pipedservice.UpdateApplicationConfigurationsRequest{
Applications: apps,
},
)
if err != nil {
return fmt.Errorf("failed to update application configurations: %w", err)
}
return
}

func (r *Reporter) readApplicationInfo(repoID, path, cfgFilename string, registeredAppPaths map[string]struct{}, wantRegistered bool) (appInfo *model.ApplicationInfo, skip bool) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be simple, registeredAppPaths map[string]struct{}, wantRegistered bool should not be a part of this function.

if !strings.HasSuffix(cfgFilename, model.DefaultDeploymentConfigFileExtension) {
return nil, true
}
gitPathID := model.BuildGitPathID(repoID, path, cfgFilename)
if _, registered := registeredAppPaths[gitPathID]; registered != wantRegistered {
return nil, true
}

cfgFilePath := filepath.Join(path, cfgFilename)
cfg, err := config.LoadFromYAML(cfgFilePath)
if err != nil {
r.logger.Error("failed to load configuration file",
zap.String("repo-id", repoID),
zap.String("config-file-path", cfgFilePath),
zap.Error(err),
)
return nil, true
}

spec, ok := cfg.GetGenericDeployment()
if !ok {
r.logger.Error(fmt.Sprintf("unsupported application kind: %s", cfg.Kind),
zap.String("repo-id", repoID),
zap.String("config-file-path", cfgFilePath),
)
return nil, true
}

return &model.ApplicationInfo{
Name: spec.Name,
// TODO: Convert Kind string into dedicated type
//Kind: cfg.Kind,
EnvId: spec.EnvID,
Path: path,
ConfigFilename: cfgFilename,
Labels: spec.Labels,
}, false
}
1 change: 1 addition & 0 deletions pkg/app/piped/cmd/piped/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ go_library(
"//pkg/app/piped/apistore/deploymentstore:go_default_library",
"//pkg/app/piped/apistore/environmentstore:go_default_library",
"//pkg/app/piped/apistore/eventstore:go_default_library",
"//pkg/app/piped/appconfigreporter:go_default_library",
"//pkg/app/piped/chartrepo:go_default_library",
"//pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics:go_default_library",
"//pkg/app/piped/controller:go_default_library",
Expand Down
17 changes: 17 additions & 0 deletions pkg/app/piped/cmd/piped/piped.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/pipe-cd/pipe/pkg/app/piped/apistore/deploymentstore"
"github.com/pipe-cd/pipe/pkg/app/piped/apistore/environmentstore"
"github.com/pipe-cd/pipe/pkg/app/piped/apistore/eventstore"
"github.com/pipe-cd/pipe/pkg/app/piped/appconfigreporter"
"github.com/pipe-cd/pipe/pkg/app/piped/chartrepo"
k8scloudprovidermetrics "github.com/pipe-cd/pipe/pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics"
"github.com/pipe-cd/pipe/pkg/app/piped/controller"
Expand Down Expand Up @@ -439,6 +440,22 @@ func (p *piped) run(ctx context.Context, input cli.Input) (runErr error) {
})
}

// Start running app-config-reporter.
{
r := appconfigreporter.NewReporter(
apiClient,
gitClient,
applicationLister,
cfg,
p.gracePeriod,
input.Logger,
)

group.Go(func() error {
return r.Run(ctx)
})
}

// Wait until all piped components have finished.
// A terminating signal or a finish of any components
// could trigger the finish of piped.
Expand Down
4 changes: 4 additions & 0 deletions pkg/config/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const (
)

type GenericDeploymentSpec struct {
Name string `json:"name"`
EnvID string `json:"env_id"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
EnvID string `json:"env_id"`
EnvID string `json:"envId"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Btw, isn't the name better than the id?

@nakabonne nakabonne Nov 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was worried about the uniqueness of env_name but let's leave it to the user to maintain the uniqueness.

It's obvious that the application name shouldn't be unique, but ideally the env name should be unique because Environment is a project-wide concept.
To do that with a document-oriented database, we need to prepare some kind of extra table with the name like envname to ensure uniqueness.

But we're planning to migrate Env to Labels in the future, again, for now let's leave it to the user to maintain the uniqueness.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we proceed with the Name as non-unique, we will have to add a not neat method to environmentstore. I'd like to hear your opinion on uniqueness before implementing it.

Labels map[string]string `json:"labels"`

// Configuration used while planning deployment.
Planner DeploymentPlanner `json:"planner"`
// Forcibly use QuickSync or Pipeline when commit message matched the specified pattern.
Expand Down
Loading