Skip to content

Commit 556ec28

Browse files
divyagayathri-hclkishanps
divyagayathri-hcl
and
kishanps
authored
[P4RT] Do not add Packet I/O ports during reconciliation, discover from APP_STATE_DB. (#1663)
* [P4RT] Restore device id and packet-io ports before setting warm start state to reconciled. Signed-off-by: kishanps <[email protected]> * [P4RT] Do not add Packet I/O ports during reconciliation, discover from APP_STATE_DB. Signed-off-by: kishanps <[email protected]> --------- Signed-off-by: kishanps <[email protected]> Co-authored-by: kishanps <[email protected]>
1 parent 6bb1599 commit 556ec28

File tree

5 files changed

+252
-10
lines changed

5 files changed

+252
-10
lines changed

‎p4rt_app/p4rt.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -534,8 +534,7 @@ int main(int argc, char** argv) {
534534
warm_restart_util.GetPortIdsFromConfigDb(),
535535
warm_restart_util.GetCpuQueueIdsFromConfigDb(),
536536
warm_restart_util.GetFrontPanelQueueIdsFromConfigDb(),
537-
warm_restart_util.GetDeviceIdFromConfigDb(),
538-
warm_restart_util.GetPortsFromConfigDb());
537+
warm_restart_util.GetDeviceIdFromConfigDb());
539538
if (reconciliation_status.ok()) {
540539
swss::WarmStart::setWarmStartState(
541540
"p4rt", swss::WarmStart::WarmStartState::RECONCILED);

‎p4rt_app/p4runtime/p4runtime_impl.cc‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1826,8 +1826,7 @@ absl::Status P4RuntimeImpl::RebuildSwStateAfterWarmboot(
18261826
const std::vector<std::pair<std::string, std::string>>& cpu_queue_ids,
18271827
const std::vector<std::pair<std::string, std::string>>&
18281828
front_panel_queue_ids,
1829-
const std::optional<int>& device_id,
1830-
const std::vector<std::string>& ports) {
1829+
const std::optional<int>& device_id) {
18311830
/**
18321831
* controller_manager_, packetio_impl_, component_state_, system_state_,
18331832
* netdev_translator_, forwarding_config_full_path_ are restored in
@@ -1905,10 +1904,6 @@ absl::Status P4RuntimeImpl::RebuildSwStateAfterWarmboot(
19051904
RETURN_IF_ERROR(UpdateDeviceId(device_id.value())).LogError();
19061905
}
19071906

1908-
for (const auto& port : ports) {
1909-
RETURN_IF_ERROR(AddPacketIoPort(port)).LogError();
1910-
}
1911-
19121907
/**
19131908
* Restore the entity_cache_ cache by RebuildEntityEntryCache()
19141909
* */

‎p4rt_app/p4runtime/p4runtime_impl.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,8 +230,7 @@ class P4RuntimeImpl : public p4::v1::P4Runtime::Service {
230230
const std::vector<std::pair<std::string, std::string>>& cpu_queue_ids,
231231
const std::vector<std::pair<std::string, std::string>>&
232232
front_panel_queue_ids,
233-
const std::optional<int>& device_id,
234-
const std::vector<std::string>& ports)
233+
const std::optional<int>& device_id)
235234
ABSL_LOCKS_EXCLUDED(server_state_lock_);
236235

237236
grpc::Status GrabLockAndEnterCriticalState(absl::string_view message)

‎p4rt_app/tests/forwarding_pipeline_config_test.cc‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,79 @@ TEST_F(ReconcileAndCommitTest, SetDuplicateForwardingPipelineConfig) {
640640
EXPECT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
641641
}
642642

643+
TEST_F(ReconcileAndCommitTest, DeletingEmptyFixedTablesIsAllowed) {
644+
auto request = GetBasicForwardingRequest();
645+
request.set_action(SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT);
646+
*request.mutable_config()->mutable_p4info() =
647+
sai::GetP4Info(sai::Instantiation::kMiddleblock);
648+
649+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
650+
651+
auto& tables = *request.mutable_config()->mutable_p4info()->mutable_tables();
652+
for (auto table = tables.begin(); table != tables.end();) {
653+
if (absl::StartsWith(table->preamble().alias(), "acl_") ||
654+
AliasesToKeep().contains(table->preamble().alias())) {
655+
++table;
656+
} else {
657+
table = tables.erase(table);
658+
}
659+
}
660+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
661+
}
662+
663+
TEST_F(ReconcileAndCommitTest, AddingFixedTablesIsAllowed) {
664+
auto request = GetBasicForwardingRequest();
665+
request.set_action(SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT);
666+
*request.mutable_config()->mutable_p4info() =
667+
sai::GetP4Info(sai::Instantiation::kMiddleblock);
668+
669+
// Apply config without non-acl tables.
670+
auto& tables = *request.mutable_config()->mutable_p4info()->mutable_tables();
671+
for (auto table = tables.begin(); table != tables.end();) {
672+
if (absl::StartsWith(table->preamble().alias(), "acl_") ||
673+
AliasesToKeep().contains(table->preamble().alias())) {
674+
++table;
675+
} else {
676+
table = tables.erase(table);
677+
}
678+
}
679+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
680+
681+
// Apply config with fixed tables.
682+
*request.mutable_config()->mutable_p4info() =
683+
sai::GetP4Info(sai::Instantiation::kMiddleblock);
684+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
685+
}
686+
687+
TEST_F(ReconcileAndCommitTest, ModifyingEmptyFixedTablesIsAllowed) {
688+
auto request = GetBasicForwardingRequest();
689+
request.set_action(SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT);
690+
*request.mutable_config()->mutable_p4info() =
691+
sai::GetP4Info(sai::Instantiation::kMiddleblock);
692+
693+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
694+
695+
// Remove some fixed table match fields.
696+
auto& tables = *request.mutable_config()->mutable_p4info()->mutable_tables();
697+
for (auto& table : tables) {
698+
if (absl::StartsWith(table.preamble().alias(), "acl")) continue;
699+
if (table.match_fields().size() < 2) continue; // All tables need a match.
700+
auto constraints = pdpi::GetAnnotationBody("entry_restriction",
701+
table.preamble().annotations());
702+
for (auto match_field = table.mutable_match_fields()->begin();
703+
match_field != table.mutable_match_fields()->end(); ++match_field) {
704+
if (AliasesToKeep().contains(match_field->name()) ||
705+
(constraints.ok() &&
706+
absl::StrContains(*constraints, match_field->name()))) {
707+
continue;
708+
}
709+
table.mutable_match_fields()->erase(match_field);
710+
break;
711+
}
712+
}
713+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(request));
714+
}
715+
643716
using GetForwardingConfigTest = ForwardingPipelineConfigTest;
644717

645718
TEST_F(GetForwardingConfigTest, ReturnsNothingIfConfigHasNotBeenSet) {

‎p4rt_app/tests/warm_restart_bootup_test.cc‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,8 +303,184 @@ TEST_F(WarmRestartTest, ReconciliationSucceeds) {
303303

304304
// Reset P4RT server
305305
EXPECT_OK(ResetGrpcServerAndClient(/*is_freeze_mode=*/true));
306+
EXPECT_OK(p4rt_service_->GetP4rtServer().RebuildSwStateAfterWarmboot(
307+
{{"Ethernet0", "1"}, {"Ethernet4", "2"}},
308+
{{"CONTROLLER_PRIORITY_1", "32"}, {"CONTROLLER_PRIORITY_2", "33"}},
309+
{{"FRONT_PANEL_1", "1"}, {"FRONT_PANEL_2", "2"}}, kDeviceId));
306310
// State Verification
307311
EXPECT_OK(p4rt_service_->GetP4rtServer().VerifyState());
312+
// Presence of HOST_STATS|CONFIG entry in STATE DB indicates that P4Info was
313+
// pushed before warm reboot and has been restored during warm bootup.
314+
EXPECT_OK(p4rt_service_->GetHostStatsStateDbTable().ReadTableEntry("CONFIG"));
315+
316+
// Packet I/O ports are added async during reconciliation.
317+
EXPECT_OK(p4rt_service_->GetP4rtServer().AddPacketIoPort("Ethernet0"));
318+
EXPECT_OK(p4rt_service_->GetP4rtServer().AddPacketIoPort("Ethernet4"));
319+
EXPECT_OK(p4rt_service_->GetP4rtServer().AddPacketIoPort("SEND_TO_INGRESS"));
320+
321+
// Verify that the ports are added by AddPacketIoPort during reconciliation.
322+
EXPECT_OK(p4rt_service_->GetFakePacketIoInterface().SendPacketOut(
323+
"Ethernet0", "test packet"));
324+
EXPECT_OK(p4rt_service_->GetFakePacketIoInterface().SendPacketOut(
325+
"Ethernet4", "test packet"));
326+
EXPECT_OK(p4rt_service_->GetFakePacketIoInterface().SendPacketOut(
327+
"SEND_TO_INGRESS", "test packet"));
328+
329+
// Verify that UpdateDeviceId() succeded during reconciliation.
330+
const p4::v1::Uint128 election_id = ElectionId(11);
331+
grpc::ClientContext primary_stream_context;
332+
std::unique_ptr<P4RuntimeStream> primary_stream;
333+
ASSERT_OK_AND_ASSIGN(
334+
primary_stream,
335+
CreatePrimaryConnection(primary_stream_context, kDeviceId, election_id));
336+
}
337+
338+
339+
TEST_F(WarmRestartTest, ReconciliationSucceedsWithAclEntries) {
340+
// Set forwarding config and save P4Info file
341+
SetForwardingPipelineConfigRequest pipeline_request =
342+
GetBasicForwardingRequest();
343+
pipeline_request.set_action(
344+
SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT);
345+
*pipeline_request.mutable_config()->mutable_p4info() =
346+
sai::GetP4Info(sai::Instantiation::kTor);
347+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(pipeline_request));
348+
EXPECT_THAT(GetSavedConfig(),
349+
IsOkAndHolds(EqualsProto(pipeline_request.config())));
350+
351+
ASSERT_OK_AND_ASSIGN(p4::v1::WriteRequest request,
352+
test_lib::PdWriteRequestToPi(
353+
R"pb(
354+
updates {
355+
type: INSERT
356+
table_entry {
357+
acl_pre_ingress_table_entry {
358+
match { is_ip { value: "0x1" } }
359+
priority: 10
360+
action { set_vrf { vrf_id: "vrf-1" } }
361+
}
362+
}
363+
}
364+
)pb",
365+
sai::GetIrP4Info(sai::Instantiation::kTor)));
366+
367+
// Expected P4RT AppDb entries.
368+
auto acl_entry = test_lib::AppDbEntryBuilder{}
369+
.SetTableName("ACL_ACL_PRE_INGRESS_TABLE")
370+
.SetPriority(10)
371+
.AddMatchField("is_ip", "0x1")
372+
.SetAction("set_vrf")
373+
.AddActionParam("vrf_id", "vrf-1");
374+
EXPECT_OK(
375+
pdpi::SetMetadataAndSendPiWriteRequest(p4rt_session_.get(), request));
376+
EXPECT_THAT(
377+
p4rt_service_->GetP4rtAppDbTable().ReadTableEntry(acl_entry.GetKey()),
378+
IsOkAndHolds(UnorderedElementsAreArray(acl_entry.GetValueMap())));
379+
380+
// Reset P4RT server
381+
EXPECT_OK(ResetGrpcServerAndClient(/*is_freeze_mode=*/true));
382+
// Perform reconciliation
383+
EXPECT_OK(p4rt_service_->GetP4rtServer().RebuildSwStateAfterWarmboot(
384+
{{"Ethernet4", "2"}}, {}, {}, kDeviceId));
385+
// State Verification
386+
EXPECT_OK(p4rt_service_->GetP4rtServer().VerifyState());
387+
// Presence of HOST_STATS|CONFIG entry in STATE DB indicates that P4Info was
388+
// pushed before warm reboot and has been restored during warm bootup.
389+
EXPECT_OK(p4rt_service_->GetHostStatsStateDbTable().ReadTableEntry("CONFIG"));
390+
}
391+
392+
TEST_F(WarmRestartTest, ReconciliationSucceedsWithFixedL3Entries) {
393+
// Set forwarding config and save P4Info file
394+
SetForwardingPipelineConfigRequest pipeline_request =
395+
GetBasicForwardingRequest();
396+
pipeline_request.set_action(
397+
SetForwardingPipelineConfigRequest::RECONCILE_AND_COMMIT);
398+
*pipeline_request.mutable_config()->mutable_p4info() =
399+
sai::GetP4Info(sai::Instantiation::kTor);
400+
ASSERT_OK(p4rt_session_->SetForwardingPipelineConfig(pipeline_request));
401+
EXPECT_THAT(GetSavedConfig(),
402+
IsOkAndHolds(EqualsProto(pipeline_request.config())));
403+
404+
ASSERT_OK(
405+
p4rt_service_->GetP4rtServer().AddPortTranslation("Ethernet4", "2"));
406+
// P4 write request for fixed l3 table
407+
ASSERT_OK_AND_ASSIGN(p4::v1::WriteRequest request,
408+
test_lib::PdWriteRequestToPi(
409+
R"pb(
410+
updates {
411+
type: INSERT
412+
table_entry {
413+
router_interface_table_entry {
414+
match { router_interface_id: "16" }
415+
action {
416+
set_port_and_src_mac {
417+
port: "2"
418+
src_mac: "00:02:03:04:05:06"
419+
}
420+
}
421+
}
422+
}
423+
}
424+
)pb",
425+
sai::GetIrP4Info(sai::Instantiation::kTor)));
426+
427+
// Expected P4RT AppDb entry.
428+
auto expected_entry = test_lib::AppDbEntryBuilder{}
429+
.SetTableName("FIXED_ROUTER_INTERFACE_TABLE")
430+
.AddMatchField("router_interface_id", "16")
431+
.SetAction("set_port_and_src_mac")
432+
.AddActionParam("port", "Ethernet4")
433+
.AddActionParam("src_mac", "00:02:03:04:05:06");
434+
435+
436+
EXPECT_OK(
437+
pdpi::SetMetadataAndSendPiWriteRequest(p4rt_session_.get(), request));
438+
EXPECT_THAT(
439+
p4rt_service_->GetP4rtAppDbTable().ReadTableEntry(
440+
expected_entry.GetKey()),
441+
IsOkAndHolds(UnorderedElementsAreArray(expected_entry.GetValueMap())));
442+
443+
// Reset P4RT server
444+
EXPECT_OK(ResetGrpcServerAndClient(/*is_freeze_mode=*/true));
445+
// Perform reconciliation
446+
EXPECT_OK(p4rt_service_->GetP4rtServer().RebuildSwStateAfterWarmboot(
447+
{{"Ethernet4", "2"}}, {}, {}, kDeviceId));
448+
// State Verification
449+
EXPECT_OK(p4rt_service_->GetP4rtServer().VerifyState());
450+
// Presence of HOST_STATS|CONFIG entry in STATE DB indicates that P4Info was
451+
// pushed before warm reboot and has been restored during warm bootup.
452+
EXPECT_OK(p4rt_service_->GetHostStatsStateDbTable().ReadTableEntry("CONFIG"));
453+
}
454+
455+
TEST_F(WarmRestartTest, ReconciliationFailsP4infoNotFoundAndPushed) {
456+
// The presence of HOST_STATS|CONFIG entry in STATE DB indicates that P4Info
457+
// was pushed before warm reboot.
458+
p4rt_service_->GetHostStatsStateDbTable().InsertTableEntry(
459+
"CONFIG", {{"last-configuration-timestamp",
460+
absl::StrCat(absl::ToUnixNanos(absl::Now()))}});
461+
// Reconciliation fails since P4Info is not saved in the file system.
462+
EXPECT_THAT(
463+
p4rt_service_->GetP4rtServer().RebuildSwStateAfterWarmboot({}, {}, {}, 1),
464+
StatusIs(absl::StatusCode::kInvalidArgument));
465+
// Fails since P4Info file path is not set.
466+
auto p4runtime_impl = p4rt_service_->BuildP4rtServer(P4RuntimeImplOptions{
467+
.translate_port_ids = true,
468+
});
469+
EXPECT_THAT(
470+
p4runtime_impl->RebuildSwStateAfterWarmboot({}, {}, {}, kDeviceId),
471+
StatusIs(absl::StatusCode::kFailedPrecondition));
472+
}
473+
474+
TEST_F(WarmRestartTest, ReconciliationSucceedsP4infoNotFoundAndNotPushed) {
475+
// The absence of HOST_STATS|CONFIG entry in STATE DB indicates that P4Info
476+
// wasn't pushed before warm reboot.
477+
EXPECT_THAT(
478+
p4rt_service_->GetHostStatsStateDbTable().ReadTableEntry("CONFIG"),
479+
StatusIs(absl::StatusCode::kNotFound));
480+
// P4Info reconciliation should succeed when P4Info wasn't pushed before warm
481+
// reboot, and thus it isn't present after warm reboot.
482+
EXPECT_OK(p4rt_service_->GetP4rtServer().RebuildSwStateAfterWarmboot(
483+
{{"Ethernet4", "2"}}, {}, {}, kDeviceId));
308484
}
309485

310486
TEST_F(WarmRestartTest, ReconciliationFailsWhenDbEntryInvalid) {

0 commit comments

Comments
 (0)