nexedi / slapos

@bk @vpelletier @rafael @alain.takoudjou

mentioned in merge request !421 (merged)

This looks ok with (my understanding) of the spec, but if someone could confirm it still renders OK with json editor it would be nice.

I guess the UI generation doesn't do anything special for "format"

According to the spec this schema is correct.

About json editor: oneOf/anyOf adds uncertainty and user must choose which part of schema will be used. But in this case the choice has no sense - for all 3 variants the string field will be added. This behavior of the schema editor can be improved by filtering of inessential choices, but this does not solve the non informativeness of the error message ("The value matches none of "oneOf ").

May be regexp could be used inside the separate library for such kind of fields as a temporary solution? This does not solve the non informativeness of the error message, but will allow to change all such fields in a custom format. No changes of json editor is needed in this case (i hope).

Custom format hostname/ipv4/ipv6 could be added in future, it will allow to add custom error messages to the validator and UI.

Thanks, this is a great point.

https://json-schema.org/understanding-json-schema/reference/string.html#format

says:

However, custom formats may also be used, as long as the parties exchanging the JSON documents also exchange information about the custom format types.

We could invent a format that would mean "hostname, ipv4 or ipv6" (for example we could name this "internet-address"). We would loose the builtin validation, but if I understood correctly:

for https://lab.nexedi.com/bk/rjs_json_form we use tv4 which allows adding custom formats easily.
for python, jsonschema also allows it.

Also, I was not clear enough on my first message that I was not understanding why this suddenly becomes needed. We use to test against a "metaschema.json" that was slightly different from draft4 - our version removes format. "Why ?" is still mystery at this point.

$ diff -u <(curl --silent https://lab.nexedi.com/nexedi/slapos/raw/88394ddb0dd826d1eddafb8fd4348d86c968740a/slapos/test/metaschema.json | python -m json.tool ) <(curl --silent http://json-schema.org/draft-04/schema | python -m json.tool )
--- /dev/fd/63  2018-11-01 06:06:17.298678282 +0100
+++ /dev/fd/62  2018-11-01 06:06:17.298678282 +0100
@@ -55,7 +55,6 @@
     "id": "http://json-schema.org/draft-04/schema#",
     "properties": {
         "$schema": {
-            "format": "uri",
             "type": "string"
         },
         "additionalItems": {
@@ -123,8 +122,10 @@
             "default": false,
             "type": "boolean"
         },
+        "format": {
+            "type": "string"
+        },
         "id": {
-            "format": "uri",
             "type": "string"
         },
         "items": {

for https://lab.nexedi.com/bk/rjs_json_form we use tv4 which allows adding custom formats easily.

for python, jsonschema also allows it.

Yes, i think the same.

About the purposes of the removing format in metaschema i have no idea.

Continuing discussion from slapos.core!55 (comment 69648) where we were discussing a name for this format:

I read a bit the linked RFC from python urlparse doc:

RFC 1808 says:

...
      "//" net_loc ::= network location and login information, as per
                       Section 3.1 of RFC 1738 
...

Section 3.1 of RFC 1738 says:

    host
        The fully qualified domain name of a network host, or its IP
        address as a set of four decimal digit groups separated by
        ".". Fully qualified domain names take the form as described
        in Section 3.5 of RFC 1034 [13] and Section 2.1 of RFC 1123
        [5]: a sequence of domain labels separated by ".", each domain
        label starting and ending with an alphanumerical character and
        possibly also containing "-" characters. The rightmost domain
        label will never start with a digit, though, which
        syntactically distinguishes all domain names from the IP
        addresses.

which has this grammar:

host           = hostname | hostnumber
hostname       = *[ domainlabel "." ] toplabel
hostnumber     = digits "." digits "." digits "." digits

so according to this, it would be host but this RFC predates IPv6 and does not clarify if we are talking about an IPv6 with brackets or not ( 1:2::3 or [1:2::3] ), but it's obsoleted by RFC 3986, which still calls this part of the URL host:

      host        = IP-literal / IPv4address / reg-name

Out of context of URL, I feel host is too "vague".

While looking at this, I realized that if we use URI instead, we don't have the problems of having to create a custom format and finding a name for it, it would also solve the ambiguity of surrounding brackets or not for IPv6.

Here is the only case where we need this "extension", but we could just use a URI with nbd:// scheme and we don't need to add complexity. This would even more make sense in this context, because we have in this schema properties for nbd-host and nbd-port ...

Of course, this would mean updating the code of software release a bit to parse URLs and extract hostnames / port in two different properties, but this is easy to do in jinja template. There might also be a buildout recipe which exposes python urlparse to buildout.

[ edit nbd ndb typos ]

Hello, @jerome

as i see brackets are needed for an unambiguous determination of colon purpose:

splitting of host and port
or splitting inside ipv6.

So if two separate fields are used for host and port, then brackets can be omitted for ipv6. If our custom format is named "host" that doesn't mean we must use brackets for ipv6.

If url is used as format to specify nbd-host and nbd-port in one field then user has to type "nbd://" which doesn't have sense load.

If url format is used in other similar places in slapos then user will be able to type authorization data by mistake and this will be not secure.

I think it would be convenient to use one field for host and port without schema (nbd://, http://, ftp://, etc).

Url parser inside js doesn't distinguish port from host if nbd schema is used because it has no standard, but this doesn't matter.

Thanks @bk

I looked a bit more at where this is nbd option is used and apparently:

qemu already use nbd url https://qemu.weilnetz.de/doc/qemu-doc.html#disk_005fimages_005fnbd
when we create the nbd url from these parameters ( here ), we add surrounding [ and ]. Does it mean that in fact only ipv6 are allowed ? if that's the case, then we should change the schema's format to ipv6. @alain.takoudjou @rafael do you know if that's true that kvm's nbd accept only ipv6 hosts ?

In any case, I'm not planning to change this to nbd URL. I'm more interested in restoring this test without introducing too many changes.

I think there is a problem with the brackets surrounding the nbd_ip in the code you referenced because it can be also a hostname.

But it is beyond the scope of this MR to fix it. The change you did for KVM release is OK.

this big version up scares me a bit. @jm do you see anything bad with this ?

I think this is needed because jsonschema setup.py uses setuptools_scm

for reference, the error was

...
Caused installation of a distribution:
jsonschema 0.0.0
with a different version.
Got None.
Version and requirements information containing jsonschema:
     [versions] constraint on jsonschema: 3.0.0a3
     Requirement of slapos.toolbox==0.81: jsonschema
     Requirement of slapos.cookbook==1.0.75: jsonschema
While:
  Installing eggs.
Error: There is a version conflict.
We already have: jsonschema 0.0.0

Let's try.

seems ok for me... (let's try)

Thanks, let's try. Slapos egg test is already passing, ERP5 tests are now running with this slapos branch at https://nexedi.erp5.net/test_result_module/20181025-3752ADB9?

There's already something looking like a potential issue.With this change it's not possible to update software "in-place", error is (error is from my dev environment):

While:
  Installing.
  Loading extensions.

An internal error occurred due to a bug in either zc.buildout or in a
recipe being used:
Traceback (most recent call last):
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/buildout.py", line 2242, in main
    getattr(buildout, command)(args)
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/buildout.py", line 638, in install
    self._install_parts(install_args)
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/buildout.py", line 652, in _install_parts
    self._load_extensions()
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/buildout.py", line 1196, in _load_extensions
    newest=self.newest, allow_hosts=self._allow_hosts)
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/easy_install.py", line 1060, in install
    return installer.install(specs, working_set, patch_dict=patch_dict)
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/easy_install.py", line 821, in install
    self._maybe_add_setuptools(ws, dist)
  File "/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/zc.buildout-2.5.2+slapos012-py2.7.egg/zc/buildout/easy_install.py", line 775, in _maybe_add_setuptools
    if ws.find(requirement) is None:
  File "build/bdist.linux-x86_64/egg/pkg_resources/__init__.py", line 715, in find
    raise VersionConflict(dist, req)
VersionConflict: (setuptools 33.1.1 (/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c/eggs/setuptools-33.1.1-py2.7.egg), Requirement.parse('setuptools==40.4.3'))
 Failed to run buildout profile in directory '/srv/slapgrid/slappart8/srv/testnode/bpy/soft/e260097bd0be7f30ab42c073b787fa7c'
Finished software releases.

so when updating after this change, we have to either:

reinstall software completely
hack locally. Manually remove software/HASH/eggs/setuptools-33.1.1-py2.7.egg should probably fix

I think it's not really an issue, because we are not supposed to update software in place. Testnode is doing it, but after a few build failures it removes software folder and reinstall from scratch - as we can see, even if the first test run failed to build, on this one it builds.

I don't know about webrunner resilient, it does not update software in place, isn't it ? cc @Nicolas

If I understand correctly, buildout can update setuptools to use another setuptools than the one from python, but once buildout has installed a setuptools, it does not know how to update to a newer setuptools version.

I feel we can ignore this and just merge.

It will be a problem in this scenario in webrunner:

developer install a software before this change, using a "non versionned url", typically, from a git checkout in ~/srv/runner/project/slapos ( or an URL using a branch and not a tag/commit hash such as https://lab.nexedi.com/nexedi/slapos/raw/master/software/xx/software.cfg, which is definitively not supported)
software is updated ( developer pull changes in his working copy )
software runs again ( developer runs slapos node software )

This step 3 will fail, so developer has to remove software folder and install completely. I think this is not really an issue, developer should be able to manage this.

This is maybe a problem in resilient webrunner, but I don't know this part well. After step 1, software is installed on the clone, but do step 2 and 3 causes an build in-place on the clone ? if it causes a build and the build fail, is webrunner doing something clever like removing the software and re-installing from scratch ? ( @Nicolas @alain.takoudjou or maybe @luke know this ?)

Hi,

For resilient webrunner there is no such clever thing. Also, slapos node software is run everytime that import is run (by default once a day).

However, there exists an unofficial half-automated way, whose steps follow yours :

software fails, thus runner-import.sh exits with an error.
a promise checks the exit code, and fails if it is not OK. A ticket is raised in Vifib master.
Vifib team connects to runner1, check what happenned, and eventually delete by hand software/HASH/eggs/setuptools-33.1.1-py2.7.egg

If this can be automated, then your change isn't an issue.

Could we implement the same trick used by erp5testnode in the case of the resilient webrunner ? As the import should try as hard as possible to mirror what's in runner0.

Thanks for feedback @Nicolas

Could we implement the same trick used by erp5testnode in the case of the resilient webrunner ? As the import should try as hard as possible to mirror what's in runner0.

I think this would be the right thing to do. But this is an issue we are highlighting now, but this is not new and can happen for many reasons, especially if the software from runner0 is being modified by developer between each export/import.

Now, I guess we can merge this already, because it would fix a test failure. We can improve resilient later.

Vifib team, you'll have to handle the potential tickets this could cause. Is it OK for you to merge ?

added 100 commits

a945fa54...88394ddb - 94 commits from branch nexedi:master
1517cd95 - software/erp5: fix json schema formatting
5c851e28 - stack/slapos: version ↑ jsonschema 3.0.0a3
f0e9b104 - stack/slapos: version ↑ six 1.11.0
7ccc4126 - stack/slapos: version ↑ setuptools 40.4.3
ac3df2bf - test: suport Draft 3,4,6 & 7 of jsonschema
69593b36 - software/kvm: fix json schemas

Compare with previous version

I think it would be good if in all schemes used in slapos $schema property has the value https://lab.nexedi.com/nexedi/slapos/raw/master/slapos-metaschema.json instead of http://json-schema.org/draft-04/schema . File https://lab.nexedi.com/nexedi/slapos/raw/master/slapos-metaschema.json can be created by copying of http://json-schema.org/draft-07/schema with additional changes:

additionalProperties: False - this allows to catch incorrect references in schemes
format: enum - (instead of string) - this allows developer to select format from list of supported formats
etc

Such custom metaschema simplifies the future upgrade (draft8 provokes developers to create custom metaschema by reference to core.schema instead of copying).

It's not about current MR, it's about future.

Thanks @bk this seems a good idea.

Only draft8 would also extending core schema instead of copying ?

format: enum - (instead of string) - this allows developer to select format from list of supported formats

so apparently, before these changes, this was already possible because we already used a patched schema. In this MR I'm removing the schema because I saw jsonschema was providing it. I thought it was something new from jsonschema so I thought "cool, let's use it", but maybe I'm breaking something here.

At this point it's mystery to me if we were intentionally patching ( @rafael do you remember this ? @vpelletier maybe you know ?) The schema was introduced in 90c5c8dd . In any case, it's not good if the we use "$schema": "http://json-schema.org/draft-04/schema#", where in fact it means "use our custom schema similar to "http://json-schema.org/draft-04/schema#", so if we are already implicitly using a modified schema, making it explicit by using the "real" $schema definition is a good thing. But without understanding this part, I'm not sure of all what we are discussing here.

If using a custom schema prevents from upgrading jsonschema egg and all the dependencies, it's also easier here.

BTW, I don't know if you gave an absolute URL to explain your idea better, but when we use it, instead of https://lab.nexedi.com/nexedi/slapos/raw/master/slapos-metaschema.json we should use a URL relative to the software release json ../../slapos-metaschema.json ( if that's possible ), so that parameters schema refers to the current version of the meta schema. ( also maybe naming this "parameter-schema.cfg" - and renaming the current schema.json to software-release-schema.cfg ? )

Only draft8 would also extending core schema instead of copying ?

I hope that when draft8 is released it will support extended core schema instead of copying. If we extend core schema currently then it will allow us to add new properties only to the first level, because core schema contains references to itself.

format: enum - (instead of string) - this allows developer to select format from list of supported formats

so apparently, before these changes, this was already possible because we already used a patched schema.

As the patched schema is used implicitly then json schema editor knows nothing about the modified schema is used and developer will not get improvement.

If using a custom schema prevents from upgrading jsonschema egg and all the dependencies, it's also easier here.

The possibility of schema to be Boolean (draft6/dratf7) looks attractively, but unfortunately that possibility must be supported by validator's code. May be Nexedi's version of validator (in which the dependence of setuptools_scm is removed) can be used?

BTW, I don't know if you gave an absolute URL to explain your idea better, but when we use it, instead of https://lab.nexedi.com/nexedi/slapos/raw/master/slapos-metaschema.json we should use a URL relative to the software release json ../../slapos-metaschema.json ( if that's possible )

According to the spec URL must be absolute. But i think absolute URL will not cause the issues, because slapos-metaschema is used only to help developers to find misprints in his schemes and to render the UI to simplify schema edit by developers. In addition - our slapos-metaschemas changes are most likely backward compatible.

When testing the jsonschema validator used in slapos it looked like it didn't walk references inside schema while schema validating, is it true? If it's true the slapos-metaschema is not used for validating.

added 1 commit

9c4d1dd6 - fixup! software/jstestnode: add json for instance parameters

Compare with previous version

marked as a Work In Progress from jerome/slapos@9c4d1dd6

mentioned in merge request !429 (merged)

@tomo FYI, the work here should fix all current failures in slapos.cookbook

Thanks to @bk help, I think we have everything to finish this. I suggest we do:

use format="internet-address" in !425 (diffs) ( better name anyone ?) as a first step without validation of format.
change jstestnode schema (by including jerome/slapos@3ed2e7fe and bk/slapos@3258c8fe on top of this branch) and update jstestnode implementation accordingly, then update the json on test suites in Nexedi ERP5.
update jsonschema and setuptools eggs in slapos. As we said, this might cause some problems when software in resilient webrunners are updated, but if it's really a problem, we can probably fix webrunner so that it supports this case, by doing same as testnode, removing all software and retry from scratch ( @Nicolas pointed out that relevant code is here, but it might be ticky to not remove the full software if webrunner lose internet for 1 hour )

At the same time you update setuptools, can you also update slapos.libnetworkcache ? This will avoid too much rebuilding.

added 5 commits

60a353dd - stack/slapos: version ↑ slapos.libnetworkcache 0.16
a375d34b - test: suport Draft 3,4,6 & 7 of jsonschema
fb299862 - software/erp5: fix schema definition of test-runner key
9e4c6f8d - software/kvm: fix json schemas
d2f37999 - software/jstestnode: move selenium configuration in test-runner key

Compare with previous version

At the same time you update setuptools, can you also update slapos.libnetworkcache ? This will avoid too much rebuilding.

Good idea, done.

The items from !425 (comment 69459) are also all done. I'm waiting for test results. I'm planning to check slapos.cookbook and jstestnode tests.

mentioned in merge request slapos.core!55 (merged)

For future reference: Because I rebased and pushed force in the branch, an interesting discussion where @bk explains explains how to improve jstestnode json schema and give general tips on how to make a schema with "conditional branches" ( on https://codepen.io/bkrocherov/pen/MzbeRx ) is no longer visible from the discussion tab of this MR. The full discussion is still at 9c4d1dd6 (comment 68970)

added 1 commit

39843233 - software/jstestnode: move selenium configuration in test-runner key

Compare with previous version

unmarked as a Work In Progress

Tests for jstestnode (on selenium server) and slapos.toolbox are both OK. To me this can be merged;

@rafael @tomo if no remaining concerns on this I'm thinking about merging next monday morning.

( PS: there was a question about kvm nbd !425 (comment 69718) )

Go ahead, merge this as soon as possible as we will release a new version of KVM soon.

Thanks @tomo I will not merge this today, because I'm not available tomorrow (and not much this afternoon) in case something goes bad.

Anyway, I need to rebase and apply the same fix on the new format: [..] introduced on 49433038, because this won't pass test the new way of testing json schemas.

added 73 commits

39843233...5486d8c6 - 64 commits from branch nexedi:master
158d9e54 - software/erp5: fix json schema formatting
b3394953 - stack/slapos: version ↑ jsonschema 3.0.0a3
70f88d2c - stack/slapos: version ↑ six 1.11.0
0dec8251 - stack/slapos: version ↑ setuptools 40.4.3
0272cb21 - stack/slapos: version ↑ slapos.libnetworkcache 0.16
ff6cd56f - test: suport Draft 3,4,6 & 7 of jsonschema
0529cb73 - software/erp5: fix schema definition of test-runner key
f1ffe8f0 - software/kvm: fix json schemas
90be6120 - software/jstestnode: move selenium configuration in test-runner key

Compare with previous version

I confirmed that after rebase this makes slapos.toolbox pass again (which also means that it compiles successfully), let's merge this.

merged

mentioned in commit 0d302559

mentioned in merge request !488 (merged)

mentioned in commit b2dcbd2f

       "description": "hostname (or IP) of the NBD server containing the boot image.",
       "type": "string",
       "format": [
         "host-name",
         "ip-address",
         "ipv6"
       "oneOf": [
+        {
           "format": "hostname"
         },
+        {
           "format": "ipv4"
         },
+        {
           "format": "ipv6"
+        }

Update to jsonschema to support Draft 7 and fix some invalid json

Revert this merge request

Cherry-pick this merge request