1410167 - change how TelemetryBotoS3CrashStorage configures its bucket

Status: RESOLVED FIXED

(Socorro :: General, task, P2)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

Right now, we specify two S3 buckets in configuration:

* resource.boto.bucket_name which covers general crash storage
* destination.storage4.bucket_name which covers the TelemetryBotoS3CrashStorage bucket

We're in the process of splitting configuration into environment/secret and behavioral configuration. This particular key is environment configuration, but the key name is behavioral. So if this key gets stored with all the environment/secret configuration and not with the behavior configuration, then we'll have a 110% [1] likelihood of disaster when we next change processor crash storage destinations.

This bug covers figuring out what to do about that.

[1] +/- 10%

Comment 1

[Will Kahn-Greene [:willkg] ET needinfo? me]

In IRC, I suggested:

option 1: Put destination.storage4.bucket_name in docker/config/local_dev.env and you'd have your version in your configuration.

option 2: Rewrite the TelemetryBotoS3CrashStorage class somehow so it gets the bucket name from configuration from a "sensible name" that isn't tied to the storage index. Maybe something like "telemetrys3crashstorage.happyfunspot"?

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

Relatedly, this affects both the local development environment as well as the docker-based infrastructure we're building.

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

Making this a P1 since it's blocking the docker infrastructure.

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

I thought of a third option:

option 3: Redo how PolyCrashStorage saves destination thing. Instead of putting it in a namespace like destination.storage4, it has that point to the namespace where the values are. So then we'd have:

destination.crashstorage_class=socorro.external.crashstorage_base.PolyCrashStorage
destination.storage_classes=socorro.external.postgresql.crashstorage.PostgreSQLCrashStorage, 
   socorro.external.boto.crashstorage.BotoS3CrashStorage, 
   socorro.external.es.crashstorage.ESCrashStorageRedactedJsonDump, 
   socorro.external.statsd.crashstorage.StatsdCrashStorage, 
   socorro.external.boto.crashstorage.TelemetryBotoS3CrashStorage
destination.storage0.namespace=destination_postgres

destination_postgres.storage_class=socorro.external.statsd.statsd_base.StatsdBenchmarkingWrapper
destination_postgres.benchmark_tag=PGBenchmarkWrite
destination_postgres.crashstorage_class=socorro.external.statsd.statsd_base.StatsdBenchmarkingWrapper
destination_postgres.statsd_prefix=processor.postgres
destination_postgres.transaction_executor_class=socorro.database.transaction_executor.TransactionExecutorWithInfiniteBackoff
destination_postgres.wrapped_crashstore=socorro.external.postgresql.crashstorage.PostgreSQLCrashStorage
destination_postgres.wrapped_object_class=socorro.external.postgresql.crashstorage.PostgreSQLCrashStorage

Then if we ever want to shuffle destinations around, it's just a question of switching namespaces around.

I don't know if this is possible in the confines of configman, but it's interesting and it'd solve most of our TelemetryBotoS3CrashStorage problem.

I'm not sure I want to spend the time to do up a new PolyCrashStorage.


Also, this option:

option 4: Redo how connection classes work--instead of having them do all the connection work, we could make them a   configuration shell around another class that isn't configman related. That'd be easier to reuse in the webapp, too, which could be nice.

This seems very doable. Might even be able to re-use what I wrote in Antenna.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

We're about to enter a change freeze and there's no way we're getting this done before that. I'll spend some time in the next month figuring out how we want to proceed and maybe preparing the way, but otherwise, this is on hold.

Given that, I'm bumping this down to P2.

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

I just fiddled with crontabber configuration things and I have a new option:

option 5: Stomp on the connection context bucket configuration option from the TelemetryS3CrashStorage so that the bucket reference_value_from is "resource.boto.telemetry" which allows the configuration option pull from "resource.boto.telemetry.bucket". That might be possible.

I want to look into this this week.

Comment 7

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/747131a53c6bbbaad239db433e482e6860e06c0a
fixes bug 1410167 - fixes TelemetryS3CrashStorage bucket_name

This allows us to set the bucket_name value without having to specify it in the
destination4 configuration key.

That makes it easier for us to move crashstorage classes around without bumping
into an environment configuration variable which will be defined in a different
place in the new infrastructure.

https://github.com/mozilla-services/socorro/commit/00182e5e1fe2bbb09b5b22528303b164b03516b0
Merge pull request #4209 from willkg/1410167-telemetry-bucket

fixes bug 1410167 - fixes TelemetryS3CrashStorage bucket_name

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

What we did is add a configuration option to TelemetryS3CrashStorage and if that's set, then it stomps on the connection context's bucket_name regardless of what that's set to.

Given that, we need to add the following configuration to -stage and -prod:

   consulate kv set socorro/common/resource.boto.telemetry_crash_bucket org-mozilla-telemetry-crashes


As a side note, UploadCrashReportJSONSchemaCronApp needs to access the same bucket. We should probably fix the configuration for that to point to the same thing so we have it set in a single place.

I'm going to reopen this bug to cover that, too.

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

I made a minor mistake, -stage and -prod have different buckets.

The -stage config is this:

   consulate kv set socorro/common/resource.boto.telemetry_crash_bucket org-allizom-telemetry-crashes

The -prod config is this:

   consulate kv set socorro/common/resource.boto.telemetry_crash_bucket org-mozilla-telemetry-crashes

I just set the config variable on -stage just now. Then I went to check the bucket and I'm not sure how I can tell if it worked or not. I'll wait until tomorrow and see if there's a 20171128 folder. If there is, then it's still saving stuff, so it's probably fine.

This hasn't been deployed to -prod, yet, so I want to wait until making the config changes there.

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

I checked the logs and turns out I had the wrong key. Yay long weekends.

Correct configuration is this:

-stage

   consulate kv set socorro/common/resource.boto.telemetry_bucket_name org-allizom-telemetry-crashes

-prod

   consulate kv set socorro/common/resource.boto.telemetry_bucket_name org-mozilla-telemetry-crashes


If I'm tailing the logs on the -stage processor node when I change the configuration, then I can see the following lines:

Nov 28 02:25:24 stage-processor-i-00a1f30cf6d4861d9 bash[1386]: 2017-11-28 02:25:24,702 INFO - processor -  - MainThread - resource.boto.telemetry_bucket_name: org-allizom-telemetry-crashes
...
Nov 28 02:25:24 stage-processor-i-00a1f30cf6d4861d9 bash[1386]: 2017-11-28 02:25:24,709 INFO - processor -  - MainThread - Using org-allizom-telemetry-crashes for TelemetryBotoS3CrashStorage bucket

Ergo, that's working correctly.

This works fine on -stage. I'll wait until it's deployed on -prod and then make the configuration change there.

Comment 11

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4224: WIP: fixes bug 1410167 - fix UploadCrashReportJSONSchemaCronApp bucket name

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

Made the configuration change in -prod, so -prod is all set now regarding telemetry_bucket_name. We can now move around the TelemetryBotoS3CrashStorage in the destination list without worrying about the bucket name it's using.

Waiting on PR 4224 and then this will be done.

Comment 13

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/4de36b6d57d9ff7e0cf9c7f2612b852162d81c0d
fixes bug 1410167 - fix UploadCrashReportJSONSchemaCronApp bucket name

This fixes the UploadCrashReportJSONSchemaCronApp cron job to use similar
logic to determine the bucket name to use to save the schema file as the
TelemetryBotoS3CrashStorage class does.

https://github.com/mozilla-services/socorro/commit/ba4280716c7585d07043457e5e82852c0cee003d
Merge pull request #4224 from willkg/1410167-cronjob-telemetry-bucket

fixes bug 1410167 - fix UploadCrashReportJSONSchemaCronApp bucket name