Enable to push outdated images #1258

nakabonne · 2020-12-16T01:33:14Z

What this PR does / why we need it:
Mainly this PR contains:

Enable to write and push the outdated images
Refactoring
Rename PipedImageWatcherRepoTarget to PipedImageWatcherFilter so Piped config will be like:

  imageWatcher:
    filters:
      - repoId: foo
        includes:
          - imagewatcher-dev.yaml

Also, currently the mutex is used for file locking. If we have to try to perform as fast as possible, I'd say it's preferable to have mutex for each repository. But premature optimization is the root of all evil. Let's use the same mutex for all file locking.

Which issue(s) this PR fixes:

Fixes #1210

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2020-12-16T01:39:13Z

Code coverage for golang is 33.81%. This pull request does not change code coverage.

khanhtc1202 · 2020-12-16T02:17:38Z

Rename PipedImageWatcherRepoTarget to PipedImageWatcherFilter

sorry for the dumb question, but I could not get the point of rename repos to filters. The name filters quite generic, and hard to understand what do we do with that field here.

khanhtc1202 · 2020-12-16T02:27:34Z

pkg/app/piped/imagewatcher/watcher.go

-		}
-	}
-	return updates, nil
+	return cfg, nil


nit: I suggest we could simplify the function signature of this config.LoadImageWatcher function to

func LoadImageWatcher(repoRoot string, includes, excludes []string) (*ImageWatcherSpec, error)

and create a new error type config.NotFound, then check it here with errors.Is.
The reasons is

current ok value here look quite weird, should it be found? such that it's clarifying shown what do we use it for.

I read the implementation of config.LoadImageWatcher once, looks like after check for os.IsNotExist we have to pass false value for it on all other cases.

What do you think of it? 😄

Thank you! No objection to that! Other part should use that error type, so let me address it as another patch: #1259

thanks 👍

nakabonne · 2020-12-16T02:45:36Z

@khanhtc1202

but I could not get the point of rename repos to filters.

Thanks for giving me the opportunity to rethink it. This config field mainly aims to filter the image watcher files. With the name repos, I feel like users can be confused, that's why I decided to rename it.

nakabonne · 2020-12-16T02:47:59Z

IMO, config field name should tell users what it actually does by setting its name, but what do you think?

nghialv · 2020-12-16T04:26:40Z

pkg/app/piped/imagewatcher/watcher.go

 }

-// run periodically compares the image stored in the given provider and one stored in git.
+// run periodically compares the image in the given provider and one in git repository.


Not in this PR's scope, but I feel that we should start thinking about simplifying the concurrency model implemented in this ImageWatcher package.

Current approach:

Spawn goroutines for each image provider

Inside each goroutine

loop the repositories

lock and pull to update the local git.repo data

load the configuration at .pipe to find the target providers

if the provider was specified, retrieve the latest images

lock and update them

Suggestion:

Spawn goroutines for each repository

Inside each goroutine

pull to update the local repo data

load the configuration at .pipe to find the target providers

loop the target providers

retrieve the latest images and update them

Pros:

don't need the look for git.Pull

avoid complex concurrent processing for git.Repo: pull and push

reduce the number of pulls and configuration loads (current: provider_num x repo_num, new: repo_num)

Cons:

The pull interval for all image providers must be same (I suggest to move that field into imageWatcher part, imageProvider part is only for defining how to interact with the image provider)

You're quite right. Current complex implementation is cause by the spec that requires to specify polling interval for each provider. I couldn't completely make up my mind for a long time. The way to be the same interval across all provider would be pretty simple, and love it. But isn't it really needed to configure the interval for each provider? I thought we should be more cautious about communication to the container registry than to git repository, that's why I designed like this.

But as you said, using the same interval is pretty good if it's not worth supporting.

For instance, a user has two image providers:

Docker Hub that has the strict pull rate limit

ECR that has not strict pull rate limit

For that, it's useful to respectively set pull intervals.

However, we don't have to handle such carefully if we define that case as an edge case.

Yes, I think in that case they can split into multiple pipeds.

Ah, agree with you. The general case had better be prioritized. Okay, let me work on them in another PR. I'll fix your other change requests.

nghialv · 2020-12-16T04:33:51Z

pkg/app/piped/imagewatcher/watcher.go

+		return fmt.Errorf("failed to replace value at %s with %s: %w", target.Field, imageInRegistry, err)
+	}
+	w.mu.Lock()
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, os.ModeAppend)


Writing directly into the git.Repo may cause unexpected behaviors.

other goroutines are also accessing this local directory too

adding a new commit may cause a pull error in the future

So I think we have to clone it into a temporary directory for pushing the changes.

Sounds great idea, will do so!

This change will be unneeded if we change to spawn goroutines for each repository according to your suggestion in the above comment, right?

Dumb... still remains an issue adding a new commit may cause a pull error in the future.

