Redmine#7871: use VarRefValueToJson consistently #2476

tzz · 2016-01-19T17:31:58Z

VarRefValueToJson() was no longer used so VarRefValueToJsonAllowScalars() was renamed to it
fixes a bug in VarRefValueToJsonAllowScalars() that leaked cf_nulls (typo: used value instead of rp)
fixes a bug in SeqShuffle() so it doesn't crash with a 0-length sequence
for https://dev.cfengine.com/issues/7871 the following functions now take a list or an array or a data container, or inline JSON: filter(), getindices(), getvalues(), join(), length(), maplist(), reverse(), unique(), intersection(), difference(), shuffle(), sort(), storejson(), string_mustache(), sublist(), sum(), product()
all the rewritten function got new acceptance tests using bundle state / expected JSON, which is much much much nicer than the old tests. I tried to add cases wherever possible. @nickanderson feel free to review these. Some tests were redundant and thus removed.
the following functions were left alone because they're either not needed or too tricky due to legacy issues: regarray(), reglist(), makerule(), nth()
we only create copies of the variables if needed, and free them only in this case. I hope that this will improve performance.

The bulk of this PR is new tests. I did it thus: 1) make sure the old test works, 2) convert the old test to expected JSON style, 3) when the converted test works fine, add at least one inline JSON test to verify operation, and usually even more cases. The rest of the C code changes are pretty small and remove a significant amount of non-standard code in favor of a uniform data reference interface.

We may have performance concerns about creating and destroying containers on every call. I don't think it's a significant issue in normal usage, but we can optimize it after the merge (correctness before premature optimization). I added a second commit that optimizes the common case of needing a read-only reference to a JsonElement from the variable table. Note that second commit also fixes some missing places where we should have been destroying JsonElements.

All the acceptance tests passed for me.

I can break the first two bugfixes into separate commits if you prefer.

tzz · 2016-01-23T22:17:22Z

I added a commit that copies and frees JsonElements only when needed. It also fixes some places where we were not freeing references on function exit. All the acceptance tests pass for me, but please let me know if it needs adjustment.

kacf · 2016-02-09T08:46:20Z

I think there should be a changelog entry for all the functions that now accept any kind of container.

This PR is big though! I got through some of it, but I need to continue this later. I had to make the comments on each commit because the diff is too big for github to show in the PR itself. For this reason I ask that you don't update the branch just yet, I'd like to pick up in the same place. I'll let you know when I'm done.

tzz · 2016-02-09T15:27:51Z

I can list the functions in the cleanup stage after the code is finished. Do you need the list sooner?

I made the changes I acknowledged on your comments so far in my local copy, and will rebase and resubmit when you give the go-ahead.

Thank you for reviewing. I think this PR, besides helping performance, will substantially improve CFEngine functions and data containers. I'm sorry it's big but the behavior is best introduced all at once, in my opinion.

kacf · 2016-02-11T08:58:28Z

I can list the functions in the cleanup stage after the code is finished. Do you need the list sooner?

No, that's ok, we can take care of that at the end.

I made the changes I acknowledged on your comments so far in my local copy, and will rebase and resubmit when you give the go-ahead.

Good, I'm still working on it.

I'm sorry it's big but the behavior is best introduced all at once, in my opinion.

Oh, certainly, I'm not arguing with that! :-)

kacf · 2016-02-11T09:44:05Z

Alright, I have looked at all the code, looks good to me apart from the comments.

The tests I only took a very brief look at, it's quite extensive. I assume that if you run the new tests with the old code, they will fail now because they will attempt to use JSON? Otherwise it would be a good confirmation that you did the test conversion correctly.

tzz · 2016-02-13T14:53:31Z

Yes, the new tests fail against the old code.

tzz · 2016-02-13T15:22:17Z

OK, I have made the adjustment for multiple array indices so the test from #2492 passes now. It was a one-line fix. All the other acceptance tests pass too. This is rebased, squashed, and ready for final review.

The list of modified functions is at the top of this PR.

kacf · 2016-02-16T08:31:42Z

