Skip to content
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

fix(orc8r): Update gateway device information during registration #12022

Merged
merged 1 commit into from Mar 21, 2022

Conversation

christinewang5
Copy link
Contributor

Previously, the gateway was registered to the device service but the gateway itself did not have the updated device info (i.e. hardware id and challenge key, etc.). This caused checkin_cli.py to fail bc the gateway was half registered.

Fixed so that the device field is updated upon calling register.py

Tested by spinning up orc8r, this was after running the register.py script
image

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Mar 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added component: cwf component: orc8r Orchestrator-related issue labels Mar 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

feg-workflow

    2 files  202 suites   33s ⏱️
363 tests 363 ✔️ 0 💤 0
377 runs  377 ✔️ 0 💤 0

Results for commit cc9780c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

cloud-workflow

1 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0

Results for commit cc9780c.

♻️ This comment has been updated with latest results.

@christinewang5 christinewang5 changed the title feat(orc8r): Update gateway device information during registration fix(orc8r): Update gateway device information during registration Mar 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

agw-workflow

  28 files    43 suites   3m 39s ⏱️
738 tests 729 ✔️ 9 💤 0
739 runs  730 ✔️ 9 💤 0

Results for commit cc9780c.

♻️ This comment has been updated with latest results.

@github-actions github-actions bot added the component: feg FEG-gateway related issues label Mar 9, 2022
@christinewang5 christinewang5 marked this pull request as ready for review March 9, 2022 16:34
@christinewang5 christinewang5 requested review from a team and hcgatewood March 9, 2022 16:34
Copy link
Contributor

@hcgatewood hcgatewood left a comment

Choose a reason for hiding this comment

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

Looks good! 2 small things

Comment on lines +137 to +140
ent, err := configurator.LoadEntity(
ctx,
networkID, orc8r.MagmadGatewayType, gatewayID,
configurator.EntityLoadCriteria{},
serdes.Entity,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a "load entity by physical ID" func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the func LoadEntityForPhysicalID gets the entity by the physical ID, but since the gateway is not yet registered to a physical ID, idt it would work... unless there is some load entity by physical ID func u are referencing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh true you're right, I misread "gateway ID" as meaning "hardware ID" -- a +1 for why functions with large # of arguments is an anti-pattern 🙂

Comment on lines 149 to 152
status, err := wrappers.GetGatewayStatus(ctx, networkID, ent.PhysicalID)
if err != nil && err != merrors.ErrNotFound {
return err
}
gw := (&models.MagmadGateway{}).FromBackendModels(ent, device, status)

_, err = configurator.UpdateEntities(ctx, networkID, gw.ToEntityUpdateCriteria(ent), serdes.Entity)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to do this with the "mutable" version of the gateway object -- i.e. without having to pull/integrate gateway state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referencing how the update gateway handlers does it here https://github.com/christinewang5/magma/blob/cc9780ca2f8bef44d42a034ac744af07b7a445aa/orc8r/cloud/go/services/orchestrator/obsidian/handlers/gateway_handlers.go#L340-L362

But I was searching for the mutable version and it seems like there are only models for specific gateway types like cwf, lte, fed, etc. Lmk if there is a way to get the mutable version just for a generic gateway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah true weird that there isn't a mutable version of the magmad gateway. So let's still do it this way, except -- we can pass in the "status" as nil, since we don't care about that when we update the ent

@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Mar 14, 2022
@github-actions github-actions bot removed the component: feg FEG-gateway related issues label Mar 14, 2022
Copy link
Contributor

@hcgatewood hcgatewood left a comment

Choose a reason for hiding this comment

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

Lgtm, just fix the 1 thing where we don't need to read in the gw status

@christinewang5 christinewang5 force-pushed the register-device branch 3 times, most recently from 5f36696 to be6642f Compare March 16, 2022 16:08
@hcgatewood hcgatewood enabled auto-merge (squash) March 17, 2022 20:36
Signed-off-by: Christine Wang <christinewang@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: cwf component: orc8r Orchestrator-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants