Skip to content

[Orc8r] Multiple Orc8r deployment changes including changes to metrics helm chart, elastic search curator chart values and orcl#7717

Merged
karthiksubraveti merged 1 commit intomagma:masterfrom
karthiksubraveti:orc8r_db.0
Jun 24, 2021
Merged

Conversation

@karthiksubraveti
Copy link
Copy Markdown
Contributor

@karthiksubraveti karthiksubraveti commented Jun 23, 2021 •
edited
Loading

Signed-off-by: Karthik Subraveti [email protected]

Summary

This PR contains following changes

Helm chart changes

  • Upgraded prometheus helm chart to enable web admin api. This is required for us to take snapshots and cleanup timeseries

Terraform changes

  • Added es_disk_threshold to set the size threshold to trigger elastic search curator to cleanup magma log indices
  • Upgraded elastic search curator chart from 2.1.3 -> 2.2.3

Orcl changes

  • Orcl - Added RDS version precheck to prevent upgrade failure to an incompatible DB version
  • Orcl - Added RDS precheck to ensure instance class is available in the region where orc8r is being deployed
  • Cleaned up Orcl docker image(added go and testlib binaries only when built with appropriate switch)
  • Minor bugfixes in configlib code

Test Plan

  • orcl checks ran fine
  • elastic search curator ran fine and was able to identify the right indices to look for cleanup

Additional Information

  • This change is backwards-breaking

@karthiksubraveti karthiksubraveti requested review from a team and andreilee June 23, 2021 00:04
@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Jun 23, 2021
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 23, 2021

Thanks for opening a PR! 💯 Please note that all commits must be signed off. This is enforced by the DCO check.

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

@karthiksubraveti karthiksubraveti force-pushed the orc8r_db.0 branch 2 times, most recently from 12ce2af to 9ac86b2 Compare June 23, 2021 00:17
@karthiksubraveti
Add minor Orc8r deployment changes.
f4a3905 
Signed-off-by: Karthik Subraveti <[email protected]>
@karthiksubraveti karthiksubraveti changed the title [Orc8r] Add minor Orc8r deployment changes. [Orc8r] Multiple Orc8r deployment changes including changes to metrics helm chart, elastic search curator chart values and orcl Jun 23, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 23, 2021 •
edited
Loading

Codecov Report

Merging #7717 (f4a3905) into master (15f09de) will increase coverage by 30.20%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7717       +/-   ##
===========================================
+ Coverage   34.20%   64.41%   +30.20%     
===========================================
  Files        1176      680      -496     
  Lines      105747    47109    -58638     
  Branches     1323     1323               
===========================================
- Hits        36171    30343     -5828     
+ Misses      66113    13304    -52809     
+ Partials     3463     3462        -1
Flag Coverage Δ
c_cpp ?
cloud_lint 65.73% <ø> (+0.01%) ⬆️
feg-lint 56.31% <ø> (ø)
lte-test 72.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lte/gateway/c/session_manager/CreditKey.h
lte/gateway/c/core/oai/common/TLVDecoder.c
lte/gateway/c/session_manager/SessionState.cpp
...te/gateway/c/session_manager/SpgwServiceClient.cpp
lte/gateway/c/core/oai/tasks/s6a/s6a_purge_ue.c
...gateway/c/core/oai/tasks/s6a/s6a_service_handler.c
...ateway/c/session_manager/datastore/Serializers.cpp
lte/gateway/c/core/oai/tasks/nas/ies/Cli.c
...eway/c/core/oai/tasks/nas/ies/SupportedCodecList.c
...way/c/core/oai/tasks/nas/emm/msg/IdentityRequest.c
... and 487 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15f09de...f4a3905. Read the comment docs.

@karthiksubraveti karthiksubraveti enabled auto-merge (squash) June 23, 2021 05:27
hcgatewood
hcgatewood approved these changes Jun 24, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Tf and Helm chart changes lgtm. Not following the deployer/etc code changes

Comment thread orc8r/cloud/deploy/orc8r_deployer/docker/Dockerfile
@@ -1,5 +1,5 @@
FROM python:3.9-slim-buster

ARG ENV=prod
Copy link
Copy Markdown
Contributor

@hcgatewood hcgatewood Jun 24, 2021

Choose a reason for hiding this comment

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

This is a dangerous pattern in general. The default should be prod -- and we only do things differently if ENV=test

Copy link
Copy Markdown
Contributor Author

@karthiksubraveti karthiksubraveti Jun 24, 2021

Choose a reason for hiding this comment

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

Actually the "prod" name doesn't make sense here. I should have named this differently. But the default is indeed the docker contained without binaries used for test. Will clean it up in a follow up PR.

Comment thread orc8r/cloud/deploy/terraform/orc8r-aws/db.tf
vpc_security_group_ids = [aws_security_group.default.id]

db_subnet_group_name = module.vpc.database_subnet_group
db_subnet_group_name = module.vpc.database_subnet_group
Copy link
Copy Markdown
Contributor

@hcgatewood hcgatewood Jun 24, 2021

Choose a reason for hiding this comment

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

Throughout this PR you're introducing trailing whitespace

Copy link
Copy Markdown
Contributor Author

@karthiksubraveti karthiksubraveti Jun 24, 2021

Choose a reason for hiding this comment

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

:(. Should fix my vscode setting to eliminate this.

Comment thread orc8r/cloud/deploy/terraform/orc8r-aws/outputs.tf
}

output "es_volume_size" {
description = "Endpoint of the ES cluster if deployed."
Copy link
Copy Markdown
Contributor

@hcgatewood hcgatewood Jun 24, 2021

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Contributor Author

@karthiksubraveti karthiksubraveti Jun 24, 2021

Choose a reason for hiding this comment

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

Will fix the description here.

@karthiksubraveti karthiksubraveti merged commit e4088e0 into magma:master Jun 24, 2021
themarwhal pushed a commit to themarwhal/magma that referenced this pull request Jun 30, 2021
@karthiksubraveti @themarwhal
Add minor Orc8r deployment changes. (magma#7717)
cb3690b 
Signed-off-by: Karthik Subraveti <[email protected]>
themarwhal pushed a commit to themarwhal/magma that referenced this pull request Jun 30, 2021
@karthiksubraveti @themarwhal
Add minor Orc8r deployment changes. (magma#7717)
a2c4608 
Signed-off-by: Karthik Subraveti <[email protected]>
Signed-off-by: Marie Bremner <[email protected]>
themarwhal pushed a commit to themarwhal/magma that referenced this pull request Jun 30, 2021
@karthiksubraveti @themarwhal
Add minor Orc8r deployment changes. (magma#7717)
542b9d0 
Signed-off-by: Karthik Subraveti <[email protected]>
Signed-off-by: Marie Bremner <[email protected]>
themarwhal added a commit that referenced this pull request Jun 30, 2021
@themarwhal @karthiksubraveti
Add minor Orc8r deployment changes. (#7717) (#7849)
3ccb023 
Co-authored-by: Karthik Subraveti <[email protected]>
@themarwhal
Copy link
Copy Markdown
Member

themarwhal commented Jun 30, 2021

backported here: #7849

rmeleromira pushed a commit to rmeleromira/magma that referenced this pull request Jul 24, 2021
@karthiksubraveti @rmeleromira
Add minor Orc8r deployment changes. (magma#7717)
6a2bf1c 
Signed-off-by: Karthik Subraveti <[email protected]>
Signed-off-by: Ramon Melero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreilee andreilee Awaiting requested review from andreilee

1 more reviewer

@hcgatewood hcgatewood hcgatewood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/L Denotes a Pull Request that changes 100-499 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@karthiksubraveti @themarwhal @hcgatewood