4964 add information about lacp connection status for lag ports #5040

durgakako · 2023-01-16T22:13:50Z

No description provided.

…sues

pkazlenka · 2023-01-26T09:34:44Z

...vice/northbound/src/main/java/org/openkilda/northbound/controller/v2/SwitchControllerV2.java

+     * @param logicalPortNumber the switch
+     *
+     */
+    @ApiOperation(value = "Read all LACP status on specific switch", response = LacpStatusResponse.class)


Seems like description should be Read LACP status on specific port of the switch
Also, as long as you don't allow ports list or range as an input, you should return LacpStatusResponse, not List

...e-topology/base-messaging/src/main/java/org/openkilda/messaging/info/event/LacpInfoData.java

...-hibernate/src/main/java/org/openkilda/persistence/hibernate/HibernateRepositoryFactory.java

dmitrii-beliakov · 2023-01-30T18:23:51Z

...istence-tinkerpop/src/main/java/org/openkilda/persistence/ferma/frames/LacpPartnerFrame.java

+
+    @Override
+    @Property(LOGICAL_PORT_NUMBER_PROPERTY)
+    public abstract void setLogicalPortNumber(int logicalPortNumber);


It seems to me, that using primitives make this field effectively mandatory non-null. Is that so for all cases here that they always have some value? Or is it possible that some of the ints or booleans are not set (are null)?

...p/src/main/java/org/openkilda/persistence/ferma/repositories/FermaLacpPartnerRepository.java

dmitrii-beliakov · 2023-01-30T18:33:36Z

...c/test/java/org/openkilda/persistence/ferma/repositories/FermaLacpPartnerRepositoryTest.java

+        assertTrue(lacpPartner.isPresent());
+
+        lacpPartner = lacpPartnerRepository.findBySwitchIdAndLogicalPortNumber(TEST_SWITCH_ID_2, LOGICAL_PORT_NUMBER_1);
+        assertTrue(lacpPartner.isPresent());


I think isPresent is not enough. If the repo returns a random result for all the calls, this test passes, but that's not ok. Please add more specific assertions.

...m-topology/src/main/java/org/openkilda/wfm/topology/nbworker/bolts/SwitchOperationsBolt.java

...logy/src/main/java/org/openkilda/wfm/topology/nbworker/services/SwitchOperationsService.java

...vice/northbound/src/main/java/org/openkilda/northbound/controller/v2/SwitchControllerV2.java

...ervice/northbound/src/main/java/org/openkilda/northbound/service/impl/SwitchServiceImpl.java

niksv · 2023-01-30T17:21:07Z

docker/db-migration/migrations/root.yaml

@@ -81,3 +81,6 @@ databaseChangeLog:
  - include:
      relativeToChangelogFile: true
      file: 023-add-lacp-reply-into-lag-class.yaml
+  - include:
+      relativeToChangelogFile: true
+      file: 025-add-lacp-partner-class.yaml


I'm not sure when migration 024 will be merged so please change your migration name to 024

Updated to 024

niksv · 2023-01-30T17:22:03Z

...e-topology/base-messaging/src/main/java/org/openkilda/messaging/info/event/LacpInfoData.java

@@ -0,0 +1,39 @@
+/* Copyright 2017 Telstra Open Source


niksv · 2023-01-30T17:24:30Z

...logy/src/main/java/org/openkilda/wfm/topology/connecteddevices/ConnectedDevicesTopology.java

+            System.exit(handleLaunchException(e));
+        }
+    }
+


Why did you move main method from the end of the file?

Its formatted by IntelliJ, Updated.

niksv · 2023-01-30T17:26:22Z

...logy/src/main/java/org/openkilda/wfm/topology/connecteddevices/ConnectedDevicesTopology.java

@@ -80,6 +95,10 @@ private void createSpout(TopologyBuilder builder) {
        declareKafkaSpout(builder, topologyConfig.getKafkaTopoConnectedDevicesTopic(), CONNECTED_DEVICES_SPOUT_ID);
    }

+    private void createLacpSpout(TopologyBuilder builder) {
+        declareKafkaSpout(builder, topologyConfig.getKafkaLacpTopic(), LACP_SPOUT_ID);


You forgot to add LACP_SPOUT_ID into input of RouterBolt. It should be

private void createRouterBolt(TopologyBuilder builder, PersistenceManager persistenceManager) { RouterBolt routerBolt = new RouterBolt(persistenceManager, ZooKeeperSpout.SPOUT_ID); declareBolt(builder, routerBolt, ROUTER_BOLT_ID) .shuffleGrouping(CONNECTED_DEVICES_SPOUT_ID) .shuffleGrouping(LACP_SPOUT_ID) .allGrouping(ZooKeeperSpout.SPOUT_ID); }

niksv · 2023-01-30T17:29:04Z

...opology/src/main/java/org/openkilda/wfm/topology/connecteddevices/service/PacketService.java

+     * <code>SwitchConnectedDeviceFrame.UNIQUE_INDEX_PROPERTY</code>
+     */
+    public static String createMessageKey(LacpInfoData data) {
+        return String.format("%s_%s_arp", data.getSwitchId(), data.getLogicalPortNumber());


your key has postfix arp but it should be lacp

niksv · 2023-01-30T17:29:58Z

...opology/src/main/java/org/openkilda/wfm/topology/connecteddevices/service/PacketService.java

+    /**
+     * This key is needed to balance load on Packet Bolt. If you see that some packet bolts have high load, and
+     * some have low load, try to extend this key. Maximum extension is equal to
+     * <code>SwitchConnectedDeviceFrame.UNIQUE_INDEX_PROPERTY</code>


Last sentence should be removed because Lacp DB frame has no UNIQUE_INDEX_PROPERTY

dkakollu and others added 5 commits December 28, 2022 12:56

Issue-4964 Add information about LACP connection status model updates

b46470c

Lacp status model, migration script typo fix

564e7e1

Issue-4964 LACP connection status API updates

4e135a7

Issue-4964 LACP connection status API updates - Updated checkstyle is…

cc2abfc

…sues

Issue-4964 Add NB API on LACP connection status

bb072f4

durgakako linked an issue Jan 16, 2023 that may be closed by this pull request

Add information about LACP connection status for LAG ports #4964

Open

durgakako requested review from pablomuri and dmitrii-beliakov January 17, 2023 23:51

pkazlenka requested changes Jan 26, 2023

View reviewed changes

Issue-4964 Update NB API to get LACP connection status

4a2c83a

durgakako requested a review from niksv January 26, 2023 23:05

durgakako added the On Review label Jan 27, 2023

dmitrii-beliakov reviewed Jan 30, 2023

View reviewed changes

niksv reviewed Jan 30, 2023

View reviewed changes

durgakako added the feature label Jan 31, 2023

Ubuntu added 4 commits January 31, 2023 23:21

#4964 LACP status - Updated based on review comments

ffeebda

#4964 LACP status - Updated based on review comments

0185f4a

Merge from develop

ab272d6

Renamed Migration Files

a153cf4

dmitrii-beliakov approved these changes Feb 7, 2023

View reviewed changes

durgakako added the Ready For Testing label Feb 10, 2023

Merge from develop

670114d

artHalMirantis removed the Ready For Testing label Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4964 add information about lacp connection status for lag ports #5040

4964 add information about lacp connection status for lag ports #5040

durgakako commented Jan 16, 2023

pkazlenka Jan 26, 2023

durgakako Jan 26, 2023

dmitrii-beliakov Jan 30, 2023

dmitrii-beliakov Jan 30, 2023

niksv Jan 30, 2023

durgakako Feb 7, 2023

niksv Jan 30, 2023

durgakako Feb 1, 2023

niksv Jan 30, 2023

durgakako Feb 1, 2023

niksv Jan 30, 2023

durgakako Feb 1, 2023

niksv Jan 30, 2023

durgakako Feb 1, 2023

niksv Jan 30, 2023

durgakako Feb 1, 2023

4964 add information about lacp connection status for lag ports #5040

Are you sure you want to change the base?

4964 add information about lacp connection status for lag ports #5040

Conversation

durgakako commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment