[merged] Atomic/diff.py: Use go-mtree for file comparisons by baude · Pull Request #777 · projectatomic/atomic

baude · 2016-12-01T16:57:02Z

The previous algorithm for comparing files used python's
dircmp and is considered to be a shallow comparision. This
allowed distinctly small possibilities that two files being
compared could be different but not caught.

We now use go-mtree to do the comparison. This can emulate the
shallow comparison we had before but we can also adding a
sha256digest as part of the comparison using the new --keywords
option.

Also, made slight tweaks to gomtree functions in Atomic.util
so we debug and influence the return of JSON data.

This solves #761

twaugh · 2016-12-01T16:58:28Z

Atomic/diff.py

+    diffp.add_argument("-k", "--keywords", nargs='+',
+                       choices=['link', 'nlink', 'mode', 'type', 'time', 'uid', 'gid', 'size', 'sha256digest'],
+                       default=['link', 'nlink', 'mode', 'type', 'time', 'uid', 'gid', 'size'],
+                       help=_("iExclusive keywords to be used for file level comparision"))


yeah, vim command :)

twaugh · 2016-12-01T16:59:09Z

This works well for me.

rhatdan · 2016-12-01T17:41:39Z

Atomic/diff.py

+    diffp.add_argument("-k", "--keywords", nargs='+',
+                       choices=['link', 'nlink', 'mode', 'type', 'time', 'uid', 'gid', 'size', 'sha256digest'],
+                       default=['link', 'nlink', 'mode', 'type', 'time', 'uid', 'gid', 'size'],
+                       help=_("Exclusive keywords to be used for file level comparision"))


Should you add "all"

@rhatdan I can do 'all' but if we ever add two algo's for digesting, they both would then get used. Adding for now as it makes sense.

rhatdan · 2016-12-01T17:44:26Z

Atomic/diff.py

-        self.chroot_left = chroot_left
-        self.chroot_right = chroot_right
-        self.delta(self.compare)
+        self.chroot_left = [] #chroot_left


Comments are useless, either describe or remove the comment.

rhatdan · 2016-12-01T17:45:32Z

Atomic/diff.py

+        self.parse_mtree_json()
+
+    def parse_mtree_json(self):
+        def extra(_result): #pylint: disable=unused-variable


These variables are used, aren't they, remove pylint?

Because of the use of eval below, pylint doesnt recognize they are used.

Leaving until better solution.

Ok, I thought they might have been left over.

rhatdan · 2016-12-01T17:45:44Z

Atomic/diff.py

+        def extra(_result): #pylint: disable=unused-variable
+            self.right.append(_result['path'])
+
+        def missing(_result): #pylint: disable=unused-variable


Same as above

Because of the use of eval below, pylint doesnt recognize they are used.

Leaving until better solution.

rhatdan · 2016-12-01T17:45:58Z

Atomic/diff.py

+        def missing(_result): #pylint: disable=unused-variable
+            self.left.append(_result['path'])
+
+        def modified(_result): #pylint: disable=unused-variable


Because of the use of eval below, pylint doesnt recognize they are used.

Leaving until better solution.

rhatdan · 2016-12-01T17:47:16Z

Atomic/diff.py

            _print_diff(self.right)
        if len(self.common_diff):
-            util.write_out("\nCommon files that are different:")
+            util.write_out("\nCommon files that are different: (reason)")


Not sure what you are doing here?

I supply a reason in the output to show why things are different. Example:

Files only in centos:latest: usr/bin/view usr/bin/ex usr/bin/rvi usr/bin/vi etc/virc usr/bin/rview Files only in 19f8d8b7147b: run/secrets Common files that are different: (reason) var/lib/rpm/Basenames (time) var/lib/rpm/Packages (time)

How does that happen. This looks like it will just output
"(reason)"

That LOC is just to print the header(title). The rest comes in the _print_diff method.

Do you handle this if there are multiple reasons?

rhatdan · 2016-12-01T17:48:08Z

bash/atomic

 		--help
 		--json
 	--display
+	--keywords, -k


It would be nice to automatically expand the keywords.

I couldn't figure that out ... do you know how to?

Ok we can merge and I will look to add this.

rhatdan · 2016-12-01T17:48:30Z

docs/atomic-diff.1.md

 **--json**
  Output in the form of JSON.

+**-k** **--keywords**


Give a list of keywords available.

Man page should include list.

rhatdan · 2016-12-01T17:50:20Z

docs/atomic-diff.1.md

+
+Compare files by 'sha256digests' and 'time' between images 'foo1' and 'foo2'
+
+    atomic diff foo1 foo2 --keywords sha256digest time


I think the handling of multiple keywords does not work well. What happens if I do

atomic diff --keywords sha256digest time foo1 foo2

Does it work
Or if I had a container named time.

atomic diff --keywords sha256digest time foo1

@rhatdan

It only works if if the keywords are the last arguments. Verified this in argparse as well. Example:

[bbaude@bbaude atomic (diff_mtree)]$ sudo ./atomic diff -k sha256digest centos:latest 19f8d8b7147b atomic diff: argument -k/--keywords: invalid choice: 'centos:latest' (choose from 'all', 'link', 'nlink', 'mode', 'type', 'time', 'uid', 'gid', 'size', 'sha256digest') Try 'atomic diff --help' for more information.

^^ Fails. Keywords have to be last.

[bbaude@bbaude atomic (diff_mtree)]$ sudo ./atomic diff centos:latest 19f8d8b7147b -k sha256digest

Ideas and/or suggestions welcome.

Lets make it a comma separated list? Or do it the way we did for TOP?

-o [{time,stime,ppid,uid,gid,user,group}], --optional [{time,stime,ppid,uid,gid,user,group}]

FWIW using "--" between the keyword list and the image names also works, but that's not very easy to use/discover.

Yes, but not easy to understand for most.

Added the ability to use -k one or more times. One keyword per -k

Why not do the "," separated list?

-k time -k stime -k ppid

Versus

-k time,ktime,ppid

But I guess it is not that big a deal

Most people will just do -k all, or is this the default?

-all is the default. I thought we agreed to mimic the top usage which is one option per keyword. Want different?

Ok, I though atomic top did both, but if it use -o foo -o bar, then LGTM

jlebon · 2016-12-01T19:31:10Z

tests/integration/test_diff.sh

 # logic can be added to a "cleanup stack", by cascading function calls
 # within traps. See tests/integration/test_mount.sh for an example.
 trap teardown EXIT
+if [ -e ${GOMTREE} ]; then


Probably better to indicate a skip here by returning 77.

Also, if you want these tests to run on the Atomic Host platforms until gomtree is pulled in automatically, you can simply add to the YAML:

packages: - gomtree

@jlebon is that done with an exit 77 or elsewise?

Yes exactly. See e.g. test_verify.sh.

The previous algorithm for comparing files used python's dircmp and is considered to be a shallow comparision. This allowed distinctly small possibilities that two files being compared could be different but not caught. We now use go-mtree to do the comparison. This can emulate the shallow comparison we had before but we can also adding a sha256digest as part of the comparison using the new --keywords option. Also, made slight tweaks to gomtree functions in Atomic.util so we debug and influence the return of JSON data. This solves projectatomic#761

rhatdan · 2016-12-05T15:07:56Z

@rh-atomic-bot r+

rh-atomic-bot · 2016-12-05T15:07:58Z

📌 Commit 171cba2 has been approved by rhatdan

rh-atomic-bot · 2016-12-05T15:08:05Z

⌛ Testing commit 171cba2 with merge 1db4288...

rh-atomic-bot · 2016-12-05T15:23:34Z

☀️ Test successful - status-atomicjenkins
Approved by: rhatdan
Pushing 1db4288 to master...

twaugh reviewed Dec 1, 2016

View reviewed changes

baude force-pushed the diff_mtree branch from 543dc8c to c07b99d Compare December 1, 2016 17:00

rhatdan reviewed Dec 1, 2016

View reviewed changes

jlebon reviewed Dec 1, 2016

View reviewed changes

baude mentioned this pull request Dec 1, 2016

[merged] generate: specify storage for mounts #778

Closed

baude force-pushed the diff_mtree branch 3 times, most recently from c1c6641 to 7c4d8ea Compare December 5, 2016 14:54

baude force-pushed the diff_mtree branch from 7c4d8ea to 171cba2 Compare December 5, 2016 15:00

rh-atomic-bot changed the title ~~Atomic/diff.py: Use go-mtree for file comparisons~~ [merged] Atomic/diff.py: Use go-mtree for file comparisons Dec 5, 2016

rh-atomic-bot closed this in 1db4288 Dec 5, 2016


		Compare files by 'sha256digests' and 'time' between images 'foo1' and 'foo2'

		atomic diff foo1 foo2 --keywords sha256digest time

Conversation

baude commented Dec 1, 2016

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!

twaugh commented Dec 1, 2016

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!

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!

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!

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