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

Update local duchy worker2 to use postgres #1160

Merged

Conversation

YuhongWang-Amazon
Copy link
Contributor

@YuhongWang-Amazon YuhongWang-Amazon commented Aug 9, 2023

Update local duchy worker2 to use postgres database

@wfa-reviewable
Copy link

This change is Reviewable

@YuhongWang-Amazon YuhongWang-Amazon changed the title Add empty cluster correctness test for postgres duchy Update worker2 of empty cluster correctness test to postgres duchy Aug 9, 2023
@YuhongWang-Amazon YuhongWang-Amazon changed the title Update worker2 of empty cluster correctness test to postgres duchy Update local duchy worker2 to use postgres Aug 10, 2023
Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 14 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: 17 of 18 files reviewed, all discussions resolved

@renjiezh
Copy link
Contributor

Hi, yuhong. Before we review this PR, could you please do some end-to-end test on GCP without merging it. This is the doc instructing deploy and test in QA environment. You could follow it to replace duchy_2 with postgres version and play around.
https://docs.google.com/document/d/1A7wXZtYWs0GLTBBZv4YEQpJ995LaHuGy5r7gWe_ZASQ/edit?resourcekey=0-yeM2zwGbYaIsPmPHgynyCw#heading=h.sjos1sttv69g

Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Sure. Will do that.
Btw, this PR didn't update any terraform and k8s/dev folder, hence all duchies in GCP should still be spanners and will not be impacted. This PR only updates the local env where I updated the worker2 to postgres.

Reviewable status: 16 of 18 files reviewed, all discussions resolved (waiting on @renjiezh)

Copy link
Contributor

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 14 files at r1, 13 of 14 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @YuhongWang-Amazon)

Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

@renjiezh Created another branch to update halo-cmm-dev worker2 to use postgres, and passed the correctness tests.. Had to fix some bugs, please take another look. Appreciated!

Reviewed 1 of 14 files at r1, 6 of 7 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @YuhongWang-Amazon)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 14 files at r1, 1 of 14 files at r2, 1 of 7 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @YuhongWang-Amazon)


src/main/docker/images.bzl line 121 at r5 (raw file):

    struct(
        name = "duchy_gcs_postgres_duchy_data_server_image",
        image = "//src/main/kotlin/org/wfanet/measurement/duchy/deploy/gcloud/server:gcs_postgres_duchy_data_server_image",

Either mirror the naming of the Spanner one to make this gcs_spanner_computations_server_image or switch to more accurate naming to avoid the vague "data server" naming to make this gcs_postgres_internal_server_image (updating the image name/repo accordingly).

This is the image for the GCS/Postgres implementation of the server that serves the Duchy internal API.

Code quote:

gcs_postgres_duchy_data_server_image

src/main/k8s/duchy.cue line 64 at r5 (raw file):

	_millPollingInterval?:                    string
	_duchy_data_server_name:                  string
  1. For some reason in the past we ended up mixing both lower_snake_case and lowerCamelCase for Cue fields. The latter is the correct casing, which should be used for all new fields.
  2. We only use these fields with non-concrete values (e.g. string) when we need the option of customizing something that can't easily be specified at the appropriate location (e.g. addressing the pod spec).

src/main/k8s/duchy.cue line 104 at r5 (raw file):

		"async-computation-control-server": {}
		"computation-control-server": _type: "LoadBalancer"
		"\(_duchy_data_server_name)": {}

Rather than having this be customized, just rename it to something generic that works for all cases.

Suggestion:

"internal-api-server"

src/main/k8s/duchy.cue line 187 at r5 (raw file):

			] + _blob_storage_flags
		}
		"\(_duchy_data_server_deployment_name)": #ServerDeployment & {

Rather than having this be customized, just rename it to something generic that works for all cases.

Suggestion:

"internal-api-server-deployment"

Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SanjayVas)


src/main/docker/images.bzl line 121 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Either mirror the naming of the Spanner one to make this gcs_spanner_computations_server_image or switch to more accurate naming to avoid the vague "data server" naming to make this gcs_postgres_internal_server_image (updating the image name/repo accordingly).

This is the image for the GCS/Postgres implementation of the server that serves the Duchy internal API.

I prefer the gcs_postgres_internal_server_image. Should I also update the class names? The class is still named GcsPostgresDuchyDataServer.kt

Copy link
Contributor

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SanjayVas and @YuhongWang-Amazon)

Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SanjayVas)


src/main/k8s/duchy.cue line 64 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…
  1. For some reason in the past we ended up mixing both lower_snake_case and lowerCamelCase for Cue fields. The latter is the correct casing, which should be used for all new fields.
  2. We only use these fields with non-concrete values (e.g. string) when we need the option of customizing something that can't easily be specified at the appropriate location (e.g. addressing the pod spec).

Done.


src/main/k8s/duchy.cue line 104 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Rather than having this be customized, just rename it to something generic that works for all cases.

Done.


src/main/k8s/duchy.cue line 187 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Rather than having this be customized, just rename it to something generic that works for all cases.

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 14 files at r1, 9 of 14 files at r2, 3 of 7 files at r4, 5 of 6 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @YuhongWang-Amazon)


src/main/docker/images.bzl line 121 at r5 (raw file):

Previously, YuhongWang-Amazon wrote…

I prefer the gcs_postgres_internal_server_image. Should I also update the class names? The class is still named GcsPostgresDuchyDataServer.kt

Yes if you wouldn't mind.


src/main/k8s/local/duchies.cue line 50 at r7 (raw file):

		protocolsSetupConfig:    "non_aggregator_protocols_setup_config.textproto"
		certificateResourceName: _worker2_cert_name
		databaseType:            "postgres"

Nice!

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @YuhongWang-Amazon)

Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/docker/images.bzl line 121 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Yes if you wouldn't mind.

I attempted to update this but found too many changes are required. Also both kingdom and reporting still have the data server names. Hence I decided to leave the class names like this for now. I prefer that we define a common name format for internal data servers of all three components and update them together later.

@YuhongWang-Amazon YuhongWang-Amazon enabled auto-merge (squash) August 17, 2023 14:14
Copy link
Contributor Author

@YuhongWang-Amazon YuhongWang-Amazon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @YuhongWang-Amazon)

@YuhongWang-Amazon YuhongWang-Amazon merged commit fd1b3c8 into main Aug 17, 2023
@YuhongWang-Amazon YuhongWang-Amazon deleted the yuhong.postgres-duchy-empty-cluster-correctness-test branch August 17, 2023 16:09
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