With nexedi/neoppod@fd80cc30 NEO/py added support to encode the compression
algorithm with the 'Compression' parameter. Before this, compression could
only be true (= with compression) or false (= without compression). Now
the absence of compression is encoded with 0. Any other number than 0
encodes a compression algorithm. The mapping is currently:

	1 = zlib

In the future, 2 could mean zstd [1].

[1] https://github.com/facebook/zstd/issues/1134

In pre-msgpack protocol an 'INVALID_{TID,OID}' was always
decoded as 'None' in NEO/py [1]. But in msgpack protocol
this isn't true anymore. An `INVALID_TID` is now decoded
as an `INVALID_TID`. And this then leads to errors later [2].
We fix this by encoding 'INVALID_{TID,OID}' to NIL on the
wire and by decoding NIL to 'INVALID_{TID,OID}'.

---

[1] https://lab.nexedi.com/nexedi/neoppod/-/blob/6332112cba979dfd29b40fe9f98d097911fde696/neo/lib/protocol.py#L579-583
[2] With SQLite backend we can see the following exception:

Traceback (most recent call last):
  File "/home/levin/neo/neo/tests/functional/__init__.py", line 192, in start
    self.run()
  File "/home/levin/neo/neo/tests/functional/__init__.py", line 288, in run
    getattr(neo.scripts,  self.command).main()
  File "/home/levin/neo/neo/scripts/neostorage.py", line 32, in main
    app.run()
  File "/home/levin/neo/neo/storage/app.py", line 196, in run
    self._run()
  File "/home/levin/neo/neo/storage/app.py", line 227, in _run
    self.doOperation()
  File "/home/levin/neo/neo/storage/app.py", line 301, in doOperation
    poll()
  File "/home/levin/neo/neo/storage/app.py", line 145, in _poll
    self.em.poll(1)
  File "/home/levin/neo/neo/lib/event.py", line 160, in poll
    to_process.process()
  File "/home/levin/neo/neo/lib/connection.py", line 508, in process
    self._handlers.handle(self, self._queue.pop(0))
  File "/home/levin/neo/neo/lib/connection.py", line 93, in handle
    self._handle(connection, packet)
  File "/home/levin/neo/neo/lib/connection.py", line 108, in _handle
    pending[0][1].packetReceived(connection, packet)
  File "/home/levin/neo/neo/lib/handler.py", line 125, in packetReceived
    self.dispatch(*args)
  File "/home/levin/neo/neo/lib/handler.py", line 75, in dispatch
    method(conn, *args, **kw)
  File "/home/levin/neo/neo/storage/handlers/client.py", line 67, in askObject
    o = app.dm.getObject(oid, at, before)
  File "/home/levin/neo/neo/storage/database/manager.py", line 484, in getObject
    before_tid and u64(before_tid))
  File "/home/levin/neo/neo/storage/database/sqlite.py", line 336, in _getObject
    r = q(sql + ' AND tid=?', (partition, oid, tid))
OverflowError: Python int too large to convert to SQLite INTEGER

Some NEO protocol packets have the field 'RowList'. This field contains
information about each row of a partition table. In NEO/go the information
of each row is represented with the 'RowInfo' type [1]. This type is defined
as a struct with the field ‘CellList’. ‘CellList’ is defined as a list of
'CellInfo' [1] (e.g. an entry for each cell).

NEO/go {en,de}codes struct types with ‘genStructHead’ (structures in
golang are encoded as arrays in msgpack) [2]. From the 'RowList'
definition, the msgpack decoder currently expects the following msgpack
array structure:

    ArrayHeader (RowList)

        ArrayHeader (RowInfo)

            ArrayHeader (CellList)

                ArrayHeader (CellInfo)

                    int32 (NID)
                    enum (State)

However NEO/py actually sends:

    ArrayHeader (RowList)

        ArrayHeader (CellList)

            ArrayHeader (CellInfo)

                int32 (NID)
                enum (State)

In other words, currently the NEO/go msgpack encoder expects one more
nesting, which NEO/py doesn’t provide (and which also doesn’t seem to
be necessary as the outer nesting would always only contain one
element). We could adjust the msgpack {en,de}coder to introduce an
exception for the 'RowInfo' type, however as the protocol definition in
'proto.go' aims to transparently reflect the structure of the packets on
the wire, it seems to be more appropriate to fix this straight in the
protocol definition. This is also less error-prone as we don't have to
fix all the different positions of the encoder, decoder & sizer and it's
less code (particularly if 'RowInfo' doesn't stay the only case for such
an issue).

[1] https://lab.nexedi.com/kirr/neo/-/blob/1ad088c8/go/neo/proto/proto.go#L391-394
[2] https://lab.nexedi.com/kirr/neo/-/blob/1ad088c8/go/neo/proto/protogen.go#L1770-1775

The data type of IdTime is 'Optional[float]' [1], [2]. Before this patch the
msgpack decoder could only decode 'IdTime' in case its data type was
'float'. Now it also supports the decoding of IdTime in case it is NIL.

[1] See here, the fifth argument is IdTime:

    https://lab.nexedi.com/nexedi/neoppod/-/blob/3ddb6663/neo/master/handlers/identification.py#L26-27

    This is found to be 'Optional[float]':

    https://lab.nexedi.com/nexedi/neoppod/-/blob/3ddb6663/neo/tests/protocol#L98

[2] This seems to be true for both, pre-msgpack and post-msgpack
    protocol, because in the pre-msgpack NEO/go there is already this
    note:

    https://lab.nexedi.com/kirr/neo/-/blob/1ad088c8/go/neo/proto/proto.go#L352-357

In NEO/go protocol, we describe some parameters that can
be send via NEO packages with structures. On the wire all of
these structures are encoded as msgpack arrays [1]. Msgpack
arrays can have 1 or more bytes as a header [2]. Therefore
it's better to use "ReadArrayHeaderBytes" than using

  >>> data = data[1:]

which fails in case we have array with 3 or 5 header bytes.

[1] One could also declare a protocol where these parameters aren't
    send as msgpack arrays but as msgpack maps. See here for the
    msgpack map specification:

    https://github.com/msgpack/msgpack/blob/9aa092d6ca81/spec.md?plain=1#L338

[2] https://github.com/msgpack/msgpack/blob/9aa092d6ca81/spec.md?plain=1#L315

In pre-msgpack protocol, the msgcode increment logic is like this:

Notify 	 add +1 to next msgcode
Request  add +0 to next msgcode   (next msg is answer & should therefore be the same)
Answer 	 add +1 to next msgcode

('Answer' msgcode is adjusted by 'AnswerBit')

In post-msgpack protocol, the logic is a bit different:

Notify 	 add +1 to next msgcode
Request  add +0 to next msgcode   (next msg is answer & should therefore be the same)
Answer 	 add +2 to next msgcode

So here we produce gaps after Request/Answer pairs.

The enum order has changed in the migration to the msgpack encoded
protocol.

See here for pre-msgpack order:

https://lab.nexedi.com/nexedi/neoppod/-/blob/6332112cb/neo/lib/protocol.py#L62-141

And here for post-msgpack order:

https://lab.nexedi.com/nexedi/neoppod/-/blob/9d0bf97a1327182ac29e95d65fd9e18742c43d1f/neo/lib/protocol.py#L102-181

As this order defines the encoding of the enum values, this needs to be
strictly followed to be compatible (to follow the protocol declaration).

This fixes the basic NEO handshake in msgpack encoding.

Because 'err' was locally assigned inside the loop of 'DialLink' [1],
it was always 'nil' as a function return value, even when
'handshakeClient' actually returned an error. This lead to the
unfortunate situation that the function sometimes returned 'link=nil'
and 'err=nil', so that the function caller tried to access 'link'
attributes which lead to 'runtime error: invalid memory address or nil
pointer dereference'. This patch fixes this and now the function
correctly returns an error if the dialing fails.

[1] `peerConn, err := networker.Dial(ctx, addr)`

With the switch to the msgpack encoding, the NEO/py server
now only supports the M encoding.

Not only for performance reasons (at least 3% faster) but also because of
several ugly things in the way packets were defined:
- packet field names, which are only documentary; for roots fields,
  they even just duplicate the packet names
- a lot of repetitions for packet names, and even confusion between the name
  of the packet definition and the name of the actual notify/request packet
- the need to implement field types for anything, like PByte to support new
  compression formats, since PBoolean is not enough

neo/lib/protocol.py is now much smaller.

* master:
  go/zodb: Handle common options in zurl in generic layer

/reviewed-by @kirr
/reviewed-on kirr/neo!4

* kirr/t+new-uri:
  Revert "Y client: Adjust URI scheme to move client-specific options to fragment"
  fixup! client.go: Fix URI client option parsing for supported + unsupported options
  client.go: Fix URI client option parsing for supported + unsupported options
  fixup! client_test: Add tests for NEO URI parser
  client_test: Add tests for NEO URI parser
  fixup! client: Refactor openClientByURL for easier testing
  client: Refactor openClientByURL for easier testing
  Y go/zodb: Handle common options in zurl in generic layer

Offload drivers from handling options such as ?read-only=1 and force
them to deal with such options only via DriverOptions, never zurl.

See added comment for details.

/reviewed-by @levin.zimmermann
/reviewed-on !4

This reverts commit kirr/neo@4c9414ea.
This patch was added at a time when nexedi/neoppod!18 wasn't
resolved yet, but we already wanted to proceed with WCFS. Now the NEO MR
is resolved and we decided to mostly leave the NEO zurl as it was
originally implemented in nexedi/neoppod!6.
This means we don't need this patch anymore which changed the NEO
zurl format.

readonly is handled by common zodb.OpenDriver.

Before this patch, the parser ignored options which were already supported
by the client (for instance 'read-only') and even raised an error. But the
client can already use this option: as a9246333 describes this should happen in the
local storage URL parser.

Furthermore not-yet-supported client options (for instance compress) broke
the NEO client before this patch. Now these options only raise a warning which
informs the user that they are ignored. Why? We want to use pre-complete NEO
in real-world projects together with NEO/py clusters. Those real-world projects
may already specify options which aren't supported by our NEO/go client yet.
But it doesn't matters so much, because those options are mostly
relevant for other NEO/py cluster clients (e.g. zope nodes). Instead of
filtering those parameters before parsing them to NEO/go in a higher
level (e.g. SlapOS), NEO/go should already support any valid NEO URL
and raise warnings for not yet implemented features.

- use simplified parseURL signature - DriverOptions are not passed nor changed there.
- read-only is handled by generic zodb layer not neo.parseURL .

This test was missing so far. Particularly recent changes of the
NEO URI scheme [1], but also problems with valid old URI [2] stressed
out the necessity for comprehensive NEO URI parser tests.

[1] kirr/neo@4c9414ea
[2] 573514c6 (comment 184417)

- no need to pass DriverOptions into parseURL - it is only zurl that is
  parsed, and also DriverOptions should not be changed by the opener.

- no need to document "If anything fails within this process an error
  and nil are returned." because that is standard omnipresent Go convention.

With all the recent changes of the NEO URI scheme we need to reliably test
the function which parses the URI and convert it into the different
parameter. Testing is much simpler if we can only analyse how the URI
parsing works. Therefore this patch moves NEO URI parsing to an
external function.

Offload drivers from handling options such as ?read-only=1 and force
them to deal with such options only via DriverOptions, never zurl.

See added commend for details.

I missed the following build failure in go/neo/cmd:

    # lab.nexedi.com/kirr/neo/go/neo/cmd/neo
    ./storage.go:128:37: cannot use master (variable of type string) as []string value in argument to neo.NewStorage

See kirr/neo!2 for discussion,
context and details.

/reviewed-by @kirr

* t-with-multiple-master-nodes:
  fixup! client_test: Add nmaster={1,2} to test matrix
  fixup! client_test: Support test cluster /w >1 master
  fixup! TalkMaster: Switch master if dialed M is secondary
  fixup! Node: Add support for NEO cluster with > 1 master
  fixup! Dial: Catch NotPrimaryMaster & return custom error
  fixup! proto: Implement Error for NotPrimaryMaster
  fixup! proto.NotPrimaryMaster: Fix .Primary data type (2)
  fixup! proto.NotPrimaryMaster: Fix .Primary data type (1)
  client_test: Add nmaster={1,2} to test matrix
  client_test: Support test cluster /w >1 master
  proto.NotPrimaryMaster: Fix .Primary data type
  TalkMaster: Switch master if dialed M is secondary
  Dial: Catch NotPrimaryMaster & return custom error
  proto: Implement Error for NotPrimaryMaster
  openClientByURL: Fix for >1 master (split URL host)
  Client.URL: Fix incomplete URL if > 1 master nodes
  Node: Add support for NEO cluster with > 1 master

Actually do test nmaster=2 case.

- show all options in error context and ran test kind

- skip nmaster > 1 for NEO/go as that is currently not implemented on NEO/go server

- use json for interacting with runneo.py, so that we can use
  whatever builtin type for any argument without hardcoding ad-hoc
  handling of specific arguments inside runneo.py.

- adjust comments + cosmetics

- validate received NotPrimaryMaster
- use Address.String() instead of printf with format that works only for ipv6
- add some logging, comments and TODO

- no need to keep Node.MasterAddr anymore - the address of current PM is
  managed by TalkMaster and is provided as part of operational context
  to user functions that TalkMaster runs.

- correct docstrings.

- cosmetics.

Expect NotPrimaryMaster only if we are trying to connect to a master.

Provide details in the error message.

Change .Promary type from int8 back to int32.

5d93e434 says that .Primary type is not NodeID. That is true, but
changing it to int8 was a mistake:

1. PSignedNull is explicitly defined to come with '!l' struct code, which
   according to https://docs.python.org/3/library/struct.html#module-struct
   comes as 4-bytes integer on the wire:

   https://lab.nexedi.com/nexedi/neoppod/blob/v1.12-13-gf2ea4be2/neo/lib/protocol.py#L560-562

2. verifying this via serializing NotPrimaryMaster on NEO/py also
   confirms that .Primary occupies 4 bytes, not one:

   In [1]: from neo.lib.protocol import NotPrimaryMaster

   In [2]: NotPrimaryMaster(0x01020304, [('m111', 111), ('m222', 222)])._body
   Out[2]: '\x01\x02\x03\x04\x00\x00\x00\x02\x00\x00\x00\x04m111\x00o\x00\x00\x00\x04m222\x00\xde'
            ^^^^^^^^^^^^^^^^
            NOTE  NOTE  NOTE

-> change .Primary type back to being 4-bytes integer, but to int32
instead of NodeID because, as 5d93e434 correctly says, .Primary comes as
array index, not a node ID. The following place of NEO/py code
explicitly confirms this:

https://lab.nexedi.com/nexedi/neoppod/blob/v1.12-13-gf2ea4be2/neo/master/handlers/identification.py#L155-159

Add corresponding test.

Rerun `go generate`. As the diff in zproto-marshal.go shows changing
NotPrimaryMaster.Primary type from NodeID to int8 actually does make a
difference. This happens because NodeID type is based on int32 and
changing that to int8 changes how NotPrimaryMaster structure is layed
out in memory and on the wire.

The changes to zproto-marshal.go in 5d93e434 seem to be done by hand
and not matching the change to proto.go even though head of
zproto-marshal.go says

	// Code generated by protogen.go; DO NOT EDIT.

Tests should work with both one master or more than one masters.

Now it's possible to run client tests against a NEO cluster which
has more than one master nodes. We need this adjustement in order
to test NEO/go client modification in order to support more than
one master node.

The '.Primary' attribute of the 'NotPrimaryMaster' packet has been
assigned to 'NodeID' data type. This is incorrect, because the data
doesn't represent the ID of the node, but an index of the
'.KnownMasterList' [1]. In the old protocol NEO/py therefore also
used 'PSignedNull' instead of 'PUUID' [2]. This patches fixes the data
type of '.Primary' and uses 'int8' instead of 'NodeID'. Technically this
doesn't make any difference, but semantically for human beings the code
is easier to understand now.

[1] https://lab.nexedi.com/nexedi/neoppod/blob/c6453626/neo/lib/handler.py#L161
[2] https://lab.nexedi.com/nexedi/neoppod/blob/c6453626/neo/lib/protocol.py#L716

When connecting to a master node, the client needs to try a different
master if the initially tried one is a secondary master node. This
statement wasn't implemented yet before this patch and therefore it was
good luck if the initally tried master was the primary one - and the
connection worked - or if it was a secondary master - and the client
got stuck in re-trying the same node forever. This patch makes NEO/go
usage with clusters of more than one master therefore much more stable.

After initial handshake a NEO node checks the identification of its peer
by sending the 'RequestIdentification' packet. In case the peer is a
secondary master it responds with 'NotPrimaryMaster'. Before this patch
'Dial' ignored the 'NotPrimaryMaster' packet and simply returned a
general error. Now - after this patch - 'Dial' returns an instance of
'proto.NotPrimaryMaster' (which implements 'Error').
This helps a caller to correctly handle the secondary-master-case, which
otherwise is impossible to differentiate from any other error
possibility.

When a client receives 'NotPrimaryMaster' from a secondary master, the
situation is similar to the situation when we receive an error: the
other node tells us, don't connect with me, connect with someone else.
Finally the peer even closes the connection.

Due to this similarity in structure (& because it helps us later to
teach NEO/go to correctly handle 'NotPrimaryMaster' with minimal
changes), we implement 'Error' for 'proto.NotPrimaryMaster'. Now
'NotPrimaryMaster' can be treated like an error.

In a NEO URI more than one master node can be specified, because a NEO
cluster may have more masters than one. But before this patch
'openClientByURL' always assumed that the given URL only specifies one
master. Now the host is split into potentially > 1 master nodes. It
therefore works now in the same way as the Python implementation [1].

[1] https://lab.nexedi.com/nexedi/neoppod/blob/342168cd/neo/client/zodburi.py#L64