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

[lte] Add per-APN static IP configuration to subscribers on API #2169

Merged
merged 1 commit into from Aug 6, 2020

Conversation

xjtian
Copy link
Contributor

@xjtian xjtian commented Aug 4, 2020

Signed-off-by: Jacky Tian xjtian@fb.com

Summary

  • Add the capability to assign APN-specific static IPs to subscribers
  • This introduces a new config field on the subscriber entity which encapsulates the LteSubscription and the static IP config.
  • To maintain backwards compatibility with API clients, the lte field is still on the subscriber entity on the read side (GET). On the write side, we add the static IP configuration as a top-level field to the MutableSubscriber.
  • Because this changes the network entity in configurator, a data migration is also included.

Test Plan

  • New unit tests for API handlers
  • Ran locally; API errored out before migration, worked after migration. Also tested migration script on dump of staging DB.
  • Tested and verified subscriber streaming on local AGW:
vagrant@magma-dev:~$ subscriber_cli.py get IMSI208950000000012
sid {
  id: "208950000000012"
}
lte {
  state: ACTIVE
  auth_key: "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
  auth_opc: "\000\001\002\003\004\005\006\007\010\t\n\013\014\r\016\017"
}
network_id {
  id: "test"
}
state {
}
sub_profile: "default"
non_3gpp {
  apn_config {
    service_selection: "inet"
    qos_profile {
      class_id: 9
      priority_level: 15
      preemption_capability: true
    }
    ambr {
      max_bandwidth_ul: 10000000
      max_bandwidth_dl: 20000000
    }
    assigned_static_ip: "192.168.100.1"
  }
}

Additional Information

  • This change is backwards-breaking

After deploying orc8r, kubectl exec into any controller pod, then:

$ cd /var/opt/magma/bin
$ envdir /var/opt/magma/envdir ./m011_subscriber_config

@xjtian xjtian added the breaking change Backwards-breaking changes that will require migration steps label Aug 4, 2020
@hcgatewood
Copy link
Contributor

I think this is a good pattern to include the data migration with the relevant breaking changes

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.

Couple small things

// Rollback the tx immediately so the DB engine doesn't have to
// wait for the conn to close
_ = tx.Rollback()
glog.Fatalf("recovered from panic: %v", r)
Copy link
Contributor

Choose a reason for hiding this comment

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

recovered -> Recovered

glog.Fatal(errors.Wrap(err, "could not open db connection"))
}

_, err = migrations.ExecInTx(db, &sql.TxOptions{Isolation: sql.LevelSerializable}, nil, doMigration)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this pattern of having just a single doMigration in main's ExecInTx -- easier to follow the logic

Signed-off-by: Jacky Tian <xjtian@fb.com>
@xjtian xjtian merged commit 763745e into magma:master Aug 6, 2020
@xjtian xjtian deleted the static_sub_ips branch August 13, 2020 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Backwards-breaking changes that will require migration steps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants