Improve sync #1102
Improve sync #1102
Conversation
dgageot
commented
Oct 5, 2018
dgageot
commented
- Add more logs and fix some of them
- Improve errors
- Pass context to commands
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
We can’t use skaffold labels to narrow down the list of pods. Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
Codecov Report
@@ Coverage Diff @@
## master #1102 +/- ##
==========================================
- Coverage 43.16% 42.96% -0.21%
==========================================
Files 74 74
Lines 3408 3403 -5
==========================================
- Hits 1471 1462 -9
- Misses 1799 1801 +2
- Partials 138 140 +2
Continue to review full report at Codecov.
|
| if err := r.Syncer.Sync(s); err != nil { | ||
| color.Default.Fprintf(out, "Syncing %d files for %s\n", len(s.Copy)+len(s.Delete), s.Image) | ||
|
|
||
| if err := r.Syncer.Sync(ctx, s); err != nil { | ||
| logrus.Warnln("Skipping build and deploy due to sync error:", err) | ||
| return nil |
There was a problem hiding this comment.
I forgot to add this but if we get an error here, we should add that dirty artifact back to the rebuild list
There was a problem hiding this comment.
How would you do that?
| @@ -271,12 +271,12 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la | |||
| return ErrorConfigurationChanged | |||
| case len(changed.needsResync) > 0: | |||
| for _, s := range changed.needsResync { | |||
| if err := r.Syncer.Sync(s); err != nil { | |||
| color.Default.Fprintf(out, "Syncing %d files for %s\n", len(s.Copy)+len(s.Delete), s.Image) | |||
There was a problem hiding this comment.
You mean I'm rebased on old code?
There was a problem hiding this comment.
Yeah just maybe the last commit. I changed this message to the debug and info statement.
There was a problem hiding this comment.
Oh, the one I asked you to add? It didn't work well in reality :-) Didn't give enough feedback
| LabelSelector: labelSelector(), | ||
| }) | ||
|
|
||
| pods, err := client.CoreV1().Pods("").List(meta_v1.ListOptions{}) |
There was a problem hiding this comment.
use the skaffold labelselector here
There was a problem hiding this comment.
I removed it because it wasn't working. What I saw is that when I use a deployment, this deployment gets the labels but not the pods.
pkg/skaffold/kubernetes/sync.go
Outdated
| return errors.Wrap(err, "syncing files") | ||
| } | ||
|
|
||
| if int(performed) != len(files) { |
There was a problem hiding this comment.
I don't think you need to count here. errgroup handles this
There was a problem hiding this comment.
The check is to make sure that we try to perform the sync for n files. It has nothing to do with errors. For example, with the code that filters on skaffold labels, it always syncs zero files without giving an error.
There was a problem hiding this comment.
Hm, I guess this is a bit tricky. What if we have 2 replicas?
There was a problem hiding this comment.
Good point, I'll see how to fix it. Each file should be synced at least once, I guess.
There was a problem hiding this comment.
Yeah I think thats the best we can do. But making sure we sync at least once is a good check.
pkg/skaffold/kubernetes/sync.go
Outdated
| if err := perform(s.Image, s.Copy, copyFileFn); err != nil { | ||
| return errors.Wrap(err, "copying files") | ||
| func (k *KubectlSyncer) Sync(ctx context.Context, s *sync.Item) error { | ||
| if len(s.Copy) > 0 { |
There was a problem hiding this comment.
maybe move the len check into perform? a few lines of code saved
Sync files in a sequential manner, it makes the code simpler and anyway, the typical use case will be to sync one file with one pod. Signed-off-by: David Gageot <david@gageot.net>
