Skip to content

terraform test: New version with local helm chart support#31527

Merged
def- merged 1 commit intoMaterializeInc:mainfrom
def-:pr-terraform-module
Feb 20, 2025
Merged

terraform test: New version with local helm chart support#31527
def- merged 1 commit intoMaterializeInc:mainfrom
def-:pr-terraform-module

Conversation

@def-
Copy link
Contributor

@def- def- commented Feb 18, 2025

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • 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).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@def- def- requested a review from a team as a code owner February 18, 2025 11:40
@def- def- force-pushed the pr-terraform-module branch 2 times, most recently from 5c5d247 to 678517d Compare February 18, 2025 12:43
@def- def- requested review from bobbyiliev and jubrad and removed request for a team February 18, 2025 14:38
@def- def- force-pushed the pr-terraform-module branch 3 times, most recently from b50dabd to d5bb254 Compare February 18, 2025 19:15
@def-
Copy link
Contributor Author

def- commented Feb 18, 2025

@bobbyiliev I'm wondering why this doesn't work: https://buildkite.com/materialize/test/builds/99223#01951a82-de29-48b1-979f-897baf998e06

error: getting semver of materialize: db error: ERROR: CLUSTER "mz_system" has no replicas available to service request

I'm explicitly setting the defaultReplicationFactor in helm_values, I assume that should work together with use_local_helm_chart.
I'm passing in the operator version here: https://github.com/MaterializeInc/materialize/pull/31527/files#diff-83d8f0169fbca62050b7506c763a1ce53697a5979da1d3759d83cb5a48e3f3eaR163 but I'm actually wondering if that will use the operator from the current source code how I could specify that.

This is somehow related to #31452, but I don't understand how we can both use a new operator version that sets the defaults to 0, but doesn't accept me to override them. cc @SangJunBak maybe you have an idea too

@SangJunBak
Copy link
Contributor

@def- from first glance, it seems like defaultReplicationFactor isn't scoped properly! If you look in values.yaml, it's scoped as clusters.defaultReplicationFactor. I am curious... If we specify clusters.defaultReplicationFactor, would we need to specify clusters.sizes and other variables in the clusters scope?

@def- def- force-pushed the pr-terraform-module branch 3 times, most recently from 03bf6bb to b9651ba Compare February 19, 2025 14:26
@def-
Copy link
Contributor Author

def- commented Feb 19, 2025

I tried that but no difference still:

error: getting semver of materialize: db error: ERROR: CLUSTER "mz_system" has no replicas available to service request

probe = 1
support = 1
analytics = 1
clusters = {
Copy link
Contributor

Choose a reason for hiding this comment

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

The clusters section is under the operator key in the values file:

https://github.com/MaterializeInc/materialize/blob/main/misc/helm-charts/operator/values.yaml#L11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is it true that if I set anything in operator I have to override everything?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this should not be the case. It will override only the values that you are modifying:

https://helm.sh/docs/chart_template_guide/subcharts_and_globals/#overriding-values-from-a-parent-chart

Are you seeing a different behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get it into a working state, I'm not sure what I'm doing wrong with the helm_values or if they don't work. I just manually alter the cluster now and that works for me. So can be merged in this state.

@def- def- force-pushed the pr-terraform-module branch 3 times, most recently from a5fbf7b to 838f77d Compare February 20, 2025 10:42
@def- def- requested a review from bobbyiliev February 20, 2025 11:15
@def- def- force-pushed the pr-terraform-module branch from 838f77d to 53123b4 Compare February 20, 2025 11:34
@def- def- force-pushed the pr-terraform-module branch from 53123b4 to 5ef5199 Compare February 20, 2025 12:38
@def- def- enabled auto-merge February 20, 2025 15:12
@def- def- merged commit 12c4491 into MaterializeInc:main Feb 20, 2025
84 checks passed
@def- def- deleted the pr-terraform-module branch February 20, 2025 16:51
@def- def- added the self-managed-backport-v25.1 Needs to be backported into the v25.1 self-managed release label Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

self-managed-backport-v25.1-done self-managed-backport-v25.1 Needs to be backported into the v25.1 self-managed release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants