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

compose.yaml: autopair first-party leader #1099

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

tgeoghegan
Copy link
Contributor

To enable the demo workflow, users will need to pair two aggregators with the control plane. Further, one of them must be first-party, which can only be done using a CLI built with feature admin. But the one we distribute via GitHub releases isn't, and I argue it shouldn't be, which means that demo users can't appropriately pair aggregators.

We could provide a curl or wget command that'd do it, but as it turns out, we already bundle the divviup CLI in the divviup_api container, so let's just use that, since the API URL and token for the aggregators are static!

I'm thinking we should have the demo user pair the other aggregator themselves with divviup to simulate what they might have to do in a realistic use case, if they bring their own helper.

This commit:

  • adds feature admin to builds of divviup_api_integration_test in docker-release.yaml so that the bundled CLI will have --first-party and --shared
  • adds a service pair_aggregator to compose.yaml that uses the /divviup entrypoint to pair janus_1_aggregator as a first-party, shared aggregator
  • adds a stanza for pair_aggregator to compose.dev.override.yaml
  • makes compose.dev.override.yaml build all images with features integration_test,admin

Part of #1096

@tgeoghegan tgeoghegan requested a review from a team as a code owner June 14, 2024 00:31
Comment on lines +65 to +68
- --api-url=http://janus_1_aggregator:8080/aggregator-api
- --bearer-token=0000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice to not duplicate these constants but YAML 🤷🏻

@tgeoghegan
Copy link
Contributor Author

The compose test is failing, because it uses divviup_api_integration_test:0.3.12, which was not built with feature admin, so divviup aggregator create fails. I'll have to break this into a couple changes: first add the feature flag to docker-release.yml, then cut a release, then bump to that version in compose.yaml and add the pair_aggregator service. I will juggle those PRs tomorrow.

Copy link
Contributor

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

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

This PR looks good in terms of functionality.

I'm thinking we should have the demo user pair the other aggregator themselves with divviup to simulate what they might have to do in a realistic use case, if they bring their own helper.

I think it would be nice to have the system preconfigured -- i.e. all aggregators paired -- to make it as easy as possible for folks to get up & running. Our demo script can call out that this will be a required step in a real deployment, maybe eventually linking out to documentation describing how the pairing is done.

I'd even go so far as to say we should create a preconfigured task, maybe providing a client config that makes it easy to start submitting data. Though that might be inflexible if e.g. the user wants to play around with a different VDAF.

tgeoghegan added a commit that referenced this pull request Jun 14, 2024
We want the `divviup` CLI packaged into the
`divviup_api_integration_test` container to be built with feature
`admin` for reasons discussed in #1099. In this commit:

- add feature `admin` to builds of `divviup_api_integration_test` in
  `docker-release.yaml` so that the bundled CLI will have
  `--first-party` and `--shared`
- adds a service `pair_aggregator` to `compose.dev.override.yaml` that
  uses the `/divviup` entrypoint to pair `janus_1_aggregator` as a
  first-party, shared aggregator
- factors the `build` stanza out of services in
  `compose.dev.override.yaml` to apply features to them uniformly
tgeoghegan added a commit that referenced this pull request Jun 14, 2024
We want the `divviup` CLI packaged into the
`divviup_api_integration_test` container to be built with feature
`admin` for reasons discussed in #1099. In this commit:

- add feature `admin` to builds of `divviup_api_integration_test` in
  `docker-release.yaml` so that the bundled CLI will have
  `--first-party` and `--shared`
- adds a service `pair_aggregator` to `compose.dev.override.yaml` that
  uses the `/divviup` entrypoint to pair `janus_1_aggregator` as a
  first-party, shared aggregator
- factors the `build` stanza out of services in
  `compose.dev.override.yaml` to apply features to them uniformly

Part of #1096
tgeoghegan added a commit that referenced this pull request Jun 14, 2024
We want the `divviup` CLI packaged into the
`divviup_api_integration_test` container to be built with feature
`admin` for reasons discussed in #1099. In this commit:

- add feature `admin` to builds of `divviup_api_integration_test` in
  `docker-release.yaml` so that the bundled CLI will have
  `--first-party` and `--shared`
- adds a service `pair_aggregator` to `compose.dev.override.yaml` that
  uses the `/divviup` entrypoint to pair `janus_1_aggregator` as a
  first-party, shared aggregator
- factors the `build` stanza out of services in
  `compose.dev.override.yaml` to apply features to them uniformly
- tunes the health checks on `divviup-api` and the aggregators so they
  will succeed sooner and speed up startup

Part of #1096
@tgeoghegan
Copy link
Contributor Author

#1100 adds the build flags.

I think it would be nice to have the system preconfigured -- i.e. all aggregators paired -- to make it as easy as possible for folks to get up & running. Our demo script can call out that this will be a required step in a real deployment, maybe eventually linking out to documentation describing how the pairing is done.

Sure!

I'd even go so far as to say we should create a preconfigured task, maybe providing a client config that makes it easy to start submitting data. Though that might be inflexible if e.g. the user wants to play around with a different VDAF.

I prefer making them create the task. If we preload one, its ID will be random anyway, so they'll have to do a divviup task list which is barely any easier than divviup task create. Plus, creating the task themselves then makes it obvious how they could tweak the divviup task create to do things like create a different histogram.

tgeoghegan added a commit that referenced this pull request Jun 14, 2024
We want the `divviup` CLI packaged into the
`divviup_api_integration_test` container to be built with feature
`admin` for reasons discussed in #1099. In this commit:

- add feature `admin` to builds of `divviup_api_integration_test` in
  `docker-release.yaml` so that the bundled CLI will have
  `--first-party` and `--shared`
- adds a service `pair_aggregator` to `compose.dev.override.yaml` that
  uses the `/divviup` entrypoint to pair `janus_1_aggregator` as a
  first-party, shared aggregator
- factors the `build` stanza out of services in
  `compose.dev.override.yaml` to apply features to them uniformly
- tunes the health checks on `divviup-api` and the aggregators so they
  will succeed sooner and speed up startup

Part of #1096
To enable the demo workflow, users will need to pair two aggregators
with the control plane. Further, one of them must be first-party, which
can only be done using a CLI built with feature `admin`. But the one we
distribute via GitHub releases isn't, and I argue it shouldn't be, which
means that demo users can't appropriately pair aggregators.

We could provide a `curl` or `wget` command that'd do it, but as it
turns out, we already bundle the `divviup` CLI in the `divviup_api`
container, so let's just use that, since the API URL and token for the
aggregators are static!

I'm thinking we should have the demo user pair the other aggregator
themselves with `divviup` to simulate what they might have to do in a
realistic use case, if they bring their own helper.

Building on previous changes, this commit moves the `pair_aggregator`
service into the main `compose.yaml`, leaving `build` and `develop`
stanzas for it in `compose.dev.override.yaml`. Additionally, the
`divviup_api_integration_test` image version is bumped to one built with
feature `admin`, which should get the tests working.

Part of #1096
@tgeoghegan tgeoghegan force-pushed the timg/auto-pair-aggregator branch from 7e6fb41 to bcc9547 Compare June 14, 2024 22:25
@tgeoghegan tgeoghegan merged commit 682f501 into main Jun 14, 2024
8 checks passed
@tgeoghegan tgeoghegan deleted the timg/auto-pair-aggregator branch June 14, 2024 22:37
@tgeoghegan tgeoghegan mentioned this pull request Jun 14, 2024
9 tasks
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.

3 participants