Thanks for updating. One last thing, can you put the entire block of information in the top of the PR in the commit message (apart from the last sentence, perhaps). It should be there for future reading.

kacf · 2016-02-16T08:31:48Z

trigger build

tzz · 2016-02-16T12:32:23Z

I updated the commit message as suggested, I hope that's what you were looking for.

kacf · 2016-02-17T09:14:03Z

Thanks! There a few tests that fail with this though. 01_vars/02_functions/getindices.cf appears to fail everywhere, this is an example from RHEL7:

lt-cf-promises: var_expressions.c:316: VarRefParseFromNamespaceAndScope: Assertion `ns == ((void *)0) && "A variable missing a scope should not have a namespace"" failed.
lt-cf-agent: var_expressions.c:316: VarRefParseFromNamespaceAndScope: Assertion `ns == ((void *)0) && "A variable missing a scope should not have a namespace"" failed.
/home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/core/tests/acceptance/workdir/__01_vars_02_functions_getindices_cf/runtest: line 8: 26066 Aborted                 (core dumped) "/home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/core/tests/acceptance/../../../core/cf-agent/cf-agent" -Kf "./01_vars/02_functions/getindices.cf" -D AUTO,DEBUG

In addition, 01_vars/02_functions/shuffle.cf fails on all non-Linux platforms. From Solaris 10 Sparc:

R: FILES DIFFER BUT SHOULD BE THE SAME
R: CONTENTS OF /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_ULTRASPARC_SOLARIS_10/core/tests/acceptance/workdir/__01_vars_02_functions_shuffle_cf/tmp/TESTDIR.cfengine/actual:
{
  "lists": [
    "a",
    "b"
  ],
  "seeds": [
    "skruf",
    "cormorant",
    "dollhouse"
  ],
  "shuffle_a_cormorant": [
    "e",
    "f",
    "d",
    "g",
    "a",
    "b",
    "c"
  ],
  "shuffle_a_dollhouse": [
    "c",
    "a",
    "g",
    "f",
    "e",
    "b",
    "d"
  ],
  "shuffle_a_skruf": [
    "g",
    "a",
    "f",
    "e",
    "b",
    "c",
    "d"
  ],
  "shuffle_b_cormorant": [
    "cf_null"
  ],
  "shuffle_b_dollhouse": [
    "cf_null"
  ],
  "shuffle_b_skruf": [
    "cf_null"
  ],
  "shuffle_inline_cormorant": [
    "farmer",
    "delta",
    "b",
    "a"
  ],
  "shuffle_inline_dollhouse": [
    "delta",
    "a",
    "b",
    "farmer"
  ],
  "shuffle_inline_skruf": [
    "b",
    "a",
    "delta",
    "farmer"
  ]
}

R: CONTENTS OF /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_ULTRASPARC_SOLARIS_10/core/tests/acceptance/./01_vars/02_functions/shuffle.cf.expected.json:
{
  "lists": [
    "a",
    "b"
  ],
  "seeds": [
    "skruf",
    "cormorant",
    "dollhouse"
  ],
  "shuffle_a_cormorant": [
    "e",
    "d",
    "b",
    "a",
    "c",
    "g",
    "f"
  ],
  "shuffle_a_dollhouse": [
    "b",
    "f",
    "a",
    "e",
    "g",
    "c",
    "d"
  ],
  "shuffle_a_skruf": [
    "f",
    "d",
    "b",
    "e",
    "c",
    "g",
    "a"
  ],
  "shuffle_b_cormorant": [
    "cf_null"
  ],
  "shuffle_b_dollhouse": [
    "cf_null"
  ],
  "shuffle_b_skruf": [
    "cf_null"
  ],
  "shuffle_inline_cormorant": [
    "delta",
    "a",
    "farmer",
    "b"
  ],
  "shuffle_inline_dollhouse": [
    "delta",
    "farmer",
    "a",
    "b"
  ],
  "shuffle_inline_skruf": [
    "b",
    "a",
    "delta",
    "farmer"
  ]
}