Leave TODO comments so may I address them when I work on the issue to spawn goroutines for each repo?

nghialv · 2020-12-16T04:34:45Z

pkg/app/piped/imagewatcher/watcher.go

+	if err := f.Close(); err != nil {
+		return fmt.Errorf("failed to close file %s: %w", path, err)
+	}
+	err = repo.Push(ctx, repo.GetClonedBranch())


We need to commit the changes before pushing.

Yup, that's the basic part...

nghialv · 2020-12-16T04:38:48Z

pkg/config/testdata/piped/piped-config.yaml

    #   encryptServiceAccountFile: /etc/piped-secret/encrypt-service-account.json

  imageWatcher:
-    repos:


One vote from me to keep it as repos. It may contain not only the filter thing but also other configurable fields (e.g. interval).

still do not have a good enough ideal for the name but the previous repos is lgtm. cc @nakabonne

From now my thought has been varied. Let's keep as is.

still do not have a good enough ideal for the name

Yup, I keep thinking the best name :D

pipecd-bot · 2020-12-16T06:09:46Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Spawn goroutines for each repository

https://github.com/pipe-cd/pipe/blob/c70a038f0c4c348c572ff7fa0e1fa16eb045ae7d/pkg/app/piped/imagewatcher/watcher.go#L69-L72

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.

2. Clone repository another temporary destination

https://github.com/pipe-cd/pipe/blob/c70a038f0c4c348c572ff7fa0e1fa16eb045ae7d/pkg/app/piped/imagewatcher/watcher.go#L72-L75

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.

3. Make it changeable the commit message

https://github.com/pipe-cd/pipe/blob/c70a038f0c4c348c572ff7fa0e1fa16eb045ae7d/pkg/app/piped/imagewatcher/watcher.go#L206-L209

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.

nakabonne · 2020-12-16T06:10:38Z

@nghialv Fixed them. I'll address the created three issues in the next PR.

pipecd-bot · 2020-12-16T06:15:58Z

Code coverage for golang is 33.81%. This pull request does not change code coverage.

nghialv · 2020-12-16T06:17:29Z

/lgtm

nakabonne · 2020-12-16T06:18:01Z

/cc @khanhtc1202

khanhtc1202 · 2020-12-16T06:32:46Z

thanks 👍
/approve

pipecd-bot · 2020-12-16T06:32:49Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Enable to push outdated images

0da59fd

pipecd-bot added release-note-none size/L area/piped labels Dec 16, 2020

pipecd-bot requested review from khanhtc1202 and nghialv December 16, 2020 01:33

khanhtc1202 reviewed Dec 16, 2020

View reviewed changes

nghialv reviewed Dec 16, 2020

View reviewed changes

nakabonne added 2 commits December 16, 2020 14:24

Revert filter to repos

f4221de

Ensure to commit

c70a038

pipecd-bot added the lgtm label Dec 16, 2020

pipecd-bot requested a review from khanhtc1202 December 16, 2020 06:18

pipecd-bot added the approved label Dec 16, 2020

pipecd-bot merged commit 5ceca2c into master Dec 16, 2020

pipecd-bot deleted the update-images branch December 16, 2020 06:33

pipecd-bot mentioned this pull request Dec 16, 2020

Spawn goroutines for each repository #1261

Closed

This was referenced Dec 16, 2020

Clone repository another temporary destination #1262

Closed

Make it changeable the commit message #1263

Closed

Add doc for pipectl #1266

Merged

Enable to push outdated images #1258

Enable to push outdated images #1258

Uh oh!

Conversation

nakabonne commented Dec 16, 2020

Uh oh!

pipecd-bot commented Dec 16, 2020

Uh oh!

khanhtc1202 commented Dec 16, 2020

Uh oh!

khanhtc1202 Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 16, 2020

Uh oh!

nakabonne commented Dec 16, 2020

Uh oh!

nghialv Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Dec 16, 2020

1. Spawn goroutines for each repository

This was created by todo plugin since "TODO:" was found in c70a038 when #1258 was merged. cc: @nakabonne.

2. Clone repository another temporary destination

This was created by todo plugin since "TODO:" was found in c70a038 when #1258 was merged. cc: @nakabonne.

3. Make it changeable the commit message

This was created by todo plugin since "TODO:" was found in c70a038 when #1258 was merged. cc: @nakabonne.

Uh oh!

nakabonne commented Dec 16, 2020

Uh oh!

pipecd-bot commented Dec 16, 2020

Uh oh!

nghialv commented Dec 16, 2020

Uh oh!

khanhtc1202 Dec 16, 2020 •

edited

Loading

nghialv Dec 16, 2020 •

edited

Loading

nakabonne Dec 16, 2020 •

edited

Loading

nghialv Dec 16, 2020 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `c70a038` when #1258 was merged. cc: @nakabonne.