Add RFC for Image Watcher #1114

nakabonne · 2020-11-18T03:08:49Z

What this PR does / why we need it:
I'd be happy to consider another way to display architecture if it's a problem to include in this repo.

Which issue(s) this PR fixes:

For #69

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2020-11-18T03:14:14Z

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

nghialv · 2020-11-19T02:59:00Z

Sorry, I will review this after releasing v0.9.0.

nakabonne · 2020-11-19T03:18:44Z

Nop, take a look it at your convenience.

nghialv · 2020-11-19T06:34:07Z

docs/rfcs/0001-image-watcher.md

+        passwordFile: /etc/piped-secret/dockerhub-pass
+```
+
+We define the information about image tag underneath `.pipe/` directory:


What happens when multiple pipeds handling the same configuration repository?

I feel like no problem even in that case. Image Watcher does just sync the tag in Git with one in Container Registry. If multiple Piped push to the Git repo, only one push will be successful.

And I'd say the case of using multiple Piped for the single repo is an edge case, but what do you think?

Depends on the way Piped commits the changes. If it directly pushes the change to the specified branch, it is ok for handling the pushing error. If it pushes the change via PR, the conflicted pull requests will remain.

Alternatively, we can update the piped configuration to watch only the specified files in the .piped directory.

I guess we can consider it is the user's responsibility to ensure that one image is watched by one Piped, but how about you?

Yes, I totally agree that it should be done by the user, yes their responsibility.
Just want to make sure we can provide a way for them to get it right.

For the use case at our company, the AB... team is going to store all configuration in a single repository, and I guess they will have at least 4 pipeds handling that repo. All of them run separately and loading the same .pipe directory.

Looks good, that's more conventional.

Ah, I now found you specify .pipe/imagewatcher.yaml as an excluded file. Does it mean to ignore/load the entire repository instead of specifying a specific application?

It means the including/excluding can be done at the image-watcher file level, not the app/manifest-path level.
For a big repository, if they want more control on multiple pipeds they should split into multiple image-watcher files inside the .pipe directory.

Let me tell you the case I consider.

Let's say that multiple environment settings are in the same repository:

repo-root ├── dev/ ├── prod/

Each piped should only monitor images for their environment.

dev piped should be:

apiVersion: pipecd.dev/v1beta1 kind: Piped spec: ... repositories: - repoId: foo remote: git@github.com:nakabonne/foo.git branch: master imageProviders: - name: my-dockerhub type: DOCKERHUB interval: 5m config: username: user passwordFile: /etc/piped-secret/dockerhub-pass # Optional imageWatcher: - repoId: foo includes: - dev/

prod piped config should be:

``yaml apiVersion: pipecd.dev/v1beta1 kind: Piped spec: ... repositories: - repoId: foo remote: git@github.com:nakabonne/foo.git branch: master imageProviders: - name: my-dockerhub type: DOCKERHUB interval: 5m config: username: user passwordFile: /etc/piped-secret/dockerhub-pass # Optional imageWatcher: - repoId: foo includes: - prod/

For a big repository, if they want more control on multiple pipeds they should split into multiple image-watcher files inside the .pipe directory.

That makes sense!

nghialv · 2020-11-24T09:40:07Z

docs/rfcs/0001-image-watcher.md

+
+### Architecture
+`Image Provider` pulls respectively periodically.
+`Image Watcher` pulls from the git repo, compares respectively and pushes the one with differences.


Can we have a description of the interaction between piped and Git?
Does it push the changes directly to the specified branch or through a pull request?

I'm thinking about pushing directly.

I think so too. Because pull request is not a primitive feature of Git. It depends on the Git hosting service such as GitHub, GitLab, BitBucket...
Can we have a sentence to clarify that thing?

Yeah exactly, I'd happy to make it clear.

docs/rfcs/0001-image-watcher.md

nghialv · 2020-11-25T02:26:27Z

docs/rfcs/0001-image-watcher.md

+        passwordFile: /etc/piped-secret/dockerhub-pass
+```
+
+We define the information about image tag underneath `.pipe/` directory:


Depends on the way Piped commits the changes. If it directly pushes the change to the specified branch, it is ok for handling the pushing error. If it pushes the change via PR, the conflicted pull requests will remain.

Alternatively, we can update the piped configuration to watch only the specified files in the .piped directory.

nghialv · 2020-11-25T02:30:37Z

docs/rfcs/0001-image-watcher.md

+
+# Unresolved questions
+
+- How to update template. Currently considering using https://github.com/vmware-tanzu/carvel-ytt


As far as I know, the main target of carvel-ytt is providing a tool for end-user, not a library for developers. So importing its go-package maybe not a good choice. They can make any change to those packages in the future.

I was attempting to execute it as a child process of piped, but anyway still not enough investigation at all.

Rather than keeping unresolved questions as is, should we merge this RFC after investigating enough?

I was attempting to execute it as a child process of piped, but anyway still not enough investigation at all.

If possible, we should avoid running those tools by using cmd. Because it requires installing the tool beside our piped binary. The user should install only a single piped to use.

Rather than keeping unresolved questions as is, should we merge this RFC after investigating enough?

I am not sure but I think the Unresolved Questions section is for storing the questions that still not be resolved after our discussion here.

I got it. I will glad to investigate more 👍

docs/rfcs/0001-image-watcher.md

nghialv · 2020-11-25T02:39:19Z

docs/rfcs/0001-image-watcher.md

+      provider: my-dockerhub
+```
+
+# Unresolved questions


Any thoughts about dealing with the rate limit from the container registry?
(caching, grouping, or triggering by CI event...)

Yeah, that's what I'm most worried about. The rate limit per second may be reached when there are a bunch of target images for a single Image Provider.

I know it's a terrible way but I'm going to call time.Sleep() at the end of each pull for now.

https://github.com/pipe-cd/pipe/blob/bff58081a707cf01e326540448912523b2a4f639/pkg/app/piped/imagewatcher/watcher.go#L157-L161

The above snippet is in the branch to be deleted after merged, so it will be invisible.

Does any provider have a watch API or something like that?

As far as I investigate, no. Most providers implement the Docker Registry API that has just simple APIs.

It could be a frequent problem, so I'm going to investigate further to see if there's anything else we can do to help.

Thanks for your investigation.

Actually, these were what I thought.

At the initial phase, we do polling as usual.

In the next phase, we provide a FAKE container registry at our control-plane and add a new kind of IMAGE_PROVIDER for the Piped to use. So for the big projects, they can

use our CLI to send metadata about newly created images to our FAKE container registry

change their pipeds to our IMAGE_PROVIDER kind

(Out FAKE container registry only contains the metadata of the images)

Wow, what a brilliant idea! For the large teams, they use the CLI tool in CI or something like that when their pushing is complete, doesn't it?

Okay, I'd be happy to work on the initial phase ;)

And make the RFC clear what each step does.

Yes, they use our CLI to publish the metadata for their newly created images.
For example, from their CI after pushing the image:

pipecd cli push-image --image=abc:v1.0.0 --api-key=key

pipecd-bot · 2020-11-25T03:22:52Z

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

pipecd-bot · 2020-11-25T04:21:22Z

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

…ple ImageWatcher file

pipecd-bot · 2020-12-04T01:16:07Z

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

nakabonne · 2020-12-04T07:58:34Z

@nghialv All unresolved problems have been gone out. Could you do the last check?

pipecd-bot · 2020-12-04T07:59:53Z

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

nghialv · 2020-12-04T08:04:11Z

docs/rfcs/0001-image-watcher.md

+Piped periodically compares the image tags defined in git with the latest tags stored in the container registry, and then pushes them to git if there are any deviations.
+
+### Usage
+Note: Where, we call the client that accesses the container registry to `Image Provider`.


Sorry, what does this mean?

Sorry, it means "Where Image Provider is the client that accesses the container registry". So I'll change to it.

nghialv · 2020-12-04T08:05:20Z

docs/rfcs/0001-image-watcher.md

+        - .pipe/imagewatcher-dev.yaml
+```
+
+Adding an ImageWatcher file at `.pipe/` directory to define what image should be watched and what file should be updated:


nit: "Adding one or more ImageWatcher files"

nghialv · 2020-12-04T08:09:14Z

/lgtm

nakabonne · 2020-12-04T08:10:40Z

Fixed done.

nghialv · 2020-12-04T08:12:13Z

/approve

pipecd-bot · 2020-12-04T08:12:17Z

This pull request is APPROVED by nghialv.

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.

nghialv · 2020-12-04T08:12:22Z

Thank you.

nakabonne · 2020-12-04T08:13:39Z

No, thank YOU!

**What this PR does / why we need it**: Base implementation of #1114. Sorry for the huge patch. I'd be happy to work on tiny issues respectively after this. **Which issue(s) this PR fixes**: Fixes # **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

Add RFC for Image Watcher

cdfdcb8

pipecd-bot added release-note-none area/docs area/site size/M labels Nov 18, 2020

pipecd-bot requested review from cakecatz and nghialv November 18, 2020 03:08

nakabonne mentioned this pull request Nov 18, 2020

Ensure to watch image from image providers #1117

Merged

nghialv reviewed Nov 19, 2020

View reviewed changes

nghialv reviewed Nov 24, 2020

View reviewed changes

nghialv reviewed Nov 25, 2020

View reviewed changes

docs/rfcs/0001-image-watcher.md Outdated Show resolved Hide resolved

nghialv reviewed Nov 25, 2020

View reviewed changes

docs/rfcs/0001-image-watcher.md Outdated Show resolved Hide resolved

nghialv reviewed Nov 25, 2020

View reviewed changes

Fix sentences

683021e

Fix typo

555dc89

nakabonne mentioned this pull request Nov 26, 2020

Add support for insights feature #1142

Closed

7 tasks

nakabonne added 2 commits November 26, 2020 19:20

Update config fields to prevent conflicts by splitting into the multi…

62ddd4d

…ple ImageWatcher file

Add description about how to deal with rate limit

a98a3ef

Remove unresolved questions

326076b

nghialv reviewed Dec 4, 2020

View reviewed changes

pipecd-bot added the lgtm label Dec 4, 2020

Fix sentences

41d9af8

pipecd-bot removed the lgtm label Dec 4, 2020

pipecd-bot added the approved label Dec 4, 2020

pipecd-bot merged commit 4909450 into master Dec 4, 2020

pipecd-bot deleted the rfc-image-watcher branch December 4, 2020 08:19

pipecd-bot mentioned this pull request Dec 16, 2020

Add doc for pipectl #1266

Merged


		# Unresolved questions

		- How to update template. Currently considering using https://github.com/vmware-tanzu/carvel-ytt

Add RFC for Image Watcher #1114

Add RFC for Image Watcher #1114

Conversation

nakabonne commented Nov 18, 2020

pipecd-bot commented Nov 18, 2020

nghialv commented Nov 19, 2020

nakabonne commented Nov 19, 2020

Choose a reason for hiding this comment

nakabonne Nov 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nakabonne Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nakabonne Nov 26, 2020 • edited

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

nakabonne Nov 25, 2020 • edited

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

nakabonne Nov 25, 2020 • edited

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

pipecd-bot commented Nov 25, 2020

pipecd-bot commented Nov 25, 2020

pipecd-bot commented Dec 4, 2020

nakabonne commented Dec 4, 2020

pipecd-bot commented Dec 4, 2020

Choose a reason for hiding this comment

nakabonne Dec 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nghialv commented Dec 4, 2020

nakabonne commented Dec 4, 2020

nghialv commented Dec 4, 2020

pipecd-bot commented Dec 4, 2020

nghialv commented Dec 4, 2020

nakabonne commented Dec 4, 2020

nakabonne Nov 19, 2020 •

edited

nakabonne Nov 25, 2020 •

edited

nakabonne Nov 26, 2020 •

edited

nakabonne Nov 25, 2020 •

edited

nakabonne Nov 25, 2020 •

edited

nakabonne Dec 4, 2020 •

edited