R: --- /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_ULTRASPARC_SOLARIS_10/core/tests/acceptance/./01_vars/02_functions/shuffle.cf.expected.json    Tue Feb 16 00:36:39 2016
+++ /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_ULTRASPARC_SOLARIS_10/core/tests/acceptance/workdir/__01_vars_02_functions_shuffle_cf/tmp/TESTDIR.cfengine/actual Tue Feb 16 00:51:42 2016
@@ -10,30 +10,30 @@
   ],
   "shuffle_a_cormorant": [
     "e",
+    "f",
     "d",
-    "b",
-    "a",
-    "c",
     "g",
-    "f"
+    "a",
+    "b",
+    "c"
   ],
   "shuffle_a_dollhouse": [
-    "b",
-    "f",
+    "c",
     "a",
-    "e",
     "g",
-    "c",
+    "f",
+    "e",
+    "b",
     "d"
   ],
   "shuffle_a_skruf": [
+    "g",
+    "a",
     "f",
-    "d",
-    "b",
     "e",
+    "b",
     "c",
-    "g",
-    "a"
+    "d"
   ],
   "shuffle_b_cormorant": [
     "cf_null"
@@ -45,16 +45,16 @@
     "cf_null"
   ],
   "shuffle_inline_cormorant": [
-    "delta",
-    "a",
     "farmer",
-    "b"
+    "delta",
+    "b",
+    "a"
   ],
   "shuffle_inline_dollhouse": [
     "delta",
-    "farmer",
     "a",
-    "b"
+    "b",
+    "farmer"
   ],
   "shuffle_inline_skruf": [
     "b",
R: /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_ULTRASPARC_SOLARIS_10/core/tests/acceptance/./01_vars/02_functions/shuffle.cf FAIL

It looks like only the order has changed, not the content. I vaguely recall us talking about canonical JSON at some point, where all JSON keys would be sorted. But maybe that doesn't apply to lists? I'm puzzled why this only fails on non-Linux platforms. Any idea, just looking at the test case?

And finally 10_files/13_file_dir/001.cf.001.cf failed on HP-UX, but I think you can ignore it for now, I have seen that test failing spuriously before. We can see how it goes in the next round.

tzz · 2016-02-17T14:27:03Z

@kacf the getindices() test was failing due to an over-aggressive assert. Now that we allow inline JSON, it's possible that we have temporary variables without a namespace or scope. The assert didn't add safety to the code in my opinion, so I've removed it from var_expressions.c and I hope you agree.

tzz · 2016-02-17T14:38:35Z

The shuffle() test was reverted because the exact results currently depend on the outcome of srand() and rand() which is platform-dependent. I moved the exact test to 01_vars/02_functions/staging/shuffle-exact.cf and opened ticket https://dev.cfengine.com/issues/7950 for it.

kacf · 2016-02-18T06:54:00Z

Don't use staging anymore, we want to move away from that structure. Use test_soft_fail instead and use !linux as condition.

I guess the fact that you need scopeless and namespaceless variables is proof that it works, and hence the checks are not needed. Looks good to me.

kacf · 2016-02-18T06:54:05Z

trigger build

kacf · 2016-02-18T14:18:47Z

FYI, your beast baby, compliance/great_expectations.cf is now failing in enterprise. :-)

Do you have access to that one?

This is the output from RHEL7:

warning: Warning promised, need to create file "/do/not/exist"
   error: Unable to make file belong to an unknown group
   error: Unable to make file belong to an unknown user
   error: Unable to create link "/home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.symlink" -> "./$(etc.passwd)", no source
   error: Unable to create link "/home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.absolute" -> "/$(etc.passwd)", no source
   error: Unable to create link "/home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.relative" -> "./../../../../../../../../../../../../$(etc.passwd)", no source
   error: Could not get list of installed packages
   error: Unable to obtain a list of installed packages - aborting
   error: Finished command related to promiser "notrepairedpackage" -- an error occurred, returned 1
   error: Package schedule execution failed for "notrepairedpackage"
   error: Unable to make file belong to an unknown group
   error: Unable to make file belong to an unknown user
   error: Method "single_file_notkept" failed in some repairs
   error: Could not get list of installed packages
   error: Unable to obtain a list of installed packages - aborting
   error: Method "single_package_notkept2" failed in some repairs
   error: Method "single_package_notkept" failed in some repairs
   error: Proposed executable file "/x/y/z/no/such/script" doesn"t exist
   error: Process promise to stop "great_expectations" could not be kept because "/x/y/z/no/such/script" the stop operator failed
   error: Command related to promiser "/bin/false" returned code defined as promise failed 1
   error: Method "test_services_variable" failed in some repairs
   error: Proposed executable file "/no/idea" doesn"t exist
   error: "/no/idea" promises to be executable but isn"t
   error: Method "test_services" failed in some repairs
   error: Proposed executable file "//home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.willfail" doesn"t exist
   error: "//home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.willfail" promises to be executable but isn"t
   error: Finished command related to promiser "/bin/false" -- an error occurred, returned 1
  notice: Q: ".../bin/echo works": works
 warning: Command "/bin/echo warns" needs to be executed, but only warning was promised
   error: Proposed executable file "//home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.willwarn" doesn"t exist
   error: "//home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/workdir/__compliance_great_expectations_cf/tmp/TEST.cfengine.willwarn" promises to be executable but isn"t
R: repaired
   error: Method "test" failed in some repairs
R: /home/jenkins/workspace/testing-enterprise-pr_core/label/PACKAGES_x86_64_linux_redhat_7/enterprise/tests/acceptance/./compliance/great_expectations.cf FAIL
   error: Method "test_run" failed in some repairs

…eeded VarRefValueToJson() was no longer used so VarRefValueToJsonAllowScalars() was renamed to it. Fixes a bug in VarRefValueToJsonAllowScalars() that leaked cf_nulls (typo: used value instead of rp). Fixes a bug in SeqShuffle() so it doesn't crash with a 0-length sequence. For https://dev.cfengine.com/issues/7871 the following functions now take a list or an array or a data container, or inline JSON: filter(), getindices(), getvalues(), join(), length(), maplist(), reverse(), unique(), intersection(), difference(), shuffle(), sort(), storejson(), string_mustache(), sublist(), sum(), product(); all the rewritten function got new acceptance tests. Some tests were redundant and thus removed. The following functions were left alone because they're either not needed or too tricky due to legacy issues: regarray(), reglist(), makerule(), nth() We only create copies of the variables if needed, and free them only in that case, which will improve performance. Changelog: Allow inline JSON to be used in function calls (Redmine#7871).

tzz · 2016-02-18T15:58:50Z

@kacf OK, I moved the test out of staging and gave it the soft fail tag as you requested.

I have no idea why compliance/great_expectations.cf would fail with this change so I'll need to investigate. It should probably be refactored to use the newer functions and bundlestate() anyhow. I haven't build Enterprise (though I have access to it) in a while so I'll need some time. Meanwhile, can you make sure it fails only because of my change, and if so, perhaps trace the problem a bit further?

tzz · 2016-02-18T16:02:07Z

Regarding the scopeless thing, what actually happens in getindices() is that we try to look up the variable { "foo": 1 } for instance. It's not a variable, of course, but the asserts don't like it before we check the variable table...

Normally that lookup is not necessary because we try to parse as inline JSON as soon as possible, but getindices() because of the hack to implement the broken behavior does it... so ugly!

kacf · 2016-02-19T08:04:31Z

Meanwhile, can you make sure it fails only because of my change, and if so, perhaps trace the problem a bit further?

I have to admit that I barely understand this test myself, if so only at a conceptual level. Digging into it would be time consuming for me. But you wrote it back in the day, didn't you? I'm hoping you still remember it well enough to spot the problem without too much trouble. :-) If you have any problems building enterprise though, just let me know!

The test fails across the board on all platforms, and never fails anywhere else (it's not one of those timing dependent "may-sometimes-fail-for-no-reason" cases), so it's definitely being triggered by your changes.

kacf · 2016-02-19T08:08:40Z

Regarding the scopeless thing, what actually happens in getindices() is that we try to look up the variable { "foo": 1 } for instance. It's not a variable, of course, but the asserts don't like it before we check the variable table...

Normally that lookup is not necessary because we try to parse as inline JSON as soon as possible, but getindices() because of the hack to implement the broken behavior does it... so ugly!

OTOH I also see it as a natural development: We now place different demands on the contents of the variable table compared to data we parse from function parameters, so it's only natural the asserted invariants need to change over time. But yeah, it probably could be prettier!

tzz · 2016-02-19T10:36:39Z

Your great expectations are my command: see https://github.com/cfengine/enterprise/pull/298 for the updated test that works for me.

* inline-json: Redmine#7871: use VarRefValueToJson consistently and copy only when needed

kacf · 2016-02-25T08:08:51Z

Merged manually. Thanks for the great work Ted, and for updating the incredibly complex Enterprise test!

tzz · 2016-02-26T15:05:04Z

The remaining work will be discussed https://dev.cfengine.com/issues/7871

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch from 93aa2b2 to 2f1b5b5 Compare January 23, 2016 22:13

tzz mentioned this pull request Jan 31, 2016

Parse /proc/net/route and allow dumping all JSON vars #2245

Closed

kacf self-assigned this Feb 9, 2016

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch 3 times, most recently from ca15351 to f7a82b4 Compare February 13, 2016 15:17

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch from f7a82b4 to e291e1a Compare February 15, 2016 11:20

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch from e291e1a to 18423a4 Compare February 16, 2016 12:31

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch from 18423a4 to 81cda98 Compare February 17, 2016 14:25

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch 2 times, most recently from 5825886 to d1e2fbd Compare February 17, 2016 14:38

tzz force-pushed the feature/redmine7871-inline-json-everywhere branch from d1e2fbd to 5916b28 Compare February 18, 2016 15:55

kacf added a commit that referenced this pull request Feb 25, 2016

Merge branch 'inline-json' (pull request #2476)

3c1fbab

* inline-json: Redmine#7871: use VarRefValueToJson consistently and copy only when needed

kacf closed this Feb 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redmine#7871: use VarRefValueToJson consistently #2476

Redmine#7871: use VarRefValueToJson consistently #2476

tzz commented Jan 19, 2016

tzz commented Jan 23, 2016

kacf commented Feb 9, 2016

tzz commented Feb 9, 2016

kacf commented Feb 11, 2016

kacf commented Feb 11, 2016

tzz commented Feb 13, 2016

tzz commented Feb 13, 2016

kacf commented Feb 16, 2016

kacf commented Feb 16, 2016

tzz commented Feb 16, 2016

kacf commented Feb 17, 2016

tzz commented Feb 17, 2016

tzz commented Feb 17, 2016

kacf commented Feb 18, 2016

kacf commented Feb 18, 2016

kacf commented Feb 18, 2016

tzz commented Feb 18, 2016

tzz commented Feb 18, 2016

kacf commented Feb 19, 2016

kacf commented Feb 19, 2016

tzz commented Feb 19, 2016

kacf commented Feb 25, 2016

tzz commented Feb 26, 2016

Redmine#7871: use VarRefValueToJson consistently #2476

Redmine#7871: use VarRefValueToJson consistently #2476

Conversation

tzz commented Jan 19, 2016

tzz commented Jan 23, 2016

kacf commented Feb 9, 2016

tzz commented Feb 9, 2016

kacf commented Feb 11, 2016

kacf commented Feb 11, 2016

tzz commented Feb 13, 2016

tzz commented Feb 13, 2016

kacf commented Feb 16, 2016

kacf commented Feb 16, 2016

tzz commented Feb 16, 2016

kacf commented Feb 17, 2016

tzz commented Feb 17, 2016

tzz commented Feb 17, 2016

kacf commented Feb 18, 2016

kacf commented Feb 18, 2016

kacf commented Feb 18, 2016

tzz commented Feb 18, 2016

tzz commented Feb 18, 2016

kacf commented Feb 19, 2016

kacf commented Feb 19, 2016

tzz commented Feb 19, 2016

kacf commented Feb 25, 2016

tzz commented Feb 26, 2016