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

RT archiver v3 #1614

Merged
merged 55 commits into from
Jul 27, 2022
Merged

RT archiver v3 #1614

merged 55 commits into from
Jul 27, 2022

Conversation

atvaccaro
Copy link
Contributor

@atvaccaro atvaccaro commented Jul 5, 2022

Description

Tech spec

Corresponding calitp-py PR

Calling this v3 because the current archiver is actually on v2.1! We already have a v3 node pool so the new node pool is v4. Versions are fun.

TODO:

  • Decide how we want to validate new Airtable config data
  • Use the secrets manager k8s plugin SDK on startup to replace the manual temporary secret
  • Adjust metrics; add informative labels (status code, response headers, RT feed type); do not have separate success/failure metrics
  • Set up CI/CD (also decide if we want a pre-prod) going to do this later, @lottspot has some WIP CI/CD changes
  • Consider changing partition scheme; maybe should be hour/tick/url/?
  • Consider setting up Prom alerts

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • agencies.yml

How has this been tested?

https://monitoring.k8s.calitp.jarv.us/d/N1XVLOe7z/gtfs-rt-archiver-v3
https://monitoring.k8s.calitp.jarv.us/d/xPWY4jL7z/gtfs-rt-archiver-resource-consumption?orgId=1&refresh=5s&var-namespace=gtfs-rt-v3

Screenshots (optional)

@atvaccaro atvaccaro force-pushed the rt-v2-experiments branch from 5375535 to 429368d Compare July 13, 2022 21:50
@atvaccaro atvaccaro changed the title start experimenting with task queue options and metrics RT archiver v2 Jul 15, 2022
@atvaccaro atvaccaro force-pushed the rt-v2-experiments branch from e82b74e to d0bf9f7 Compare July 15, 2022 20:14
@atvaccaro atvaccaro changed the title RT archiver v2 RT archiver v3 Jul 23, 2022
@atvaccaro atvaccaro self-assigned this Jul 23, 2022
@atvaccaro atvaccaro changed the base branch from main to c2-rt July 23, 2022 16:59
memory: 512Mi
cpu: 1
limits:
memory: 1Gi
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 it would be better to use maxmemory + an eviction policy for redis... I'll take it upon myself to add this on a derivative branch today

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not hold up the merge over this-- easy change to layer on.

operator: In
values:
- gtfsrtv3
podAntiAffinity:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is clever!

Copy link
Contributor

@mjumbewu mjumbewu left a comment

Choose a reason for hiding this comment

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

question: Did you intend for something to be in services/gtfs-rt-archiver-v3/README.rst? Also, why restructured text vs markdown?

@atvaccaro
Copy link
Contributor Author

question: Did you intend for something to be in services/gtfs-rt-archiver-v3/README.rst? Also, why restructured text vs markdown?

Ah nope that's just a file that poetry automatically creates, will remove or add content.

@atvaccaro atvaccaro requested review from lottspot and mjumbewu July 27, 2022 15:44
Copy link
Contributor

@mjumbewu mjumbewu left a comment

Choose a reason for hiding this comment

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

Just a couple more thoughts below to make errors a bit more readable(?). Also, for posterity, could you link to cal-itp/calitp-py#69 from the description?

Comment on lines +63 to +71
AUTH_KEYS = [
"AC_TRANSIT_API_KEY",
"AMTRAK_GTFS_URL",
"CULVER_CITY_API_KEY",
"MTC_511_API_KEY",
"SD_MTS_SA_API_KEY",
"SD_MTS_VP_TU_API_KEY",
"SWIFTLY_AUTHORIZATION_KEY_CALITP",
]
Copy link
Contributor

@mjumbewu mjumbewu Jul 27, 2022

Choose a reason for hiding this comment

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

thought (non-blocking): These hard-coded key names bother me, but only a little. I guess on one hand they could be seen as a feature, declaring explicitly which auth keys are supported/configured. OTOH, would it be fine to just declare that in the environment and not doubly here?

If a feed maintainer (e.g. Evan) should be able to add a feed with auth without having to worry about a code change, then we could just prefix the env variables with something like ARCHIVER_AUTH__ and pull in anything from the environment with that prefix, stripping the prefix to create the auth_dict.

However, if having these explicitly laid out here is actually a feature, then it's fine.

Copy link
Contributor Author

@atvaccaro atvaccaro Jul 27, 2022

Choose a reason for hiding this comment

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

I do think it's a feature but I could be convinced otherwise. Mainly, I want a source code location that signals the API keys we will read in, and whether you need to add a missing key vs fixing an existing but misconfigured key. The k8s YAML could technically contain this but that would then require changing a kustomize file vs source code in an image.

A more specific limitation is that I don't necessarily want to iterate over and/or load all secrets from Secret Manager; there may be a way to tag/categorize?

Edit: Actually now that I'm thinking about it, I do like the idea of relying on the prefix as a signal in theory. It's just tricky since right now the code has to know the keys to fetch from Secret Manager. If we were to use k8s secrets via the plugin I think the prefix idea would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I definitely don't think this is a blocker. It's something that can be revisited if it turns out to feel inefficient.

memory: 512Mi
cpu: 1
limits:
memory: 1Gi
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not hold up the merge over this-- easy change to layer on.

Copy link
Contributor

@mjumbewu mjumbewu left a comment

Choose a reason for hiding this comment

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

Excited to see this go!

@atvaccaro
Copy link
Contributor Author

I am merging this since it's already deployed. CI/CD and Airtable validation can be follow-ups while we're kicking the tires on performance, metrics, and alerts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants