*: Rename "default" cluster to "quickstart" #24019

ParkMyCar · 2023-12-19T15:39:34Z

This PR renames the "default" cluster to "quickstart" for new environments, existing environments will not have their cluster renamed. To keep the default value for the "cluster" variable to "default" we add a Catalog migration that runs for existing environments.

Motivation

Fixes https://github.com/MaterializeInc/database-issues/issues/6755

Tips for reviewer

There are a large number of changes here, but the majority of them are test-only changes. The PR is broken up into commits that can ideally be review independently:

The changes to actually rename the "default" cluster to "quickstart"
Updates to sqllogictests
Updates to testdrive tests
Updates to our python tests
Updates to our dbt tests
Updates to our Rust tests
Updates to the legacy-upgrade tests

The accompanying changes to our user documentation is here: #24035

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- Renames the cluster created for every environment from "default" to "quickstart"

def-

Glad to see you triggered nightly already, I'm expected lots of related failures. If that's green, ok

def-

The "Detect references to already closed issues" can be ignored, but platform-checks looks like a new issue:

Running manipulate() from <materialize.checks.all_checks.identifiers.Identifiers object at 0x7ff8495ce250>
--- docker compose exec -T testdrive testdrive --kafka-addr=kafka:9092 --schema-registry-url=http://schema-registry:8081 --materialize-url=postgres://materialize@materialized:6875 --materialize-internal-url=postgres://materialize@materialized:6877 --aws-endpoint=http://localstack:4566 --no-reset --default-timeout=300s --seed=1 --persist-blob-url=file:///mzdata/persist/blob --persist-consensus-url=postgres://root@materialized:26257?options=--search_path=consensus --var=replicas=1 --var=default-replica-size=4-4 --var=default-storage-size=4-1 --source=/var/lib/buildkite-agent/builds/buildkite-builders-d43b1b5-i-092248735da8c3952-1/materialize/nightlies/misc/python/materialize/checks/all_checks/identifiers.py:150
> SET CLUSTER=identifiers;
rows match; continuing at ts 1703179510.3750253
> SET DATABASE="	� Â…Â áš€â€‚â€ƒâ€‚â€ƒâ€„â€…â€†â€‡â€ˆâ€‰â€Šâ€‹â€¨â€©â€¯â�Ÿã€€";
rows match; continuing at ts 1703179510.3760698
> CREATE MATERIALIZED VIEW "../../../../../../../../../../../etc/hosts"." " IN CLUSTER quickstart AS
SELECT "<foo val=â€œbarâ€� />", c2 as "<foo val=â€œbarâ€� />" FROM "../../../../../../../../../../../etc/hosts"."<a href=""javascript\x0D:javascript:alert(1)"" id=""fuzzelement1"">test</a>";

^^^ +++
4:1: error: executing query failed: db error: ERROR: column "<foo val=â€œbarâ€� />" specified more than once: ERROR: column "<foo val=â€œbarâ€� />" specified more than once

I'm wondering why that is failing now

ParkMyCar · 2023-12-21T17:37:37Z

Glad to see you triggered nightly already, I'm expected lots of related failures. If that's green, ok

There were a lot of test failures, but I've been iterating :)

Running manipulate() from <materialize.checks.all_checks.identifiers.Identifiers object at 0x7ff8495ce250>
--- docker compose exec -T testdrive testdrive --kafka-addr=kafka:9092 --schema-registry-url=http://schema-registry:8081 --materialize-url=postgres://materialize@materialized:6875 --materialize-internal-url=postgres://materialize@materialized:6877 --aws-endpoint=http://localstack:4566 --no-reset --default-timeout=300s --seed=1 --persist-blob-url=file:///mzdata/persist/blob --persist-consensus-url=postgres://root@materialized:26257?options=--search_path=consensus --var=replicas=1 --var=default-replica-size=4-4 --var=default-storage-size=4-1 --source=/var/lib/buildkite-agent/builds/buildkite-builders-d43b1b5-i-092248735da8c3952-1/materialize/nightlies/misc/python/materialize/checks/all_checks/identifiers.py:150
> SET CLUSTER=identifiers;
rows match; continuing at ts 1703179510.3750253
> SET DATABASE="	� Â…Â áš€â€‚â€ƒâ€‚â€ƒâ€„â€…â€†â€‡â€ˆâ€‰â€Šâ€‹â€¨â€©â€¯â�Ÿã€€";
rows match; continuing at ts 1703179510.3760698
> CREATE MATERIALIZED VIEW "../../../../../../../../../../../etc/hosts"." " IN CLUSTER quickstart AS
SELECT "<foo val=â€œbarâ€� />", c2 as "<foo val=â€œbarâ€� />" FROM "../../../../../../../../../../../etc/hosts"."<a href=""javascript\x0D:javascript:alert(1)"" id=""fuzzelement1"">test</a>";

^^^ +++
4:1: error: executing query failed: db error: ERROR: column "<foo val=â€œbarâ€� />" specified more than once: ERROR: column "<foo val=â€œbarâ€� />" specified more than once

I'm wondering why that is failing now

I'm not sure why this is failing either, I changed some strings to f-strings but not this one. I was going to dig into it

ParkMyCar · 2023-12-21T17:51:04Z

@def- it seems like these two lines have the same exact content, was that intentional?. I copied them into Rust Playground and inspected their byte representation. So it seems possible that this conflict could happen, because column and alias could map to the same ident. Maybe we're just getting unlucky now?

def- · 2023-12-21T17:58:45Z

Oooh, that is indeed not supposed to happen. Let me scan that file for other duplicates, thanks!

Since our tests depend on them being unique, thanks to ParkMyCar for noticing: MaterializeInc#24019 (comment)

jkosh44 · 2023-12-21T18:40:19Z

misc/python/materialize/checks/checks.py

+        if self.base_version >= MzVersion.parse_mz("v0.81.0-dev"):
+            return "quickstart"
+        else:
+            return "default"


jkosh44 · 2023-12-21T18:42:44Z

src/adapter/src/catalog/migrate.rs

+    _conn_catalog: &ConnCatalog<'_>,
+    _connection_context: &ConnectionContext,
+) -> Result<(), anyhow::Error> {
+    txn.upsert_system_config("cluster", "default".to_string())?;


Do we need to check if they've already overwritten their cluster var and not upsert in that case?

#23903 has yet to be included in a release, so it wouldn't have been previously possible for users to overwrite their cluster var in this case

jkosh44 · 2023-12-21T18:43:31Z

src/adapter/src/catalog/state.rs

@@ -220,7 +220,7 @@ impl CatalogState {
            state: Cow::Borrowed(self),
            unresolvable_ids: BTreeSet::new(),
            conn_id: SYSTEM_CONN_ID.clone(),
-            cluster: "default".into(),
+            cluster: "quickstart".into(),


Can we use mz_catalog::durable::initialize::DEFAULT_USER_CLUSTER_NAME here?

This made me realize we shouldn't use a static value for a session-less user, and instead we should use whatever value is in the system vars, updated!

jkosh44 · 2023-12-21T18:45:08Z

src/catalog/tests/snapshots/debug__stash_opened_trace.snap

+                                    id: 6,
+                                    event_type: Grant,
+                                    object_type: Database,
                                    user: None,
                                    occurred_at: Some(
                                        EpochMillis {
                                            millis: 0,
                                        },
                                    ),
                                    details: Some(
-                                        IdNameV1(
-                                            IdNameV1 {
-                                                id: "u1",
-                                                name: "default",
+                                        UpdatePrivilegeV1(
+                                            UpdatePrivilegeV1 {
+                                                object_id: "Du1",
+                                                grantee_id: "p",
+                                                grantor_id: "s1",
+                                                privileges: "U",


What happened here? There's a lot more changes than I was expecting.

Turns out the snapshot isn't sorted, so the large amount of changes comes from audit log events being ordered differently in the formatting of the snapshot. If you'd like I can fix the ordering in this PR, or in a follow up!

No that's ok. As long as it's just ordering it's fine.

jkosh44 · 2023-12-21T18:45:48Z

test/cluster/query-without-default-cluster/query-without-default-cluster.td


-# to make it easier to run the test again, let's recreate the default cluster
+# to make it easier to run the test again, let's recreate the defaquickstartult cluster


Suggested change

# to make it easier to run the test again, let's recreate the defaquickstartult cluster

# to make it easier to run the test again, let's recreate the quickstart cluster

Whoops, updated!

jkosh44

LGTM

rename default cluster to quickstart add catalog migration so existing users' cluster is 'default' fix catalog migration

update sqllogictests, again

update testdrive to pick cluster name based on db version

python test fmt update python tests, again update python tests, again again update python tests, again python fmt python type fixes python, use base version not current version

update dbt tests, again update dbt tests, again

rust test fmt update rust tests, again

ParkMyCar force-pushed the cluster/rename-default-to-quickstart branch 14 times, most recently from 0c7b1fd to a8f61a5 Compare December 21, 2023 16:53

ParkMyCar marked this pull request as ready for review December 21, 2023 17:15

ParkMyCar requested review from a team as code owners December 21, 2023 17:15

ParkMyCar requested review from a team and jkosh44 December 21, 2023 17:15

def- reviewed Dec 21, 2023

View reviewed changes

def- requested changes Dec 21, 2023

View reviewed changes

ParkMyCar force-pushed the cluster/rename-default-to-quickstart branch from a8f61a5 to 1ca6a4e Compare December 21, 2023 17:42

def- added a commit to def-/materialize that referenced this pull request Dec 21, 2023

blns: Remove duplicate strings

f1dd841

Since our tests depend on them being unique, thanks to ParkMyCar for noticing: MaterializeInc#24019 (comment)

def- mentioned this pull request Dec 21, 2023

blns: Remove duplicate strings #24078

Merged

5 tasks

jkosh44 reviewed Dec 21, 2023

View reviewed changes

morsapaes approved these changes Dec 23, 2023

View reviewed changes

morsapaes mentioned this pull request Dec 23, 2023

Replace mentions to default cluster with quickstart MaterializeInc/terraform-provider-materialize#425

Closed

jkosh44 approved these changes Dec 27, 2023

View reviewed changes

ParkMyCar requested a review from a team as a code owner December 29, 2023 02:01

ParkMyCar added 8 commits January 2, 2024 09:12

rename the default cluster to quickstart

b0994ea

rename default cluster to quickstart add catalog migration so existing users' cluster is 'default' fix catalog migration

update all required sqllogictests

3d3ec65

update sqllogictests, again

update all testdrive tests

0801389

update testdrive to pick cluster name based on db version

update python tests

039b724

python test fmt update python tests, again update python tests, again again update python tests, again python fmt python type fixes python, use base version not current version

update dbt tests

d938095

update dbt tests, again update dbt tests, again

update Rust tests and dev docs

a261a77

rust test fmt update rust tests, again

update legacy upgrade checks to use cluster other than default

53c96a0

reload system config after migrations, responded to GitHub feedback

236d460

ParkMyCar force-pushed the cluster/rename-default-to-quickstart branch from b966ba0 to 236d460 Compare January 2, 2024 14:17

ParkMyCar added 2 commits January 2, 2024 09:34

update references of v0.81 to v0.82

ad84b63

update migrate to account for users changing their default

dc20915

def- approved these changes Jan 2, 2024

View reviewed changes

ParkMyCar merged commit f82746e into MaterializeInc:main Jan 2, 2024
152 of 155 checks passed

ParkMyCar deleted the cluster/rename-default-to-quickstart branch June 17, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Rename "default" cluster to "quickstart" #24019

*: Rename "default" cluster to "quickstart" #24019

ParkMyCar commented Dec 19, 2023 •

edited

Loading

def- left a comment

def- left a comment •

edited

Loading

ParkMyCar commented Dec 21, 2023

ParkMyCar commented Dec 21, 2023

def- commented Dec 21, 2023

jkosh44 Dec 21, 2023

jkosh44 Dec 21, 2023

ParkMyCar Dec 27, 2023

jkosh44 Dec 21, 2023

ParkMyCar Dec 27, 2023

jkosh44 Dec 21, 2023

ParkMyCar Dec 27, 2023

jkosh44 Dec 27, 2023

jkosh44 Dec 21, 2023

ParkMyCar Dec 27, 2023

jkosh44 left a comment


		# to make it easier to run the test again, let's recreate the default cluster
		# to make it easier to run the test again, let's recreate the defaquickstartult cluster

*: Rename "default" cluster to "quickstart" #24019

*: Rename "default" cluster to "quickstart" #24019

Conversation

ParkMyCar commented Dec 19, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

def- left a comment

Choose a reason for hiding this comment

def- left a comment • edited Loading

Choose a reason for hiding this comment

ParkMyCar commented Dec 21, 2023

ParkMyCar commented Dec 21, 2023

def- commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkosh44 left a comment

Choose a reason for hiding this comment

ParkMyCar commented Dec 19, 2023 •

edited

Loading

def- left a comment •

edited

Loading