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.

Sign up for GitHub

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

Jump to bottom

feat(dp): Enodebd triggered update #12804

Merged

WojSad merged 6 commits into magma:master from openEPC:enodebd-triggered-update

May 31, 2022

Contributor

WojSad commented May 20, 2022 •

edited

feat(dp): Enodebd triggered update

Summary

DP now has two grpc methods to update a cbsd (UserUpdateCbsd and EnodebdUpdateCbsd)
The separation was needed for the purpose of single-step registration (under development). Enodebd needs to be able to update a cbsd separately with installation param, radio category and single_step_enabled flag.
EnodebdUpdateCbsd receives single-step related information from enodebd and updates the cbsd in the db by calling the storage side Update method.

Introduced data builders for tests to make writing mocked entities and models easier
Refactored tests
Updated protos and swaggers

Introduced select for update in the sqorc library

Test Plan

All tests related to Cbsd CRUD have been refactored, new tests were added.

WojSad requested review from a team, xbend and maxhbr

May 20, 2022 13:30

netlify bot commented May 20, 2022 •

edited

👷 Deploy request for competent-euler-513227 pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`ecf40e4`

pull-request-size bot added the size/XXL label

Contributor

github-actions bot commented May 20, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

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

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

github-actions bot added component: dp component: nms component: orc8r labels

github-actions bot reviewed

View reviewed changes

orc8r/cloud/go/obsidian/swagger/v1/swagger.yml

+                      description: AGL - relative to the ground level. AMSL - relative to the mean
+                        sea level
+                      enum:
+                      - agl

Contributor

github-actions bot May 20, 2022

[yamllint] _{reported by reviewdog 🐶}
[error] wrong indentation: expected 10 but found 8 (indentation)

Contributor

github-actions bot commented May 20, 2022 •

edited

Oops! Looks like you failed the Python Format Check.

Howto

Instructions on running the formatter and linter locally are provided in the format AGW doc
Guide to the different CI checks and resolution guidelines

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

WojSad force-pushed the enodebd-triggered-update branch from 550f354 to c4d8557 Compare

May 20, 2022 13:37

Contributor

github-actions bot commented May 20, 2022 •

edited

feg-workflow

    2 files 202 suites 38s ⏱️
371 tests 371 ✔️ 0 💤 0 ❌
385 runs 385 ✔️ 0 💤 0 ❌

Results for commit a821237.

♻️ This comment has been updated with latest results.

Contributor

github-actions bot commented May 20, 2022 •

edited

nms-workflow

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

Results for commit a821237.

♻️ This comment has been updated with latest results.

Contributor

github-actions bot commented May 20, 2022 •

edited

cloud-workflow

    7 files 355 suites 2m 15s ⏱️
979 tests 979 ✔️ 0 💤 0 ❌

Results for commit a821237.

♻️ This comment has been updated with latest results.

Contributor

github-actions bot commented May 20, 2022 •

edited

agw-workflow

    77 files   122 suites 6m 36s ⏱️
1 151 tests 1 147 ✔️ 4 💤 0 ❌
1 152 runs 1 148 ✔️ 4 💤 0 ❌

Results for commit a821237.

♻️ This comment has been updated with latest results.

Contributor

github-actions bot commented May 20, 2022 •

edited

dp-workflow

  2 files   2 suites 3m 10s ⏱️
15 tests 15 ✔️ 0 💤 0 ❌

Results for commit a821237.

♻️ This comment has been updated with latest results.

jkmar reviewed

View reviewed changes

dp/tools/deployment/tests/test_runner_deployment_orc8r.yml Outdated

@@ @@ -16,8 +16,8 @@ spec: @@
                   - image: test_runner_image
                     name: test-runner-orc8
                     imagePullPolicy: IfNotPresent
-                    command: ["python"]
-                    args: ["-m", "pytest", "-vvv", "-s", "-m", "orc8r", "--junit-xml=/backend/test_runner/test-results/test_report.xml", "tests"]
+                    command: ["sleep"]

Contributor

jkmar May 20, 2022

@sklgromek I think we should add some target to makefile to do that, since this is not the first time such debugging version was committed by accident

Contributor

sklgromek May 23, 2022

@jkmar I agree. I will prepare something for that

dp/cloud/go/services/dp/builders_test/builders.go Outdated

		@@ -0,0 +1,517 @@
		package builders_test

Contributor

jkmar May 20, 2022

Since this is separate package already, I would go with just builders as package name

Contributor Author

WojSad May 24, 2022

done

dp/cloud/go/services/dp/servicers/db_field_setters.go Outdated

+              	"magma/dp/cloud/go/services/dp/storage/db"
+              )
+              func dbFloat64OrNil(field *sql.NullFloat64, val *wrappers.DoubleValue) {

Contributor

jkmar May 20, 2022

How about this:

if val == nil {
    return sql.NullFloat64{}
}
return db.MakeFloat(val.Value)

Contributor Author

WojSad May 24, 2022

done

dp/cloud/go/services/dp/obsidian/cbsd/handlers_test.go Outdated

               	e := echo.New()
               	obsidianHandlers := cbsd.GetHandlers()
               	payload := &models.MutableCbsd{
-              		Capabilities: models.Capabilities{
-              			AntennaGain:      to_pointer.Float(1),
+              		Capabilities: &models.Capabilities{

Contributor

jkmar May 23, 2022

Why builder is not used here? (while it is used for same type just a few lines above) It's a bit inconsistent

dp/cloud/go/services/dp/obsidian/models/conversion_test.go Outdated

               	"magma/dp/cloud/go/services/dp/obsidian/models"
               	"magma/dp/cloud/go/services/dp/obsidian/to_pointer"
+              	"github.com/stretchr/testify/assert"

Contributor

jkmar May 23, 2022

go imports

Contributor Author

WojSad May 24, 2022

done

dp/cloud/go/services/dp/obsidian/models/conversion_test.go Outdated

+              func TestCbsdFromBackendWithEmptyFrequencies(t *testing.T) {
+              	details := b.NewDetailedProtoCbsdBuilder(
+              		b.NewCbsdProtoPayloadBuilder()).Details
+              	details.Data.Preferences.FrequenciesMhz = nil

Contributor

jkmar May 23, 2022

Why is it not done with builder?

Contributor Author

WojSad May 24, 2022

done

dp/cloud/go/services/dp/obsidian/models/validation_test.go

-              	}, {
-              		name:          "Should validate antenna gain",
-              		data:          newMutableCbsd(withAntennaGain(nil)),
-              		expectedError: "antenna_gain in body is required",
               	}, {

Contributor

jkmar May 23, 2022

How about adding some tests to validate installation params?

Contributor Author

WojSad May 24, 2022

done, validated height type since this is the only enum

dp/cloud/go/services/dp/storage/cbsd_manager.go Outdated

@@ @@ -46,6 +47,16 @@ type MutableCbsd struct { @@
               	DesiredState *DBCbsdState
               }
+              type EnodebdPayload struct {

Contributor

jkmar May 23, 2022 •

edited

Why is this exported? (It is unused)

Contributor Author

WojSad May 24, 2022

deleted

dp/cloud/go/services/dp/storage/cbsd_manager.go

-              func (c *cbsdManagerInTransaction) checkIfCbsdExists(networkId string, id int64) error {
-              	_, err := db.NewQuery().
+              func (c *cbsdManagerInTransaction) enodebdUpdateCbsd(data *DBCbsd) error {
+              	mask := db.NewIncludeMask("id")

Contributor

jkmar May 23, 2022

This should be a broader mask, so that comparison of installation params is possible

Contributor Author

WojSad May 24, 2022

as agreed, will do in a later pr

jkmar approved these changes

View reviewed changes

WojSad force-pushed the enodebd-triggered-update branch 3 times, most recently from e8e3fbd to 291b8ec Compare

May 24, 2022 11:31

Contributor Author

WojSad commented May 24, 2022

@magma/approvers-orc8r please have a look at this PR. The orc8r part affected is the SQL LOCKER bit in sqorc library

xbend added the ready2merge label

Contributor

xbend commented May 25, 2022

@maxhbr
FYI, this PR includes the changes that were already contained in a PR you have reviewed, but it was suggested to have those changes be pushed together with a code that uses it, so those changes are included here

xbend mentioned this pull request

feat(enodebd): Send CBSD parameters to Domain Proxy #12850

Merged

1 task

Contributor

jpad-freedomfi commented May 27, 2022

@maxhbr Can you provide review for this so we can merge?

Contributor

xbend commented May 30, 2022

@magma/approvers-orc8r
Bump, can anybody from orc8r approves take a look at the sqorc changes?

maxhbr approved these changes

View reviewed changes

Member

maxhbr left a comment

The orc8r part loos good

Wojciech Sadowy and others added 6 commits

May 31, 2022 10:31


          feat(dp): CBSD update modification

a82e6ce

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>


          chore(dp): Add lock to db framework

45a136d

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>


          chore(orc8r): Add sql locker to sqorc lib

6f3ce11

Not all sql dialects support locking feature (SELECT FOR UPDATE),
so in order to use locking for different dialects,
a locker interface was introduced.

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>


          fixed tests

ad523af

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>


          moved pointer converters

43468a1

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>


          fixes

a821237

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

WojSad force-pushed the enodebd-triggered-update branch from 291b8ec to a821237 Compare

May 31, 2022 08:31

WojSad enabled auto-merge (squash)

May 31, 2022 09:14

WojSad merged commit 72e819c into magma:master

mpfirrmann pushed a commit to mpfirrmann/magma that referenced this pull request


          feat(dp): Enodebd triggered update (magma#12804)

deaa6d7

* feat(dp): CBSD update modification

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* chore(dp): Add lock to db framework

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>

* chore(orc8r): Add sql locker to sqorc lib

Not all sql dialects support locking feature (SELECT FOR UPDATE),
so in order to use locking for different dialects,
a locker interface was introduced.

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>

* fixed tests

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* moved pointer converters

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* fixes

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

Co-authored-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>
Co-authored-by: Kuba Marciniszyn <kuba@freedomfi.com>

WojSad deleted the enodebd-triggered-update branch

June 28, 2022 13:05

emakeev pushed a commit to emakeev/magma that referenced this pull request


          feat(dp): Enodebd triggered update (magma#12804)

c7a1b3d

* feat(dp): CBSD update modification

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* chore(dp): Add lock to db framework

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>

* chore(orc8r): Add sql locker to sqorc lib

Not all sql dialects support locking feature (SELECT FOR UPDATE),
so in order to use locking for different dialects,
a locker interface was introduced.

Signed-off-by: Kuba Marciniszyn <kuba@freedomfi.com>

* fixed tests

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* moved pointer converters

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

* fixes

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

Co-authored-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>
Co-authored-by: Kuba Marciniszyn <kuba@freedomfi.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sklgromek sklgromek left review comments

github-actions[bot] github-actions left review comments

maxhbr maxhbr approved these changes

jkmar jkmar approved these changes

xbend Awaiting requested review from xbend