Jib Auto Sync - core + maven implementation #3369

loosebazooka · 2019-12-12T00:09:23Z

Just putting this up here for now, if it feels like I'm messing too much with the core system maybe we can restructure that. I'll clean up the code some more and add tests if we can kinda agree on the changes in here?

So most of this is baked into the jib package, but I don't think anything in it necessarily HAS to be jib specific. If we want to extend the model out for other systems we can. But it mostly
feels like a java problem anyway?
Added an InitSync before first build in dev mode to initalize sync of the files - requires a build to get the sync data
I feel like there's some weird cases I haven't handled yet
logging is wrong, probably need to fix that

So most of this is baked into the jib package, but I don't think anything in it necessarily HAS to be jib specific. If we want to extend the model out for other systems we can. But it mostly feels like a java problem anyway? Gradle is not covered here.

chanseokoh

Overall, I'm not very familiar with how exactly this works, so there are many questions.

chanseokoh · 2019-12-12T16:12:31Z

pkg/skaffold/build/jib/maven.go

@@ -85,8 +85,24 @@ func getCommandMaven(ctx context.Context, workspace string, a *latest.JibArtifac
 	return MavenCommand.CreateCommand(ctx, workspace, args)
 }

+func getSyncMapCommandMaven(ctx context.Context, workspace string, a *latest.JibArtifact) *exec.Cmd {
+	cmd := MavenCommand.CreateCommand(ctx, workspace, mavenBuildArgs("_skaffold-sync-map", a, true))


mavenBuildArgs append either package or prepare-package. Does that make sense? ~~And I don't really think _skaffold-sync-map will work. It should be jib:_skaffold-sync-map?~~

Also we pass --quiet for _skaffold-files_v2. But I'm not sure if --quiet helps, because I didn't see any Maven output in your short demo.

Sorry, forget prepending jib:_. I see it's added in mavenBuildArgs.

chanseokoh · 2019-12-12T16:16:57Z

pkg/skaffold/build/jib/jib_sync.go

+	}
+
+	// To parse the output, search for "BEGIN JIB JSON", then unmarshal the next line into the pathMap struct.
+	matches := regexp.MustCompile(`BEGIN JIB JSON\r?\n({.*})`).FindSubmatch(stdout)


I liked @briandealwis's idea to have this extensible. Something we should keep in mind to make things match.

chanseokoh · 2019-12-12T16:19:14Z

pkg/skaffold/build/jib/jib_sync.go

+	if (err != nil) {
+		return err
+	}
+	syncLists[artifact.Project] = *syncMap


Is it possible that multiple Java projects have the same "JibArtifact.Project" name as a key to the map? The map is a global map, isn't it?

And isn't artifact.Project == "" for a single-module project setup?

chanseokoh · 2019-12-12T16:29:57Z

pkg/skaffold/build/jib/jib_sync.go

+	Src  string `json:"src"`
+	Dest string `json:"dest"`
+
+	filetime time.Time


Does an entry always refer to a file and not a directory? Just to understand what the modification time of a directory may mean, if it can be a directory. But I kind of remember that our sync map will list every single file and not directories?

And I always forget. For a Java file, will the entry be a .java file or a .class file?

chanseokoh · 2019-12-12T16:32:35Z

pkg/skaffold/build/jib/jib_sync.go

+		}
+		for _, bf := range buildFiles {
+			if f == bf {
+				return nil, nil, nil


Considering the method name, I think it's worth adding a comment nil, nil means a full rebuild.

chanseokoh · 2019-12-12T16:38:42Z

pkg/skaffold/build/jib/jib_sync.go

+	}
+
+	// we need to do another build and get a new sync map
+	newSyncMap, err := getSyncMap(ctx, workspace, artifact)


Oh, so getSyncMap is really supposed to trigger a build itself (i.e., doing package)?

I'd like to understand how this could potentially interfere (race condition) with normal file watching, as we talked about before.

chanseokoh · 2019-12-12T16:44:50Z

pkg/skaffold/build/jib/jib_sync.go

+func updateModTime(se []SyncEntry) error {
+	for i, _ := range se {
+		e := &se[i]
+		if info, err := os.Stat(e.Src); err != nil {


I hope this (and the operation to compute absolute path above) is cheap to do. The sync map will inherently be small, so we should probably good?

briandealwis · 2019-12-18T15:38:54Z

pkg/skaffold/build/jib/jib.go

@@ -88,6 +88,10 @@ type filesLists struct {
 // watchedFiles maps from project name to watched files
 var watchedFiles = map[string]filesLists{}

+func GetBuildDefinitions(a *latest.JibArtifact) []string {
+		return watchedFiles[a.Project].BuildDefinitions


I suspect a.Project needs to be combined with the artifact context. As @chanseokoh noted below, a.Project may be the empty string, and it's entirely possible to have two artifacts with different contexts (workspaces) and same project name.

briandealwis · 2020-01-06T20:32:49Z

pkg/skaffold/build/jib/jib_sync.go

+
+var syncLists = make(map[string]SyncMap)
+
+type SyncMap struct {


Add some go doc to describe that this structure is filled in from information from the jib builder (mvn jib:_.../gradle _jib...). I have to admit that Direct didn't have obvious semantics, but I don't have anything better to suggest.

briandealwis · 2020-01-06T20:34:55Z

pkg/skaffold/build/jib/jib_sync.go

+// returns toCopy, toDelete, error
+func GetSyncDiff(ctx context.Context, workspace string, artifact *latest.JibArtifact, e filemon.Events) (map[string][]string, map[string][]string, error) {


You could consider using named return values to make this more explicit:

func GetSyncDiff(...) (toCopy map[string][]string, toDelete map[string][]string, err error) {

Or maybe it'd be better to return an explicit struct SyncDiff?

briandealwis · 2020-01-06T20:38:00Z

pkg/skaffold/build/jib/jib_sync.go

+
+	if len(e.Deleted) != 0 {
+		// change into logging
+		fmt.Println("Deletions are not supported by jib auto sync at the moment")


Suggested change

fmt.Println("Deletions are not supported by jib auto sync at the moment")

fmt.Println("Deletions are not yet supported by jib auto sync")

briandealwis · 2020-01-06T20:44:04Z

pkg/skaffold/build/jib/jib_sync.go

+		return errors.New("failed to get Jib Sync data")
+	}
+
+	line := bytes.Replace(matches[1], []byte(`\`), []byte(`\\`), -1)


This line throws me every time I come across it. I believe this replace is required is because of Windows paths? Should we instead fix our emitting code in Jib?

briandealwis · 2020-01-06T20:49:42Z

pkg/skaffold/build/jib/jib_sync.go

+
+
+	// if all files are modified and direct, we don't need to build anything
+	if len(e.Deleted) == 0 || len(e.Added) == 0 {


|| seems at odd with the comment? And we could skip this if len(e.Modified) > len(oldSyncMap.Direct)?

briandealwis · 2020-01-06T20:54:05Z

pkg/skaffold/build/jib/jib_sync.go

+				if !filepath.IsAbs(f) {
+					if ff, err := filepath.Abs(f); err != nil{
+						return nil, nil, err
+					} else {
+						f = ff
+					}
+				}


This could be moved to the outer for loop

briandealwis · 2020-01-06T21:13:00Z

pkg/skaffold/build/jib/jib_sync.go

+	}
+
+	// it seems less than efficient to keep the original JSON structure when doing look ups, so maybe we should only use the json objects for serialization
+	// and store the sync data in a better type?


The original format leads to O(n) checks

briandealwis · 2020-01-06T21:16:41Z

pkg/skaffold/sync/sync.go

+		if a.Sync != nil && a.Sync.Auto != nil{
+			if a.JibArtifact != nil {
+				if err := jib2.InitSync(ctx, a.Workspace, a.JibArtifact); err != nil {
+					// maybe should just disable sync here, instead of dead?


Suggested change

// maybe should just disable sync here, instead of dead?

// maybe should just disable sync here, instead of dying?

loosebazooka · 2020-01-15T17:36:38Z

this is covered by a bunch of new PRs

loosebazooka requested review from balopat, briandealwis, chanseokoh, dgageot, nkubala, TadCordle and tejal29 as code owners December 12, 2019 00:09

googlebot added the cla: yes label Dec 12, 2019

pull-request-size bot added the size/L label Dec 12, 2019

loosebazooka added build/jib and removed cla: yes size/L labels Dec 12, 2019

googlebot added the cla: yes label Dec 12, 2019

loosebazooka added !! wip !! and removed cla: yes labels Dec 12, 2019

googlebot added the cla: yes label Dec 12, 2019

chanseokoh reviewed Dec 12, 2019

View reviewed changes

loosebazooka mentioned this pull request Dec 13, 2019

Add new Auto sync option #3382

Merged

briandealwis reviewed Jan 6, 2020

View reviewed changes

chanseokoh mentioned this pull request Jan 8, 2020

Use unique key for jib caches #3483

Merged

tejal29 assigned balopat Jan 14, 2020

loosebazooka closed this Jan 15, 2020

dgageot deleted the jib-sync-auto branch May 11, 2020 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jib Auto Sync - core + maven implementation #3369

Jib Auto Sync - core + maven implementation #3369

loosebazooka commented Dec 12, 2019

chanseokoh left a comment

chanseokoh Dec 12, 2019 •

edited

Loading

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

chanseokoh Dec 12, 2019

briandealwis Dec 18, 2019

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

briandealwis Jan 6, 2020

loosebazooka commented Jan 15, 2020


		var syncLists = make(map[string]SyncMap)

		type SyncMap struct {

		// returns toCopy, toDelete, error
		func GetSyncDiff(ctx context.Context, workspace string, artifact *latest.JibArtifact, e filemon.Events) (map[string][]string, map[string][]string, error) {

	fmt.Println("Deletions are not supported by jib auto sync at the moment")
	fmt.Println("Deletions are not yet supported by jib auto sync")



		// if all files are modified and direct, we don't need to build anything
		if len(e.Deleted) == 0 \|\| len(e.Added) == 0 {

	// maybe should just disable sync here, instead of dead?
	// maybe should just disable sync here, instead of dying?

Jib Auto Sync - core + maven implementation #3369

Jib Auto Sync - core + maven implementation #3369

Conversation

loosebazooka commented Dec 12, 2019

chanseokoh left a comment

Choose a reason for hiding this comment

chanseokoh Dec 12, 2019 • edited Loading

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

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

loosebazooka commented Jan 15, 2020

chanseokoh Dec 12, 2019 •

edited

Loading