fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

corymhall · 2024-06-21T17:41:39Z

This PR removes the MaxItemsOne config for the subnetIds property. Originally this was a MaxItemsOne property in upstream, but it was removed when Multi-AZ support was added by AWS. In Terraform removing MaxItemsOne is not a breaking change, but for Pulumi it is.

Because it looks like there is very low usage of this resource, and the change enables a (maybe) more common use case (Multi-AZ filesystem) we have decided to take the breaking change now.

In order to upgrade users will only need to update their code so that subnetIds is now a list. For example,

From

const test = new aws.fsx.OpenZfsFileSystem("test", {
    storageCapacity: 64,
    subnetIds: test1.id,
    deploymentType: "SINGLE_AZ_1",
    throughputCapacity: 64,
});

To

const test = new aws.fsx.OpenZfsFileSystem("test", {
    storageCapacity: 64,
    subnetIds: [test1.id],
    deploymentType: "SINGLE_AZ_1",
    throughputCapacity: 64,
});

Because we are including the TransformFromState function, users should not need to make any changes to the state themselves.

BREAKING CHANGE: fsx.OpenZfsFileSystem.subnetIds now accepts a list instead of a string

closes #3106, closes #3034

This PR removes the `MaxItemsOne` config for the `subnetIds` property. Originally this was a `MaxItemsOne` property in upstream, but it was removed when Multi-AZ support was added by AWS. In Terraform removing `MaxItemsOne` is not a breaking change, but for Pulumi it is. Because it looks like there is very low usage of this resource, and the change enables a (maybe) more common use case (Multi-AZ filesystem) we have decided to take the breaking change now. In order to upgrade users will only need to update their code so that `subnetIds` is now a list. For example, From ```ts const test = new aws.fsx.OpenZfsFileSystem("test", { storageCapacity: 64, subnetIds: test1.id, deploymentType: "SINGLE_AZ_1", throughputCapacity: 64, }); ``` To ```ts const test = new aws.fsx.OpenZfsFileSystem("test", { storageCapacity: 64, subnetIds: [test1.id], deploymentType: "SINGLE_AZ_1", throughputCapacity: 64, }); ``` Because we are including the `TransformFromState` function, users should not need to make any changes to the state themselves. BREAKING CHANGE: `fsx.OpenZfsFileSystem.subnetIds` now accepts a list instead of a string closes #3106, closes #3034

github-actions · 2024-06-21T18:30:07Z

Does the PR have any schema changes?

Found 4 breaking changes:

Resources

"aws:fsx/openZfsFileSystem:OpenZfsFileSystem":
- 🟡 inputs: "subnetIds" type changed from "string" to "array":
  - 🟡 items had no type but now has &{Type:string Ref: AdditionalProperties: Items: OneOf:[] Discriminator: Plain:false}
- 🟡 properties: "subnetIds" type changed from "string" to "array":
  - 🟡 items had no type but now has &{Type:string Ref: AdditionalProperties: Items: OneOf:[] Discriminator: Plain:false}
    No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

t0yv0 · 2024-06-24T15:16:51Z

provider/resources.go

-			"aws_fsx_windows_file_system":           {Tok: awsResource(fsxMod, "WindowsFileSystem")},
+			"aws_fsx_openzfs_file_system": {
+				Tok: awsResource(fsxMod, "OpenZfsFileSystem"),
+				TransformFromState: func(ctx context.Context, state resource.PropertyMap) (resource.PropertyMap, error) {


Curious if you found this necessary? If I recall correctly the bridge might be already applying this transformation generically to permit lenient state reads to compensate for MaxItems=1 mismatches. If it doesn't at the moment we could consider moving it there anyway.

Yes it was necessary. Without it it showed a resource replacement.

flostadler

lgtm

flostadler · 2024-06-24T15:11:26Z

provider/provider_test.go

+						idiff := inputDiff.(map[string]interface{})
+						if adds, ok := idiff["adds"]; ok {
+							a := adds.(map[string]interface{})
+							delete(a, "region")


Why do we delete the region, that shouldn't change, right?

It can change between what you set it to locally and what is set in CI. I ended up changing the test to set the region explicitly, but I thought to keep this in.

But wouldn't change more in this case? E.g. ARNs or URLs

Hmm that's true. I"ll remove it.

provider/provider_yaml_test.go

provider/provider_test.go

t0yv0 · 2024-06-24T15:29:07Z

provider/provider_test.go

+			return
+		}
+
+		pulumiTest.T().Log("Plan is not equal, re-running up")


This is https://www.pulumi.com/docs/concepts/update-plans/ right?

t0yv0 · 2024-06-24T15:38:32Z

provider/provider_test.go

@@ -82,6 +89,100 @@ func testProviderUpgrade(t *testing.T, dir string, opts *testProviderUpgradeOpti
 	assertpreview.HasNoReplacements(t, result)
 }

+type testProviderCodeChangesOptions struct {


I'm looking at this and I'm thinking that ideally we'd figure out how to upstream this into https://github.com/pulumi/providertest

This is almost like:

func PreviewProviderUpgrade(t pulumitest.PT, pulumiTest *pulumitest.PulumiTest, providerName string, baselineVersion string, opts ...optproviderupgrade.PreviewProviderUpgradeOpt) auto.PreviewResult {

But not quite.

One difference is that it lets you specify two sets of programs and options which I've also wanted and I think would be a very useful thing to have.

Another difference is that your test also runs pulumi up? Or am I misreading this?

Yes I think we could upstream this since I think it is a generic enough use case. And yes those are the two main differences.

t0yv0 · 2024-06-24T15:39:31Z

provider/provider_test.go

+// This should be used when the plan is a good representation of what you are testing. Sometimes
+// there are issues where the plan is consistent, but the apply fails. In those cases a snapshot test is not
+// a good fit.
+func pulumiUpWithSnapshot(t *testing.T, pulumiTest *pulumitest.PulumiTest) {


This looks curious, I could use some help understanding what this is testing additionally, and perhaps if this is generally useful if we can upstream it to pulumitest.

I'm experimenting with something here. I think it may be useful to upstream, but I'm not sure yet since I think it would be easy to footgun yourself. I'm trying to figure out ways we can short circuit the test.

I think it is useful for the type of test where everything that you want to test exists in the plan snapshot. In those cases we can short circuit the test and only actually run the test when the plan has changed. In this case I'm testing that subnets as a list is able to be deployed and the snapshot contains this information.

In other cases the snapshot won't contain enough information, for example if the test depends on the value of unknowns the plan won't have those values so will never show a difference.

So you're looking for an optimization that doesn't run pulumi up when the plan hasn't changed, but does run it if there's changes in plans? This can be interesting. I worry that plan support is not yet solid enough to trust for this, but it can be interesting.

pulumi-bot · 2024-06-27T15:27:11Z

This PR has been shipped in release v6.42.0.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/aws](https://pulumi.io) ([source](https://togithub.com/pulumi/pulumi-aws)) | dependencies | minor | [`6.41.0` -> `6.42.0`](https://renovatebot.com/diffs/npm/@pulumi%2faws/6.41.0/6.42.0) | --- ### Release Notes <details> <summary>pulumi/pulumi-aws (@pulumi/aws)</summary> ### [`v6.42.0`](https://togithub.com/pulumi/pulumi-aws/releases/tag/v6.42.0) [Compare Source](https://togithub.com/pulumi/pulumi-aws/compare/v6.41.0...v6.42.0) ##### What's Changed - Add extra notes to critical resource properties by [@guineveresaenger](https://togithub.com/guineveresaenger) in [https://github.com/pulumi/pulumi-aws/pull/4088](https://togithub.com/pulumi/pulumi-aws/pull/4088) - upstream v5.55.0 by [@flostadler](https://togithub.com/flostadler) in [https://github.com/pulumi/pulumi-aws/pull/4102](https://togithub.com/pulumi/pulumi-aws/pull/4102) - Upgrade pulumi-terraform-bridge to v3.85.0 by [@pulumi-bot](https://togithub.com/pulumi-bot) in [https://github.com/pulumi/pulumi-aws/pull/4103](https://togithub.com/pulumi/pulumi-aws/pull/4103) - Enable TypedDict input types for the Python SDK by [@julienp](https://togithub.com/julienp) in [https://github.com/pulumi/pulumi-aws/pull/4108](https://togithub.com/pulumi/pulumi-aws/pull/4108) - fix!: unable to create fsx for OpenZfs Multi-AZ file system by [@corymhall](https://togithub.com/corymhall) in [https://github.com/pulumi/pulumi-aws/pull/4095](https://togithub.com/pulumi/pulumi-aws/pull/4095) - Fix unexpected alb.Listener target group refresh by [@t0yv0](https://togithub.com/t0yv0) in [https://github.com/pulumi/pulumi-aws/pull/4083](https://togithub.com/pulumi/pulumi-aws/pull/4083) ##### ⚠ BREAKING CHANGES - fsx.OpenZfsFileSystem - The `subnetIds` argument now accepts a list instead of a string ##### New Contributors - [@julienp](https://togithub.com/julienp) made their first contribution in [https://github.com/pulumi/pulumi-aws/pull/4108](https://togithub.com/pulumi/pulumi-aws/pull/4108) **Full Changelog**: pulumi/pulumi-aws@v6.41.0...v6.42.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/aws](https://pulumi.io) ([source](https://togithub.com/pulumi/pulumi-aws)) | dependencies | minor | [`6.41.0` -> `6.42.0`](https://renovatebot.com/diffs/npm/@pulumi%2faws/6.41.0/6.42.0) | --- ### Release Notes <details> <summary>pulumi/pulumi-aws (@pulumi/aws)</summary> ### [`v6.42.0`](https://togithub.com/pulumi/pulumi-aws/releases/tag/v6.42.0) [Compare Source](https://togithub.com/pulumi/pulumi-aws/compare/v6.41.0...v6.42.0) ##### What's Changed - Add extra notes to critical resource properties by [@guineveresaenger](https://togithub.com/guineveresaenger) in [https://github.com/pulumi/pulumi-aws/pull/4088](https://togithub.com/pulumi/pulumi-aws/pull/4088) - upstream v5.55.0 by [@flostadler](https://togithub.com/flostadler) in [https://github.com/pulumi/pulumi-aws/pull/4102](https://togithub.com/pulumi/pulumi-aws/pull/4102) - Upgrade pulumi-terraform-bridge to v3.85.0 by [@pulumi-bot](https://togithub.com/pulumi-bot) in [https://github.com/pulumi/pulumi-aws/pull/4103](https://togithub.com/pulumi/pulumi-aws/pull/4103) - Enable TypedDict input types for the Python SDK by [@julienp](https://togithub.com/julienp) in [https://github.com/pulumi/pulumi-aws/pull/4108](https://togithub.com/pulumi/pulumi-aws/pull/4108) - fix!: unable to create fsx for OpenZfs Multi-AZ file system by [@corymhall](https://togithub.com/corymhall) in [https://github.com/pulumi/pulumi-aws/pull/4095](https://togithub.com/pulumi/pulumi-aws/pull/4095) - Fix unexpected alb.Listener target group refresh by [@t0yv0](https://togithub.com/t0yv0) in [https://github.com/pulumi/pulumi-aws/pull/4083](https://togithub.com/pulumi/pulumi-aws/pull/4083) ##### ⚠ BREAKING CHANGES - fsx.OpenZfsFileSystem - The `subnetIds` argument now accepts a list instead of a string ##### New Contributors - [@julienp](https://togithub.com/julienp) made their first contribution in [https://github.com/pulumi/pulumi-aws/pull/4108](https://togithub.com/pulumi/pulumi-aws/pull/4108) **Full Changelog**: pulumi/pulumi-aws@v6.41.0...v6.42.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>

corymhall requested review from t0yv0 and flostadler June 24, 2024 12:27

corymhall added 2 commits June 24, 2024 10:06

update plan cleaner

773edec

add stack.json

328ee74

corymhall self-assigned this Jun 24, 2024

t0yv0 reviewed Jun 24, 2024

View reviewed changes

flostadler approved these changes Jun 24, 2024

View reviewed changes

t0yv0 reviewed Jun 24, 2024

View reviewed changes

t0yv0 self-requested a review June 24, 2024 16:24

t0yv0 approved these changes Jun 24, 2024

View reviewed changes

corymhall added 5 commits June 24, 2024 12:53

fixing

fe72ac4

Merge remote-tracking branch 'origin/master' into corymhall/fix-openzfs

34ca15e

updating helper

7bd36d9

forgot to set other region

b863b52

attempt 2

f40e73d

corymhall merged commit 507f620 into master Jun 25, 2024
24 checks passed

corymhall deleted the corymhall/fix-openzfs branch June 25, 2024 18:43

This was referenced Jun 27, 2024

Unable to create Fsx for OpenZFS Multi-AZ file system #3106

Closed

OpenZfsFileSystem.subnetIds is a string, not a string array #3034

Closed

renovate bot mentioned this pull request Dec 23, 2024

Bump pulumi dependencies (major) gitfool/Pulumi.Dungeon#104

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

corymhall commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading

t0yv0 Jun 24, 2024

corymhall Jun 24, 2024

flostadler left a comment

flostadler Jun 24, 2024

corymhall Jun 24, 2024

flostadler Jun 25, 2024

corymhall Jun 25, 2024

t0yv0 Jun 24, 2024

corymhall Jun 24, 2024

t0yv0 Jun 24, 2024

corymhall Jun 24, 2024

t0yv0 Jun 24, 2024

corymhall Jun 24, 2024

t0yv0 Jun 24, 2024

pulumi-bot commented Jun 27, 2024

fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

Conversation

corymhall commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 • edited Loading

Does the PR have any schema changes?

Resources

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flostadler left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulumi-bot commented Jun 27, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading