fix: modify error handling of SyncerMux#Sync

[nyuta01]

Fixes: #4246

Description

Fixed an issue where DevLoop would stop working if multiple services were running at the same time.

In the case of multiple deployers using DeployerMux[1], even though the update target may not exist depending on the called syncer[2], the behavior at this time is regarded as an error and the update is stopped[3].

[1] https://github.com/GoogleContainerTools/skaffold/blob/v1.35.0/pkg/skaffold/runner/deployer.go#L126
[2] https://github.com/GoogleContainerTools/skaffold/blob/v1.35.0/pkg/skaffold/sync/syncer_mux.go#L26
[3] https://github.com/GoogleContainerTools/skaffold/blob/v1.35.0/pkg/skaffold/runner/v1/dev.go#L95

User facing changes (remove if N/A)

skaffold v1.35.0.
https://github.com/GoogleContainerTools/skaffold/tree/v1.35.0

$ skaffold dev -d $DEFAULT_REPO
...
// Change demo2
DEBU[0014] Change detectednotify.Write: "/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go"  subtask=-1 task=DevLoop
DEBU[0014] Change detectednotify.Write: "/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go"  subtask=-1 task=DevLoop
[app] [00] demo1 - Hello world!!
[app] [00] demo2 - Hello world!!
DEBU[0015] Found dependencies for dockerfile: [{go.mod /app true 5 5} {go.sum /app true 5 5} {. /app true 8 8}]  subtask=-1 task=DevLoop
DEBU[0015] Found dependencies for dockerfile: [{go.mod /app true 5 5} {go.sum /app true 5 5} {. /app true 8 8}]  subtask=-1 task=DevLoop
INFO[0015] files modified: [/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go]  subtask=-1 task=DevLoop
DEBU[0016]  devloop: build false, sync true, deploy false  subtask=-1 task=DevLoop
Syncing 1 files for gcr.io/xxx/temp/demo2:aaaf972@sha256:d06a6275f7163adc656ae4fa4b267b125a4cba16d3d50effccf7edb4fee7684b
INFO[0016] Copying files:map[/yyy/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go:[/app/main.go]]togcr.io/yyy/temp/demo2:aaaf972@sha256:d06a6275f7163adc656ae4fa4b267b125a4cba16d3d50effccf7edb4fee7684b  subtask=-1 task=DevLoop
DEBU[0016] getting client config for kubeContext: `zzz`  subtask=-1 task=DevLoop
WARN[0016] Skipping deploy due to sync error:copying files: didn't sync any files  subtask=-1 task=DevLoop
Watching for changes...

skaffold PR version.

$ skaffold dev -d $DEFAULT_REPO
...
// Change demo2
DEBU[0033] Change detectednotify.Write: "/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go"  subtask=-1 task=DevLoop
DEBU[0033] Change detectednotify.Write: "/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go"  subtask=-1 task=DevLoop
DEBU[0034] Found dependencies for dockerfile: [{go.mod /app true 5 5} {go.sum /app true 5 5} {. /app true 8 8}]  subtask=-1 task=DevLoop
DEBU[0034] Found dependencies for dockerfile: [{go.mod /app true 5 5} {go.sum /app true 5 5} {. /app true 8 8}]  subtask=-1 task=DevLoop
INFO[0034] files modified: [/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go]  subtask=-1 task=DevLoop
DEBU[0036]  devloop: build false, sync true, deploy false  subtask=-1 task=DevLoop
Syncing 1 files for gcr.io/yyy/temp/demo2:aaaf972-dirty@sha256:d80fc1ba3a9368eb61ebfaea38f9436da5a242345152fc691718daee7982f6a4
INFO[0036] Copying files:map[/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go:[/app/main.go]]togcr.io/yyy/temp/demo2:aaaf972-dirty@sha256:d80fc1ba3a9368eb61ebfaea38f9436da5a242345152fc691718daee7982f6a4  subtask=-1 task=DevLoop
DEBU[0036] getting client config for kubeContext: `zzz`  subtask=-1 task=DevLoop
INFO[0036] Copying files:map[/xxx/github.com/nyuta01/skaffold-devloop-demo/demo2/main.go:[/app/main.go]]togcr.io/yyy/temp/demo2:aaaf972-dirty@sha256:d80fc1ba3a9368eb61ebfaea38f9436da5a242345152fc691718daee7982f6a4  subtask=-1 task=DevLoop
DEBU[0036] getting client config for kubeContext: `zzz`  subtask=-1 task=DevLoop
DEBU[0036] Running command: [kubectl --context zzz exec demo2 --namespace demo2 -c app -i -- tar xmf - -C / --no-same-owner]  subtask=-1 task=DevLoop
DEBU[0036] Command output: [], stderr: tar: Removing leading `/' from member names  subtask=-1 task=DevLoop
Watching for changes...
[app] [00] Killing service
[app] [00] ^Csignal: interrupt
[app] [00] Starting service
[app] [00] demo2 - Hello world!!
[app] [00] demo1 - Hello world!!

Follow-up Work (remove if N/A)

demo
https://github.com/nyuta01/skaffold-devloop-demo

[nyuta01]

I have signed the CLA, please Re-run workflow.

📝 If you are not currently covered under a CLA, please visit https://cla.developers.google.com/. Once you've signed (or fixed any issues), please request a rescan from a repository manager.
https://github.com/GoogleContainerTools/skaffold/pull/6934/checks?check_run_id=4415693176

[tejal29]

@nyuta01 Thanks for your fix.

IIUC, your change will resume the devloop for other services, is that right?
Our current strategy is to exit early and not continue with the deploy.

[codecov]

Codecov Report

Merging #6934 (701af9e) into main (290280e) will decrease coverage by 1.35%.
The diff coverage is 61.83%.

@@            Coverage Diff             @@
##             main    #6934      +/-   ##
==========================================
- Coverage   70.48%   69.12%   -1.36%     
==========================================
  Files         515      547      +32     
  Lines       23150    25072    +1922     
==========================================
+ Hits        16317    17332    +1015     
- Misses       5776     6580     +804     
- Partials     1057     1160     +103     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	52.00% <ø> (-1.85%)	⬇️
cmd/skaffold/app/cmd/dev.go	84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/flags.go	91.00% <0.00%> (+0.18%)	⬆️
cmd/skaffold/app/cmd/render.go	36.66% <0.00%> (-4.72%)	⬇️
cmd/skaffold/skaffold.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go	62.50% <14.28%> (-1.14%)	⬇️
cmd/skaffold/app/cmd/fix.go	68.85% <40.00%> (-7.62%)	⬇️
cmd/skaffold/app/cmd/lint.go	42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go	48.88% <50.00%> (+0.24%)	⬆️
cmd/skaffold/app/skaffold.go	76.19% <70.00%> (-8.43%)	⬇️
... and 167 more

---

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 feafa43...701af9e. Read the comment docs.

[nyuta01]

@tejal29

Thanks for the reply.

Yes, this PR has been changed to not stop Devloop if any one of the multiple syncer is successfully updated.

I see, I didn't have a correct understanding of skaffold's strategy.
By the way, I would like to ask you one point related to that strategy.
The current skaffold dev command has a problem that Devloop will stop if multiple services with different namespace are started and the program is updated [1].
(When there are multiple syncer with different namespaces, if numSynced becomes 0 in any of the syncer[2], devloop will stop[3].)
Is this expected behavior?

[1] https://github.com/nyuta01/skaffold-devloop-demo
[2] https://github.com/GoogleContainerTools/skaffold/blob/v1.35.0/pkg/skaffold/sync/sync.go#L353
[3] https://github.com/GoogleContainerTools/skaffold/blob/v1.35.0/pkg/skaffold/runner/v1/dev.go#L95

[tejal29]

I see. From your example, looks like you are using skaffold modules. The question is what should skaffold do if one module sync fails. Does it make sense to sync other modules?

Sync is a light weight operation and I feel it should be fine.
When considering deploy, do you also expect skaffold to deploy other modules or microservices?

[nyuta01]

Sorry, I didn't explain it well enough.
It is true that my sample project uses multi-module[1].

Yes. The problem is how to handle the other modules when the sync in one module fails.

Does it make sense to sync other modules?

Yes. it makes sense.

Let me explain why using my sample project as an example.

First of all, there are two modules in this project, demo1 and demo2, and for the sake of development, we want to run both modules at the same time.

To accomplish this requirement, I ran the following command.

$ git clone git@github.com:nyuta01/skaffold-devloop-demo.git
$ cd skaffold-devloop-demo
$ skaffold dev -d $DEFAULT_REPO

Listing files to watch...
 - temp/demo1
 - temp/demo2
Generating tags...
 - temp/demo1 -> $DEFAULT_REPO/temp/demo1:xxx
 - temp/demo2 -> $DEFAULT_REPO/temp/demo2:xxx
Checking cache...
 - temp/demo1: Found. Tagging
 - temp/demo2: Found. Tagging
Tags used in deployment:
 - temp/demo1 -> $DEFAULT_REPO/temp/demo1:xxx@sha256:yyy
 - temp/demo2 -> $DEFAULT_REPO/temp/demo2:xxx@sha256:yyy
Starting deploy...
 - namespace/demo1 created
 - pod/demo1 created
 - namespace/demo2 created
 - pod/demo2 created
Waiting for deployments to stabilize...
Deployments stabilized in 181.40925ms
Press Ctrl+C to exit
Watching for changes...
[app] [00] Starting service
[app] [00] demo1 - Hello world!!
[app] [00] Starting service
[app] [00] demo2 - Hello world!!

Next, I wanted to make some changes to demo2.
For example, i made the following changes and saved it.

// demo2/main.go
func main() {
	for {
-		fmt.Println("demo2 - Hello world!!")
+		fmt.Println("demo2 - Hello world!!!")

		time.Sleep(time.Second * 10)
	}
}

Now I have a problem.
I made some changes to the demo2 module, but the changes were not reflected.

Syncing 1 files for $DEFAULT_REPO/temp/demo2:xxx@sha256:yyy
WARN[0067] Skipping deploy due to sync error:copying files: didn't sync any files  subtask=-1 task=DevLoop
Watching for changes...

$ kubectl logs demo2 -n demo2 | tail -n 3
[00] demo2 - Hello world!!
[00] demo2 - Hello world!!
[00] demo2 - Hello world!!

Why did this problem occur?
As explained in the previous comments, the current implementation has multiple modules, and if any of them fails to sync, the DevLoop will stop at that point [2].
In this example, the DevLoop of demo1 failed, causing demo2 to not be sync and the changes not being reflected.
This makes it impossible to run a continuous development cycle using skaffold dev.

This explains why it is important to sync other modules.
(I hope I didn't miss anything in this explanation...)

[1] https://skaffold.dev/docs/design/config/#multiple-configuration-support
[2] #6934 (comment)

[MarlonGamez]

@nyuta01 Thank you very much for the in depth explanations and examples. I think these changes make sense, and seems to fix the issue and provide a better experience.

@tejal29 if you agree I think we should merge this