search

opensourcerouting / frr

Free Range Routing Protocol Suite
Other
37 stars 12 forks source link

WIP : Support for multiple pce's #17

Closed rampxxxx closed 4 years ago

rampxxxx commented 4 years ago

Signed-off-by: Javier Garcia javier.garcia@voltanet.io

bradyallenjohnson commented 4 years ago

Regarding storing the PCE with the IP/Port, dont put much effort into that, since with my new CLI changes, the PCEs will be stored with the PCE Name as the key.

Brady

On Thu, Jun 11, 2020 at 12:09 PM Sébastien Merle notifications@github.com wrote:

@sylane requested changes on this pull request.

In pathd/path_pcep.c https://github.com/opensourcerouting/frr/pull/17#discussion_r438582908:

  • if (pcep_ctrl_update_pce_options(pcep_g->fpt, 1, pce_opts))
  • int current_pcc_id = get_pcc_id_by_ip(pcep_g->fpt, &pce_addr);

Instead of doing that, I would change the PCE index to be the IP and port and use a map. Even though this shouldn't be a problem, this approach is not safe, you could have another client change the configuration at the same time and the index you get is not valid anymore.

In pathd/path_pcep.c https://github.com/opensourcerouting/frr/pull/17#discussion_r438677627:

  • i++;
  • if (i >= argc) {
  • return CMD_ERR_NO_MATCH;
  • }
  • if (IS_IPADDR_V6(&pce_addr)) {
  • if (!inet_pton(AF_INET6, argv[i]->arg, &pce_addr.ipaddr_v6)) {
  • return CMD_ERR_INCOMPLETE;
  • }
  • } else {
  • if (!inet_pton(AF_INET, argv[i]->arg, &pce_addr.ipaddr_v4)) {
  • return CMD_ERR_INCOMPLETE;
  • }
  • }
  • int current_pcc_id = get_pcc_id_by_ip(pcep_g->fpt, &pce_addr);

Same thing,

In pathd/path_pcep_controller.c https://github.com/opensourcerouting/frr/pull/17#discussion_r438678518:

@@ -191,6 +191,7 @@ int pcep_ctrl_initialize(struct thread_master main_thread, ctrl_state->pcc_count = 0; ctrl_state->pcc_opts = XCALLOC(MTYPE_PCEP, sizeof(ctrl_state->pcc_opts));

  • memset(ctrl_state->pcc, 0, sizeof(ctrl_state->pcc[0]) * MAX_PCC);

XCALLOC give you zeroed memory, the C means "cleared", so I don't think the memset is needed.

In pathd/path_pcep_controller.c https://github.com/opensourcerouting/frr/pull/17#discussion_r438679879:

@@ -710,6 +713,7 @@ int pcep_thread_event_pathd_event(struct ctrl_state *ctrl_state,

for (i = 0; i < ctrl_state->pcc_count; i++) { struct pcc_state *pcc_state = ctrl_state->pcc[i];

  • path->is_delegated = is_best_pce(ctrl_state, i);

We want the pcc to do this, the controller shouldn't be responsible for that. The controller may elect a pcc and call some function like pcep_pcc_set_primary(bool is_primary) and then the pcc do the path customization when required.

In pathd/path_pcep_controller.c https://github.com/opensourcerouting/frr/pull/17#discussion_r438680726:

@@ -753,7 +757,43 @@ int pcep_main_event_handler(struct thread *thread)

/ ------------ Helper functions ------------ / +int is_best_pce(struct ctrl_state *ctrl_state, int pce)

Instead of doing that, the controller should elect a pcc every time a new one appears or one of them fail to connect for a maximum number or retries/time, and update them by calling some pcc api function, like the last comment.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/opensourcerouting/frr/pull/17#pullrequestreview-428643063, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALEQRHLJEDDNTAUH53NCXDRWCUMXANCNFSM4NPZIAKA .

rampxxxx commented 4 years ago

Closed because another PR #21 was merged