Chromium integration#2632
Conversation
Now when lighty-minimal-parent has odlparent as its parent, there is no need to specify these versions. JIRA: LIGHTY-427 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
There was a problem hiding this comment.
Code Review
This pull request upgrades OpenDaylight (ODL) dependencies and plugins, refactors gNMI module dependencies to use ODL gNMI artifacts instead of local models, and introduces a new lighty-gnmi-device-simulator wrapper module. It also updates configuration loading and classpath model resolution utilities. The review feedback highlights several improvement opportunities: explicitly defined dependency versions in lighty-rcgnmi-app-module/pom.xml should be managed by the imported BOM, potential InputStream resource leaks exist in RcGnmiAppModuleConfigUtils.java due to unclosed streams, and the ServiceLoader lookup in getModelsFromClasspath is inefficient and should be optimized by pre-loading providers into a map.
I am having trouble creating individual review comments. Click here to see my feedback.
lighty-applications/lighty-rcgnmi-app-aggregator/lighty-rcgnmi-app-module/pom.xml (53-71)
The versions for certificates-storage-model, force-capabilities, topology-model, and yang-storage-model are explicitly set to 1.0.0. Since org.opendaylight.gnmi:gnmi-artifacts (version 3.0.0-SNAPSHOT) is imported in dependency-versions/pom.xml, these dependencies should have their versions managed automatically. Specifying 1.0.0 explicitly overrides the managed version and can lead to version mismatch issues. Please remove the <version> tags.
<dependency>
<groupId>org.opendaylight.gnmi</groupId>
<artifactId>certificates-storage-model</artifactId>
</dependency>
<dependency>
<groupId>org.opendaylight.gnmi</groupId>
<artifactId>force-capabilities</artifactId>
</dependency>
<dependency>
<groupId>org.opendaylight.gnmi</groupId>
<artifactId>topology-model</artifactId>
</dependency>
<dependency>
<groupId>org.opendaylight.gnmi</groupId>
<artifactId>yang-storage-model</artifactId>
</dependency>
lighty-applications/lighty-rcgnmi-app-aggregator/lighty-rcgnmi-app-module/src/main/java/io/lighty/applications/rcgnmi/module/RcGnmiAppModuleConfigUtils.java (85)
The InputStream returned by Files.newInputStream(path) is not closed, which can lead to a resource leak if an exception is thrown or even during normal execution. Use a try-with-resources block to ensure the stream is properly closed.
GnmiConfiguration gnmiConfiguration;
try (InputStream is = Files.newInputStream(path)) {
gnmiConfiguration = getGnmiConfiguration(is);
}lighty-applications/lighty-rcgnmi-app-aggregator/lighty-rcgnmi-app-module/src/main/java/io/lighty/applications/rcgnmi/module/RcGnmiAppModuleConfigUtils.java (96)
The InputStream returned by Files.newInputStream(path) is not closed, which can lead to a resource leak. Use a try-with-resources block to ensure the stream is properly closed.
final ModulesConfig modulesConfig;
try (InputStream is = Files.newInputStream(path)) {
modulesConfig = ModulesConfig.getModulesConfig(is);
}lighty-applications/lighty-rcgnmi-app-aggregator/lighty-rcgnmi-app-module/src/main/java/io/lighty/applications/rcgnmi/module/RcGnmiAppModuleConfigUtils.java (152-164)
Iterating over ServiceLoader inside a loop (filterYangModelBindingProviders is called for each QName in filter) is highly inefficient because ServiceLoader traversal can be slow. Instead, pre-load the providers into a map once, and then look them up by QName. Additionally, Collections.unmodifiableSet(resolvedModules.values().stream().collect(Collectors.toSet())) can be simplified to Set.copyOf(resolvedModules.values()) for better readability and performance.
public static Set<YangModuleInfo> getModelsFromClasspath(final Set<QName> filter) {
Map<QName, YangModuleInfo> resolvedModules = new HashMap<>();
ServiceLoader<YangModelBindingProvider> yangProviderLoader = ServiceLoader.load(YangModelBindingProvider.class);
Map<QName, YangModuleInfo> allProviders = new HashMap<>();
for (YangModelBindingProvider provider : yangProviderLoader) {
YangModuleInfo info = provider.getModuleInfo();
allProviders.put(info.getName(), info);
}
for (QName moduleId : filter) {
YangModuleInfo info = allProviders.get(moduleId);
if (info != null) {
resolvedModules.put(info.getName(), info);
addDependencies(resolvedModules, info.getImportedModules());
}
}
return Set.copyOf(resolvedModules.values());
}Everything from this module is now redundant, since its possible to get it from ODL-gnmi repository as a dependency. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
There is no need for aggregator, since all gNMI now needs is inside one module. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Just like with other lighty.io modules, rename this class to match lighty.io code-style. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Use imports and dependencies from odl gnmi repository since most of lighty.io gnmi code was removed. Also remove unused dependencies from pom.xml. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
These models were moved to ODL and thus renamed. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
GnmiConfigUtils got reworked and no longer provide configuration from JSON file, because karaf does not need JSON configuration. This patch extract everything necessary into RcGnmiAppModuleConfigUtils. https://github.com/opendaylight/gnmi/blob/master/modules/gnmi-sb/src/main/java/org/opendaylight/gnmi/southbound/yangmodule/util/GnmiConfigUtils.java JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
These classes now accept YangParserFactory in their constructor, Add DefaultYangParserFactory to it. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
The jarfile available from nexus does not work since it's not a fat jar. Use this wrapper as a workaround to get the gNMI simulator working. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Use the odl gNMI simulator from our wrapper to run the example application and the scripts used in GH workflow. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
These models can be get from ODL gnmi repository now, so there is no need to store them in Lighty.io JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Lighty.io configuration relies on having a path to directory of yang models to build effective model context. Since the models got removed from Lighty.io, they have to be included in built target directory for lighty configuration to be accessed. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Ensure LightyGnmiSouthboundModule provides a non-null reactor to the underlying OpenDaylight GnmiSouthboundProvider. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Update RcGnmiAppModuleConfigUtils to automatically share the YANG models loaded by the controller with the gNMI configuration. This resolves "Missing models" errors (iana-if-type, ietf-interfaces, etc.) during device schema context building. By injecting the controller's schema models into the GnmiConfiguration, we eliminate the need for redundant model definitions in the application's JSON configuration files. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
By default, gnmi starts with ietf-interfaces@2018-02-20 when no configuration is provided. This means that when we add ietf-interfaces@2014-05-08 we are causing two different versions of the same model to be present. This causes the controller to fail to connect to the device simulator. https://github.com/opendaylight/gnmi/blob/master/modules/gnmi-sb/src/main/java/org/opendaylight/gnmi/southbound/yangmodule/GnmiSouthboundModule.java#L136 JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
This workflow assumed that Lighty.io controller is ready to accept the downloaded yang models right after the workflow starts. This caused the requests to add the models to fail simply because the controller was not ready yet to accept them. This patch adds a check to verify that the controller is ready to accept them and then adds them via RPC. JIRA: LIGHTY-416 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
The getModelsFromClasspath method iterates over the filter set and for each element, it iterates over the entire ServiceLoader in filterYangModelBindingProviders. This results in an O(N*M) complexity. It would be more efficient to iterate over the ServiceLoader once to build a map of QName to YangModuleInfo and then look up the modules in the filter. JIRA: LIGHTY-409 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
6a56c19 to
0a3f49a
Compare
- odlparent-14.3.1 - infrautils-7.1.12 - yangtools-15.0.2 - ietf-2.0.2 - mdsal-16.0.3 - controller-13.0.2 - aaa-0.23.2 - netconf-11.0.0 - bgpcep-2.0.0 - gnmi-3.0.0 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
This method is a superfluous wrapper around a static field: expose the field directly: opendaylight/yangtools@7aae7f5 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
The transport wiring of OpenAPI was split into separate restconf-openapi-jaxrs module. Thus it needs to be add. JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
opendaylight/netconf@45afbb4#diff-28799befb2681209d93c0cccbad076367bcd2b93f645d48c372e7da29b7b8c17 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
The DatabindContext was migrated to yangtools in: opendaylight/yangtools@2265500#diff-68d7a1625a8aa4627a2444bced7c908fe1eb13b1ec5bce910ca5d5e72dec0cea JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
OpenAPI now gets base path from string instead of JaxRsEndpoint: opendaylight/netconf@064f52e JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
No need to add default domain: opendaylight/aaa@269ed45 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
opendaylight/yangtools@0978ec6 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
opendaylight/netconf@3628f2e opendaylight/netconf@21e26bf JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
This dependency version is incorrect and does not even need to be specified. Remove it. JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
opendaylight/netconf@c1b8141 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Remove this parameter as it is not required thanks to: opendaylight/yangtools@b141d2b#diff-c1ec33029a615e55f18a251c6d6870ca018a6b2ac58a7545e89f167c14ee6292L43 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
This class now has YangTextToIRSourceTransformer in its constructor: opendaylight/yangtools@5597167#diff-0e508ee84fe3fc8d6cbb4ddeea57567acee8678616e260fab3d8f1a0bf0dc369R102 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
This class now has YangTextToIRSourceTransformer in its constructor: opendaylight/netconf@f0f3304#diff-033efda10023b4a761caf19552150022e574e075cbf76e5e999a568f2624b2cfR63 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Include the missing configuration fields for netty: opendaylight/netconf@a9a5eb9#diff-da88dcd810f7663a4afddbcee9ffe0bd4e4790a82a0d705541a0c777e9494259 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Due to changes made in latest chromium gNMI release, we will no longer be providing CrossSourceStatementReactor to LightyGnmiSouthboundModule as it is no longer used, and was replaced with YangParserFactory and YangTextToIRSourceTransformer. This also required us to rework other modules which use gNMI. opendaylight/gnmi@6597252#diff-6ab18db10324419edd4f532cb0860d51acc3796c4a5eaeb395dfa8946ee41880 JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Add this dependency to resolve IllegalState No YangXPathParserFactory found. This Exception is thrown because DefaultYangParserFactory uses ServiceLoader To try to lookup YangXPathParserFactory, and the YangXPathParserFactory was moved in: opendaylight/yangtools@811376f JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Add this dependency to resolve IllegalState No YinTextToDOMSourceTransformer found. This Exception is thrown because DefaultYangParserFactory uses ServiceLoader To try to lookup YinTextToDOMSourceTransformer, which was moved in: opendaylight/yangtools@67071bc JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Without this parser, the application fails to parse yang model and crashes on the firs one it encounters. JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
OpenAPI now recieves the restconf path as a string instead of getting it from JaxRsEndpoint. JIRA: LIGHTY-435 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
0a3f49a to
86a2d0b
Compare
|
Rebased on #2622 |
No description provided.