Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

statedb: Simplify integration with Hive #28892

Merged
merged 2 commits into from Nov 13, 2023

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Oct 31, 2023

Instead of relying on the group values use an explicit registration for the tables. This allows more flexibility for how to create the tables and how to for example scope them to a module or even to keep RWTable[T] hidden inside an object without having the complicated NewProtectedTableCell and so on helpers which obfuscate what is going on.

// A constructor function for a table. Used by the real implementation and integration tests of components
// that depend on this table can use this to create and populate it.
func NewExampleTable() statedb.RWTable[*Example] {
  return statedb.NewTable[*Example]("test", ExampleIDIndex, ExampleNameIndex)
}

func example() {
  hive.New(
    statedb.Cell,
    cell.Module("example", "Example",
      cell.ProvidePrivate(NewExampleTable), // Provides RWTable[*Example] just to this module.

      // 1st way to register a table.
      cell.Invoke(statedb.RegisterTable[*Example]),

      // 2nd longer way to register a table
      cell.Invoke(func(db *statedb.DB, table statedb.RWTable[*Example]) error {
        return db.RegisterTable(table)
      }),       

    // Provide the read-only "Table[*Example]" API to the whole hive. Here or from a start
    // hook we can for example populate it before anything reads it. We get the guarantee
    // that the start hook is called before any readers if we append the hook from this constructor.
    cell.Provide(func(table statedb.RWTable[*Example]) statedb.Table[*Example] {
      return table
    }),

    // Shorter way to provide Table[*Example] using ToTable.
    // This method referenced this way has type "func(table RWTable[*Example]) Table[*Example]"
    cell.Provide(statedb.RWTable[*Example].ToTable),
  )
}

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 31, 2023
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Nov 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 2, 2023
@joamaki joamaki changed the title statedb: Experiment with a potentially simpler API statedb: Simplify integration with Hive Nov 2, 2023
@joamaki joamaki marked this pull request as ready for review November 3, 2023 06:46
@joamaki joamaki requested review from a team as code owners November 3, 2023 06:46
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the potential panics in NewTable.

pkg/statedb/table.go Outdated Show resolved Hide resolved
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@joamaki
Copy link
Contributor Author

joamaki commented Nov 6, 2023

/test

@joamaki
Copy link
Contributor Author

joamaki commented Nov 7, 2023

/test

@joamaki joamaki force-pushed the pr/joamaki/statedb-simplify branch 2 times, most recently from 1bd731f to 1e7845a Compare November 8, 2023 13:49
@joamaki
Copy link
Contributor Author

joamaki commented Nov 8, 2023

/test

@bimmlerd
Copy link
Member

bimmlerd commented Nov 9, 2023

CI triage:

@bimmlerd
Copy link
Member

bimmlerd commented Nov 9, 2023

/test

Instead of relying on the group values use an explicit registration
((*DB).RegisterTable) for the tables. This allows more flexibility
for how to create the tables and how to for example scope them to a
module or even to keep RWTable[T] hidden inside an object.

Example code:

  // A constructor function for a table. Used by the real implementation
  // and integration tests of components that depend on this table can
  // use this to create and populate it.
  func NewExampleTable() statedb.RWTable[*Example] {
    return statedb.NewTable[*Example]("test", ExampleIDIndex, ExampleNameIndex)
  }

  func example() {
    hive.New(
      statedb.Cell,
      cell.Module("example", "Example",
        cell.ProvidePrivate(NewExampleTable), // Provides RWTable[*Example] just to this module.

        // 1st way to register a table.
        cell.Invoke(statedb.RegisterTable[*Example]),

        // 2nd longer way to register a table
        cell.Invoke(func(db *statedb.DB, table statedb.RWTable[*Example]) error {
          return db.RegisterTable(table)
        }),

      // Provide the read-only "Table[*Example]" API to the whole hive. Here or from a start
      // hook we can for example populate it before anything reads it. We get the guarantee
      // that the start hook is called before any readers if we append the hook from this constructor.
      cell.Provide(func(table statedb.RWTable[*Example]) statedb.Table[*Example] {
        return table
      }),

      // Shorter way to provide Table[*Example] using ToTable.
      // This method referenced this way has type "func(table RWTable[*Example]) Table[*Example]"
      cell.Provide(statedb.RWTable[*Example].ToTable),
    )
  }

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Since with RegisterTable we can now create and register tables at runtime
it's better to avoid panics on user-data in the public API.

Added MustNewTable for tests.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@bimmlerd
Copy link
Member

bimmlerd commented Nov 10, 2023

/test (rebased to fix conflicts)

@bimmlerd
Copy link
Member

bimmlerd commented Nov 10, 2023

CI triage:

  • ci-ginkgo failed on E2E Test (1.28, f09-datapath-misc-2), but one of the cluster nodes just didn't come up, cilium agent never got to run on there, hence unrelated and rerunning.
  • GKE failed with connectivity test failed: unmarshaling cilium runtime config json: unexpected end of JSON input for one of three - sounds like an infra issue. Not required.
  • AKS failed a connectivity test [pod-to-pod-encryption/pod-to-pod-encryption/curl-ipv4: cilium-test/client-6b4b857d98-ktkp4 (10.0.1.192) -> cilium-test/echo-other-node-6f988f8697-mhm6k (10.0.0.64:8080)] with ❌ command "curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 http://10.0.0.64:8080/" failed: error dialing backend: EOF, but is also not required.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 10, 2023
@squeed squeed merged commit 1f989de into cilium:main Nov 13, 2023
58 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants