Skip to content

WIP: Titanium Integration #2165

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

Tobianas
Copy link
Contributor

@Tobianas Tobianas commented Mar 11, 2025

Adopt:

  • odlparent-14.0.8
  • infrautils-7.1.1
  • yangtools-14.0.11
  • mdsal-14.0.11
  • controller-11.0.0-SNAPSHOT
  • aaa-0.21.0-SNAPSHOT
  • netconf-9.0.0-SNAPSHOT
  • bgpcep-0.23.0-SNAPSHOT

@Tobianas Tobianas changed the title Titanium Integration WIP: Titanium Integration Mar 11, 2025
@Tobianas Tobianas marked this pull request as draft March 11, 2025 11:12
@Tobianas Tobianas force-pushed the titanium branch 5 times, most recently from 32c847b to 35ad001 Compare March 14, 2025 11:45
Adopt:
- odlparent-14.0.8
- infrautils-7.1.1
- yangtools-14.0.11
- mdsal-14.0.11
- controller-11.0.0-SNAPSHOT
- aaa-0.21.0-SNAPSHOT
- netconf-9.0.0-SNAPSHOT
- bgpcep-0.23.0-SNAPSHOT

Signed-off-by: tobias.pobocik <[email protected]>
Tobianas added 18 commits March 18, 2025 14:10
opendaylight/controller@20d4ef4

JIRA: LIGHTY-354
Signed-off-by: tobias.pobocik <[email protected]>
opendaylight/netconf@48249d0

JIRA: LIGHTY-354
Signed-off-by: tobias.pobocik <[email protected]>
Instead of using Lighty CompletingServerRequest, rather use ODL
CompletingServerRequest.

JIRA: LIGHTY-354
Signed-off-by: tobias.pobocik <[email protected]>
opendaylight/bgpcep@48b3e26

JIRA: LIGHTY-354
Signed-off-by: tobias.pobocik <[email protected]>
Rename this parameter to fix this checkstyle violation.

JIRA: LIGHTY-354
Signed-off-by: tobias.pobocik <[email protected]>
Remove this commit when non-snapshot versions will be available.

Signed-off-by: tobias.pobocik <[email protected]>
@ihrasko
Copy link
Collaborator

ihrasko commented May 21, 2025

/gemini review

@ihrasko ihrasko marked this pull request as ready for review May 21, 2025 06:27
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request focuses on integrating Titanium dependencies and migrating from Akka to Pekko. The changes involve updating dependency versions in POM files, adjusting configuration files (JSON and Helm templates) to use Pekko paths and class names, and updating YANG module revisions. There are also some related code changes to adapt to API updates in the new dependency versions. Overall, the core changes for the Pekko migration seem mechanically correct based on the diffs provided.

Summary of Findings

  • Pekko Management Configuration Duplication: The pekko-default.conf file includes a pekko.management.http configuration block that might be redundant or conflict with management settings provided via Helm chart values in a Kubernetes deployment. (Medium severity, commented)
  • Pekko Snapshot Directory: The snapshot-store.local.dir property is commented out in pekko-default.conf, changing the default snapshot location. The intended new location and its suitability for persistence should be clarified. (Medium severity, commented)
  • Datastore File Path: A hardcoded path Path.of("data") is now used for the datastore base directory. Clarification on its purpose and consideration for making it configurable for different deployment environments would be beneficial. (Medium severity, commented)
  • Snapshot Repository in POMs: The opendaylight-snapshot repository is added to several POM files with a TODO to remove it later. This is acceptable for a WIP branch but should be addressed before merging to a stable branch. (Low severity, not commented)
  • API Adaptations: The code correctly adapts to API changes in updated dependencies, including Pekko, AAA, and Netconf databind. (Low severity, not commented)
  • Test Code Refactoring: Test code has been refactored to use a library class (CompletingServerRequest) instead of a local definition, which is a good practice. (Low severity, not commented)
  • Configuration File Updates: Configuration files (JSON, Helm templates, .conf) have been updated consistently with the Pekko migration and YANG model revisions. (Low severity, not commented)

Merge Readiness

This pull request introduces significant dependency updates and the migration to Pekko. While the core migration changes appear mechanically correct, there are a few medium-severity issues identified related to configuration clarity and potential conflicts (Pekko management config, snapshot directory, datastore path). Given that this is marked as WIP and these points require clarification or potential adjustments for production readiness, I recommend addressing the commented issues before merging. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.

Comment on lines +1 to +8
pekko {
management {
http {
hostname = "127.0.0.1"
port = 8558
bind-hostname = "0.0.0.0"
bind-port = 8558
}

Choose a reason for hiding this comment

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

medium

This pekko.management.http configuration block has been added. It defines management hostname and port. Given that the Helm chart already provides configuration for Pekko management (e.g., lighty.akka.managementPort), is this block necessary in this default configuration file? It might be redundant or potentially conflict with the Helm chart values when deployed in Kubernetes.

pekko {
  remote {
    artery {
      enabled = on
      transport = tcp
      canonical.hostname = "127.0.0.1"
      canonical.port = 2550
    }
  }

  cluster {

@@ -28,13 +36,12 @@
# modifying the following two properties. The directory location specified may be a relative or absolute path.
# The relative path is always relative to KARAF_HOME.

snapshot-store.local.dir = "target/snapshots"
# snapshot-store.local.dir = "target/snapshots"

Choose a reason for hiding this comment

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

medium

The snapshot-store.local.dir property has been commented out. This changes the default directory for Pekko snapshots. Is this intentional? If so, where will snapshots be stored now, and is this location suitable for persistent storage, especially in a clustered environment?

snapshot-store.local.use-lz4-compression = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants