1. 18 Sep, 2024 5 commits
    • Kirill Smelkov's avatar
      X go.mod: Adjust it for updated zodb/internal/weak · efde5253
      Kirill Smelkov authored
      - go 1.14 -> 1.18 as we now use generics
      - `go get go4.org/unsafe/assume-no-moving-gc` since weak package uses that
      - `go mod tidy` as go build/test now was requesting this
      efde5253
    • Kirill Smelkov's avatar
      Merge branch 'master' into t · bf9c82e7
      Kirill Smelkov authored
      This replaces previous attempts to fix zodb/internal/weak with patches
      from master.
      
      * master:
        go/zodb/internal/weak: Disable support for weak references
        go/zodb/internal/weak: Assert that the object was not moved in the finalizer
        go/zodb/internal/weak: Try to fix GC crashes via reworking Ref to keep only one word instead of two
      bf9c82e7
    • Kirill Smelkov's avatar
      go/zodb/internal/weak: Disable support for weak references · ee23551d
      Kirill Smelkov authored
      Experience shows that we cannot provide reliably working weak references
      support without crashing GC because there needs to be a coordination in
      between object resurrection and at least GC mark phase[1,2] with [3] and
      [4] showing an idea what kind it coordination it needs to be.
      
      And since support for weak references is upcoming in standard library
      with hopefully Go 1.24 [5], it does not make sense to continue pushing
      hard to make weak references work on our standalone side.
      
      By disabling support for custom weak references we will avoid crashes,
      but will start to leak unused objects in zodb.Connection live cache.
      This should be relatively ok for now as WCFS in wendelin.core is
      currently the only known ZODB/go user and it cares to drop ZBlk* state
      after loading the data. The leak, thus, should be modest with,
      hopefully, not creating problems in practice.
      
      And once std package weak is out there we will switch to that restoring
      automatic LiveCache cleanup.
      
      [1] https://github.com/golang/go/issues/41303
      [2] wendelin.core@9b44fc23
      [3] https://github.com/golang/go/commit/dfc86e922cd0
      [4] https://github.com/golang/go/commit/79fd633632cd
      [5] https://github.com/golang/go/issues/67552
      
      /reviewed-by @levin.zimmermann
      /reviewed-on !11
      ee23551d
    • Kirill Smelkov's avatar
      go/zodb/internal/weak: Assert that the object was not moved in the finalizer · c48f7008
      Kirill Smelkov authored
      This is good to do since we are relying on non-moving property of
      current GC. It never triggered but it removed one uncertainty while
      debugging GC crash issue. Good to have it in place.
      
      /reviewed-by @levin.zimmermann
      /reviewed-on !11
      c48f7008
    • Kirill Smelkov's avatar
      go/zodb/internal/weak: Try to fix GC crashes via reworking Ref to keep only one word instead of two · 55491547
      Kirill Smelkov authored
      We are observing garbage-collector crashes due to weak package under
      load(*) with GC crashing as e.g.
      
          runtime: full=0xc0001f10000005 next=205 jobs=204 nDataRoots=1 nBSSRoots=1 nSpanRoots=16 nStackRoots=184
          panic: non-empty mark queue after concurrent mark
          fatal error: panic on system stack
      
          runtime stack:
          runtime.throw({0x5c60fe?, 0x601d70?})
                  /home/kirr/src/tools/go/go1.21/src/runtime/panic.go:1077 +0x5c fp=0xc000051e88 sp=0xc000051e58 pc=0x436efc
          panic({0x585100?, 0x601d70?})
                  /home/kirr/src/tools/go/go1.21/src/runtime/panic.go:840 +0x6ea fp=0xc000051f38 sp=0xc000051e88 pc=0x436e0a
          runtime.gcMark(0x118946?)
                  /home/kirr/src/tools/go/go1.21/src/runtime/mgc.go:1464 +0x40c fp=0xc000051fb0 sp=0xc000051f38 pc=0x41bd6c
          runtime.gcMarkTermination.func1()
                  /home/kirr/src/tools/go/go1.21/src/runtime/mgc.go:962 +0x17 fp=0xc000051fc8 sp=0xc000051fb0 pc=0x41b277
          runtime.systemstack()
                  /home/kirr/src/tools/go/go1.21/src/runtime/asm_amd64.s:509 +0x4a fp=0xc000051fd8 sp=0xc000051fc8 pc=0x468eea
      
      One problem in current implementation is that weak.Ref keeps two words
      for the copy of original interface object and recreates that interface
      object on Ref.Get from those two words _nonatomically_. Which is
      explicitly documented by Go memory model as something that can lead to
      corrupted memory and crashes. From https://go.dev/ref/mem#restrictions:
      
          This means that races on multiword data structures can lead to
          inconsistent values not corresponding to a single write. When the values
          depend on the consistency of internal (pointer, length) or (pointer,
          type) pairs, as can be the case for interface values, maps, slices, and
          strings in most Go implementations, such races can in turn lead to
          arbitrary memory corruption.
      
      We can avoid doing multiword writes during object resurrection by using
      concrete type T* instead of interface{}.
      
      Unfortunately as wendelin.core@9b44fc23
      shows it does not help with the above issue and the GC continues to
      crash with the same "panic: non-empty mark queue after concurrent mark"
      message. This is because weak.Ref implementation needs tight integration
      with concurrent GC that Go does and in practice we are unable to do that
      from outside of Go runtime internals. See e.g.
      https://github.com/golang/go/commit/dfc86e922cd0 and
      https://github.com/golang/go/commit/79fd633632cd to get an idea what
      kind of integration that needs to be.
      
      Anyway before disabling weak references support I wanted to commit this
      change to show that one-word approach was tried and it does not work.
      This leaves a trace in the history.
      
      On the good side we are going to use standard package weak in the future
      hopefully (https://github.com/golang/go/issues/67552). That needs to
      wait for at least Go 1.24 though.
      
      (*) see https://github.com/golang/go/issues/41303 for details
      
      /reviewed-by @levin.zimmermann
      /reviewed-on !11
      55491547
  2. 20 Aug, 2024 4 commits
    • Kirill Smelkov's avatar
      Merge branch 'master' into t · e7fde147
      Kirill Smelkov authored
      * master:
        go/neo/proto: Update 'Compression' to int to support different compression algorithms
      e7fde147
    • Levin Zimmermann's avatar
      go/neo/proto: Update 'Compression' to int to support different compression algorithms · 8e00bfdc
      Levin Zimmermann authored
      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
      
      /reviewed-by @kirr
      /reviewed-on kirr/neo!6
      8e00bfdc
    • Levin Zimmermann's avatar
      go/neo/proto: Update 'Compression' to int to support different compression algorithms · 12a6a923
      Levin Zimmermann authored
      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
      
      /reviewed-by @kirr
      /reviewed-on kirr/neo!6
      12a6a923
    • Levin Zimmermann's avatar
      identification: Set possible answers to same msgid · 20b37b60
      Levin Zimmermann authored
      When a node tries to connect to another node it initially sends a
      'RequestIdentification' packet. The other node can either reply with
      'AcceptIdentification' or in case of a secondary master with
      'NotPrimaryMaster'.
      
      In the second case the message id differs from the initial requests
      message id. This makes it difficult in a multi-threaded implementation
      to proceed this answer: due to the different msg/connection - id the
      multi-threaded implementation tries to proceed this incoming message in
      a different thread, while the requesting thread waits forever for its
      peers reply.
      
      The most straightforward solution is to use the same connection - id for
      both possible answers to the 'RequestIdentification' packet. This
      doesn't break given NEO/py implementation and is only a small patch.
      A workaround in a multi-threaded implementation on the other hand
      seems to be much more complicated and time-consuming. Finally it also makes
      sense semantically, because "Message IDs are used to identify response
      packets" and in the given context the 'NotPrimaryMaster' *is* the de facto
      response of a 'RequestIdentification'.
      
      /suggested-at kirr/neo!2
      /proposed-for-review-at nexedi/neoppod!20
      20b37b60
  3. 19 Aug, 2024 5 commits
    • Kirill Smelkov's avatar
      Merge branch 'master' into t · efdfb19d
      Kirill Smelkov authored
      * master:
        go/neo/proto: Fix KnownMasterList nesting
        go/neo/proto/RowInfo: Fix representation on the wire
      efdfb19d
    • Levin Zimmermann's avatar
      go/neo/proto: Fix KnownMasterList nesting · dc844465
      Levin Zimmermann authored
      Before this patch, the 'KnownMasterList' field of the 'NotPrimaryMaster'
      was expected to be structured in the following way:
      
      	ArrayHeader (KnownMasterList)
      
      	    ArrayHeader (KnownMaster)
      
      		ArrayHeader (Address)
      
      		    Host (string)
      		    Port (uint16)
      
      However NEO/py sends the following structure:
      
      	ArrayHeader (KnownMasterList)
      
      	    ArrayHeader (Address)
      
      		Host (string)
      		Port (uint16)
      
      This also makes sense, as 'KnownMaster' doesn't need to add another
      nesting, because it only includes the address.
      
      This patch amends the NEO/go protocol definition to transparently
      represent the nesting as it's send by NEO/py. See also
      levin.zimmermann/neoppod@18287612 for a similar issue.
      
      ----
      kirr: tests currently live only on t branch.
      
      /reviewed-by @kirr
      /reviewed-on !6
      dc844465
    • Levin Zimmermann's avatar
      go/neo/proto/RowInfo: Fix representation on the wire · 7f96ad77
      Levin Zimmermann authored
      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
      
      --------
      
      kirr: I've applied the following interdiff to the original patch of
      levin.zimmermann/neoppod@c93d5dbc :
      
          --- a/go/neo/neo_test.go
          +++ b/go/neo/neo_test.go
          @@ -100,7 +100,7 @@ func _TestMasterStorage(t0 *tEnv) {
                          PTid:           1,
                          NumReplicas:    0,
                          RowList:        []proto.RowInfo{
          -                       proto.RowInfo{proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
          +                       {proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
                          },
                  }))
      
          @@ -173,7 +173,7 @@ func _TestMasterStorage(t0 *tEnv) {
                          PTid:           1,
                          NumReplicas:    0,
                          RowList:        []proto.RowInfo{
          -                       proto.RowInfo{proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
          +                       {proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
                          },
                  }))
      
          --- a/go/neo/proto/proto_test.go
          +++ b/go/neo/proto/proto_test.go
          @@ -210,9 +210,9 @@ func TestMsgMarshal(t *testing.T) {
                                  PTid:        0x0102030405060708,
                                  NumReplicas: 34,
                                  RowList: []RowInfo{
          -                               {CellInfo{11, UP_TO_DATE}, CellInfo{17, OUT_OF_DATE}},
          -                               {CellInfo{11, FEEDING}},
          -                               {CellInfo{11, CORRUPTED}, CellInfo{15, DISCARDED}, CellInfo{23, UP_TO_DATE}},
          +                               {{11, UP_TO_DATE}, {17, OUT_OF_DATE}},
          +                               {{11, FEEDING}},
          +                               {{11, CORRUPTED}, {15, DISCARDED}, {23, UP_TO_DATE}},
                                  },
                                  },
      
          @@ -229,9 +229,9 @@ func TestMsgMarshal(t *testing.T) {
                                  hex("cf0102030405060708") +
                                  hex("22") +
                                  hex("93") +
          -                               hex("92"+"920bd40001"+"9211d40000") +
          -                               hex("91"+"920bd40002") +
          -                               hex("93"+"920bd40003"+"920fd40004"+"9217d40001"),
          +                               hex("92"+"920bd40401"+"9211d40400") +
          +                               hex("91"+"920bd40402") +
          +                               hex("93"+"920bd40403"+"920fd40404"+"9217d40401"),
                          },
      
                          // map[Oid]struct {Tid,Tid,bool}
      
      for cosmetics and because the tests were failing as
      
          --- FAIL: TestMsgMarshal (0.00s)
              proto_test.go:106: M/proto.AnswerPartitionTable: encode result unexpected:
              proto_test.go:107:  have: 93cf0102030405060708229392920bd404019211d4040091920bd4040293920bd40403920fd404049217d40401
              proto_test.go:108:  want: 93cf0102030405060708229392920bd400019211d4000091920bd4000293920bd40003920fd400049217d40001
      
      /reviewed-by @kirr
      /reviewed-on !6
      7f96ad77
    • Levin Zimmermann's avatar
      go/neo/proto: Fix KnownMasterList nesting · 30c69d9a
      Levin Zimmermann authored
      Before this patch, the 'KnownMasterList' field of the 'NotPrimaryMaster'
      was expected to be structured in the following way:
      
      	ArrayHeader (KnownMasterList)
      
      	    ArrayHeader (KnownMaster)
      
      		ArrayHeader (Address)
      
      		    Host (string)
      		    Port (uint16)
      
      However NEO/py sends the following structure:
      
      	ArrayHeader (KnownMasterList)
      
      	    ArrayHeader (Address)
      
      		Host (string)
      		Port (uint16)
      
      This also makes sense, as 'KnownMaster' doesn't need to add another
      nesting, because it only includes the address.
      
      This patch amends the NEO/go protocol definition to transparently
      represent the nesting as it's send by NEO/py. See also
      18287612 for a similar issue.
      
      /reviewed-by @kirr
      /reviewed-on kirr/neo!6
      30c69d9a
    • Levin Zimmermann's avatar
      go/neo/proto/RowInfo: Fix representation on the wire · 2cf10114
      Levin Zimmermann authored
      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
      
      --------
      
      kirr: I've applied the following interdiff to the original patch of
      c93d5dbc :
      
          --- a/go/neo/neo_test.go
          +++ b/go/neo/neo_test.go
          @@ -100,7 +100,7 @@ func _TestMasterStorage(t0 *tEnv) {
                          PTid:           1,
                          NumReplicas:    0,
                          RowList:        []proto.RowInfo{
          -                       proto.RowInfo{proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
          +                       {proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
                          },
                  }))
      
          @@ -173,7 +173,7 @@ func _TestMasterStorage(t0 *tEnv) {
                          PTid:           1,
                          NumReplicas:    0,
                          RowList:        []proto.RowInfo{
          -                       proto.RowInfo{proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
          +                       {proto.CellInfo{proto.NID(proto.STORAGE, 1), proto.UP_TO_DATE}},
                          },
                  }))
      
          --- a/go/neo/proto/proto_test.go
          +++ b/go/neo/proto/proto_test.go
          @@ -210,9 +210,9 @@ func TestMsgMarshal(t *testing.T) {
                                  PTid:        0x0102030405060708,
                                  NumReplicas: 34,
                                  RowList: []RowInfo{
          -                               {CellInfo{11, UP_TO_DATE}, CellInfo{17, OUT_OF_DATE}},
          -                               {CellInfo{11, FEEDING}},
          -                               {CellInfo{11, CORRUPTED}, CellInfo{15, DISCARDED}, CellInfo{23, UP_TO_DATE}},
          +                               {{11, UP_TO_DATE}, {17, OUT_OF_DATE}},
          +                               {{11, FEEDING}},
          +                               {{11, CORRUPTED}, {15, DISCARDED}, {23, UP_TO_DATE}},
                                  },
                                  },
      
          @@ -229,9 +229,9 @@ func TestMsgMarshal(t *testing.T) {
                                  hex("cf0102030405060708") +
                                  hex("22") +
                                  hex("93") +
          -                               hex("92"+"920bd40001"+"9211d40000") +
          -                               hex("91"+"920bd40002") +
          -                               hex("93"+"920bd40003"+"920fd40004"+"9217d40001"),
          +                               hex("92"+"920bd40401"+"9211d40400") +
          +                               hex("91"+"920bd40402") +
          +                               hex("93"+"920bd40403"+"920fd40404"+"9217d40401"),
                          },
      
                          // map[Oid]struct {Tid,Tid,bool}
      
      for cosmetics and because the tests were failing as
      
          --- FAIL: TestMsgMarshal (0.00s)
              proto_test.go:106: M/proto.AnswerPartitionTable: encode result unexpected:
              proto_test.go:107:  have: 93cf0102030405060708229392920bd404019211d4040091920bd4040293920bd40403920fd404049217d40401
              proto_test.go:108:  want: 93cf0102030405060708229392920bd400019211d4000091920bd4000293920bd40003920fd400049217d40001
      
      /reviewed-by @kirr
      /reviewed-on kirr/neo!6
      2cf10114
  4. 06 Aug, 2024 1 commit
    • Kirill Smelkov's avatar
      Merge branch 'master' into t · 6fb93a60
      Kirill Smelkov authored
      * master:
        go/neo/neonet: DialLink: Fix SIGSEGV in case client handshake fails
        go/neo/neonet: Demonstrate DialLink misbehaviour when all handshake attempts fail
      6fb93a60
  5. 04 Aug, 2024 2 commits
    • Levin Zimmermann's avatar
      go/neo/neonet: DialLink: Fix SIGSEGV in case client handshake fails · d75f4ac2
      Levin Zimmermann authored
      In case the last 'handshakeClient' call returns an error, 'DialLink'
      returns 'link = nil, err = nil'. Callers of 'DialLink' then don't
      recognize that 'link' is 'nil', as it's the convention to only check if
      'err' is 'nil', which leads to a 'segmentation violation' as soon as
      subsequent code tries to access fields of 'link':
      
      ```
      panic: runtime error: invalid memory address or nil pointer dereference
      [signal SIGSEGV: segmentation violation code=0x1 addr=0x14 pc=0x7087ae]
      
      goroutine 5 [running]:
      lab.nexedi.com/kirr/neo/go/neo/neonet.(*NodeLink).NewConn(0x0)
      	/srv/slapgrid/slappart82/srv/runner/instance/slappart6/software_release/parts/wendelin.core/wcfs/neo/go/neo/neonet/connection.go:404 +0x4e
      lab.nexedi.com/kirr/neo/go/neo/xneo.Dial.func1()
      	/srv/slapgrid/slappart82/srv/runner/instance/slappart6/software_release/parts/wendelin.core/wcfs/neo/go/neo/xneo/connect.go:138 +0x52
      lab.nexedi.com/kirr/neo/go/internal/xio.WithCloseOnErrCancel.func2()
      	/srv/slapgrid/slappart82/srv/runner/instance/slappart6/software_release/parts/wendelin.core/wcfs/neo/go/internal/xio/xio.go:114 +0x6a
      created by lab.nexedi.com/kirr/neo/go/internal/xio.WithCloseOnErrCancel in goroutine 21
      	/srv/slapgrid/slappart82/srv/runner/instance/slappart6/software_release/parts/wendelin.core/wcfs/neo/go/internal/xio/xio.go:109 +0x1ad
      ```
      
      This patch fixes this issue so that now 'err' and 'link' are never both
      'nil' again.
      
      /reviewed-by @kirr
      /reviewed-on !10
      d75f4ac2
    • Kirill Smelkov's avatar
      go/neo/neonet: Demonstrate DialLink misbehaviour when all handshake attempts fail · 03db1d8a
      Kirill Smelkov authored
      Levin found that when all handshake attempts fail DialLink returns
      both link=nil and err=nil which breaks what callers expect and lead to
      segmentation fault when accessing that nil link.
      
      -> Add test to demonstrate the problem.
      
      With xfail removed that test currently fails as
      
          --- FAIL: TestDialLink_AllHandshakeErr (0.00s)
          panic: lab.nexedi.com/kirr/neo/go/neo/neonet.TestDialLink_AllHandshakeErr.gox.func4.1: lab.nexedi.com/kirr/neo/go/neo/neonet.TestDialLink_AllHandshakeErr.func2: DialLink to handshake-rejecting server:
          have: link=<nil> err=<nil>
          want: link=<nil> err=client:1 - server:2: handshake (client): unexpected EOF [recovered]
      
      We will fix the problem in the next patch.
      
      /reported-by @levin.zimmermann
      /reported-at !10
      03db1d8a
  6. 23 Jul, 2024 2 commits
  7. 21 Jul, 2024 1 commit
    • Kirill Smelkov's avatar
      X: Sync zurl format with NEO/py · 95572d6a
      Kirill Smelkov authored
      Hello Kirill,
      
      in nexedi/neoppod!18 and nexedi/neoppod!21 we could find a common solution for a zurl format that previously diverged between NEOgo and NEOpy. The purpose of this MR is to sync again NEOgo and NEOpy zurl format. After merging this, we can continue to sync NEO zurl format in 'wendelin.core' & 'slapos'. Then we finally have unified approach again, which simplifies understanding and reduces unnecessary mental overhead.
      
      As this is strongly related to nexedi/neoppod!21 I thought it'd be a good idea to generally reduce difference and to replace WIP commits with merged NEOpy upstream commits.
      
      Best, Levin
      
      /reviewed-by @kirr
      /reviewed-on kirr/neo!7
      
      * lev/sync-zurl:
        client: Don't allow oPtion_nAme in zurl
        app: Remember SSL credentials so that it is possible to retrieve them
        client: Allow to force TLS via neos:// scheme
        client: Don't allow master_nodes and name to be present in options
        Revert "."
        Revert "Y client: Fix URI scheme to move credentials out of query"
        Revert "X Adjust NEO/go to neo:// URL change + py fixups"
        Revert "fixup! Y client: Fix URI scheme to move credentials out of query"
        Revert "Y client: Don't allow master_nodes and name to be present in options"
        go/client/zurl: Sync format to py upstream
      95572d6a
  8. 19 Jul, 2024 10 commits
  9. 02 Feb, 2024 3 commits
  10. 29 Jan, 2024 7 commits
    • Levin Zimmermann's avatar
      Revert "Y client: Adjust URI scheme to move client-specific options to fragment" · c9490507
      Levin Zimmermann authored
      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.
      c9490507
    • Kirill Smelkov's avatar
      fixup! client.go: Fix URI client option parsing for supported + unsupported options · f1a1bb9d
      Kirill Smelkov authored
      readonly is handled by common zodb.OpenDriver.
      f1a1bb9d
    • Levin Zimmermann's avatar
      client.go: Fix URI client option parsing for supported + unsupported options · d6c33660
      Levin Zimmermann authored
      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.
      d6c33660
    • Kirill Smelkov's avatar
      fixup! client_test: Add tests for NEO URI parser · 4e9311d5
      Kirill Smelkov authored
      - use simplified parseURL signature - DriverOptions are not passed nor changed there.
      - read-only is handled by generic zodb layer not neo.parseURL .
      4e9311d5
    • Levin Zimmermann's avatar
      client_test: Add tests for NEO URI parser · 1fca6ad4
      Levin Zimmermann authored
      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)
      1fca6ad4
    • Kirill Smelkov's avatar
      fixup! client: Refactor openClientByURL for easier testing · 2aa9b909
      Kirill Smelkov authored
      - 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.
      2aa9b909
    • Levin Zimmermann's avatar
      client: Refactor openClientByURL for easier testing · 7bad0dda
      Levin Zimmermann authored
      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.
      7bad0dda