Resources now inherit the DeletedWith Resource Option across SDKs. #12572

dixler · 2023-03-30T22:58:26Z

Description

This PR uses this information to set the DeletedWith when it is left unset by the user's program and there is a Parent resource to inherit the value from.

From #12446 which was reverted due to #12562 having difficulty with MLCs and Read resources.

TODO

Add test case to catch the condition causing Could not find parent goal after update #12562
Fix data race

Fixes #12158

Needs follow up on: pulumi/pulumi-hugo#2566

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2023-03-30T22:59:19Z

Changelog

[uncommitted] (2023-06-02)

Features

[engine] DeletedWith ResourceOption is now inherited from its parent across SDKs.
#12572

dixler · 2023-04-17T16:22:33Z

pkg/resource/deploy/step_generator_test.go

+// TestResourceInheritsOptionsFromParentNoGoal checks that configureGoal does not error when the parent goal is not
+// present in the step generator. This can occur for Read resources (i.e. pkg.Resource.get()) and MLCs.
+func TestResourceInheritsOptionsFromParentNoGoal(t *testing.T) {
+	t.Parallel()
+
+	parentURN := resource.NewURN("a", "proj", "d:e:f", "a:b:c", "parent")
+
+	childURN := resource.NewURN("a", "proj", "d:e:f", "a:b:c", "child")
+	goal := &resource.Goal{
+		Parent: parentURN,
+		Type:   childURN.Type(),
+		Name:   childURN.Name(),
+	}
+
+	// Create a step generator without the parent goal.
+	sg := stepGenerator{
+		urns: map[resource.URN]bool{
+			parentURN: true,
+		},
+		resourceGoals: map[resource.URN]*resource.Goal{},
+	}
+	err := sg.configureGoal(goal)
+
+	assert.Nil(t, err)
+}


Catches the regression.

Changed approach to track goals in the resource monitor.

Resources without goals should show up in the resource monitor (even MLCs). Read resources don't actually have resource options sent over the wire to propagate and expectation for imported resources to be inherited by child resources are an inaccuracy.

Frassle

So if the parent is an MLC or read resource we don't propagate the options? That feels wrong.

pkg/resource/deploy/step_generator.go

Frassle · 2023-04-19T14:26:37Z

pkg/resource/deploy/step_generator_test.go

@@ -360,3 +360,107 @@ func TestEngineDiff(t *testing.T) {
 		})
 	}
 }
+
+func TestResourceInheritsOptionsFromParent(t *testing.T) {


I'm not sure how much I'd trust tests around the step_generator like this. I'd rather see a test in pkg/engine/lifecycletest that runs a whole deployment checking that we can have parent child combinations of the product of [mlc, readResource, registerResource].

ReadResource requests don't have ResourceOptions attached other than Parent

pulumi/sdk/proto/go/resource.pb.go

Lines 135 to 153 in ec1b046

// ReadResourceRequest contains enough information to uniquely qualify and read a resource's state.

type ReadResourceRequest struct {

state protoimpl.MessageState

sizeCache protoimpl.SizeCache

unknownFields protoimpl.UnknownFields

Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` // the ID of the resource to read.

Type string `protobuf:"bytes,2,opt,name=type,proto3" json:"type,omitempty"` // the type of the resource object.

Name string `protobuf:"bytes,3,opt,name=name,proto3" json:"name,omitempty"` // the name, for URN purposes, of the object.

Parent string `protobuf:"bytes,4,opt,name=parent,proto3" json:"parent,omitempty"` // an optional parent URN that this child resource belongs to.

Properties *structpb.Struct `protobuf:"bytes,5,opt,name=properties,proto3" json:"properties,omitempty"` // optional state sufficient to uniquely identify the resource.

Dependencies []string `protobuf:"bytes,6,rep,name=dependencies,proto3" json:"dependencies,omitempty"` // a list of URNs that this read depends on, as observed by the language host.

Provider string `protobuf:"bytes,7,opt,name=provider,proto3" json:"provider,omitempty"` // an optional reference to the provider to use for this read.

Version string `protobuf:"bytes,8,opt,name=version,proto3" json:"version,omitempty"` // the version of the provider to use when servicing this request.

AcceptSecrets bool `protobuf:"varint,9,opt,name=acceptSecrets,proto3" json:"acceptSecrets,omitempty"` // when true operations should return secrets as strongly typed.

AdditionalSecretOutputs []string `protobuf:"bytes,10,rep,name=additionalSecretOutputs,proto3" json:"additionalSecretOutputs,omitempty"` // a list of output properties that should also be treated as secret, in addition to ones we detect.

AcceptResources bool `protobuf:"varint,12,opt,name=acceptResources,proto3" json:"acceptResources,omitempty"` // when true operations should return resource references as strongly typed.

PluginDownloadURL string `protobuf:"bytes,13,opt,name=pluginDownloadURL,proto3" json:"pluginDownloadURL,omitempty"` // the server url of the provider to use when servicing this request.

}

Nothing in deploytest indicates to me that we call RegisterResource on ReadResources.
This leads me to believe that a ReadResource shouldn't have any ResourceOptions for the Child resource to Inherit.

pkg/resource/deploy/source_eval.go

dixler · 2023-05-11T19:33:37Z

pkg/engine/lifecycletest/step_generator_test.go

+	assert.Nil(t, res)
+}
+
+// TODO determine if this is redundant.


This seems redundant at this point. This test checks that a RegisterResource call inside of a MLC provider is properly parented, but the distinction between the MLC provider and the program itself parenting an MLC resource seems irrelevant.

pkg/resource/deploy/source_eval.go

pkg/engine/lifecycletest/step_generator_test.go

pkg/resource/deploy/source_eval.go

Frassle · 2023-05-31T17:10:27Z

pkg/resource/deploy/source_eval.go

@@ -993,6 +996,16 @@ func (rm *resmon) ReadResource(ctx context.Context,
 	}, nil
 }

+// inheritFromParent returns a new goal that inherits from the given parent goal.


Ugh this really sucks that we're getting inheritance logic in step_generator and source_eval now. That's gonna bite us in the ass at some point for sure.

I don't think there's any quick fix for that, so probably have to live with it for this change but this is tech debt that needs paying down. (cc @justinvp)

Technically, now it's only in source_eval

@Frassle, do you mean alias inheritance in step_generator, or something else?

alias inheritance but also checking for parent correctness and defaulting that to the stack resource if it exists

Frassle

Looks ok, I think we'll want to work out a way to move parent logic consistently to step_generator at some point but will take time to think that through

This is implemented in the engine and interprets the empty string `""` to inherit the value from the resource's parent if it exists signifying that it was unspecified by the user's program. There is currently no way to override this in a child to unset it when set by the parent, but can be addressed by not parenting the resource to a resource with `deletedWith` set.

dixler · 2023-06-02T16:21:48Z

bors merge

bors · 2023-06-02T16:51:59Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

bors-ok

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from cbd19e8 to fb272a5 Compare April 17, 2023 16:21

dixler commented Apr 17, 2023

View reviewed changes

dixler requested a review from a team April 17, 2023 20:00

dixler marked this pull request as ready for review April 17, 2023 20:00

Frassle reviewed Apr 18, 2023

View reviewed changes

dixler changed the title ~~[wip] Resources now inherit the DeletedWith Resource Option across SDKs.~~ Resources now inherit the DeletedWith Resource Option across SDKs. Apr 18, 2023

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch 3 times, most recently from 45bd920 to 81dc491 Compare April 18, 2023 21:14

Frassle reviewed Apr 19, 2023

View reviewed changes

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch 3 times, most recently from 9dec774 to c7b26ba Compare May 11, 2023 19:16

dixler requested review from abhinav, justinvp, Frassle and a team and removed request for a team, abhinav, justinvp and Frassle May 11, 2023 19:20

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from c7b26ba to f7c75fa Compare May 11, 2023 19:26

dixler commented May 11, 2023

View reviewed changes

pkg/resource/deploy/source_eval.go Outdated Show resolved Hide resolved

dixler commented May 11, 2023

View reviewed changes

pkg/resource/deploy/source_eval.go Outdated Show resolved Hide resolved

dixler commented May 11, 2023

View reviewed changes

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from f7c75fa to 88c4521 Compare May 11, 2023 19:36

dixler requested review from abhinav, justinvp and a team May 11, 2023 20:09

dixler requested a review from Frassle May 11, 2023 20:09

justinvp reviewed May 19, 2023

View reviewed changes

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from 326f18e to b84f498 Compare May 30, 2023 17:00

dixler requested a review from justinvp May 30, 2023 17:51

Frassle reviewed May 31, 2023

View reviewed changes

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from 08a7387 to 3835d75 Compare May 31, 2023 20:35

Frassle approved these changes Jun 2, 2023

View reviewed changes

dixler force-pushed the dixler/12158/deleted-with-engine-2 branch from 3835d75 to 653adbe Compare June 2, 2023 16:20

bors bot merged commit 3feb137 into master Jun 2, 2023
40 checks passed

bors bot deleted the dixler/12158/deleted-with-engine-2 branch June 2, 2023 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resources now inherit the DeletedWith Resource Option across SDKs. #12572

Resources now inherit the DeletedWith Resource Option across SDKs. #12572

dixler commented Mar 30, 2023 •

edited

pulumi-bot commented Mar 30, 2023 •

edited

dixler Apr 17, 2023

dixler May 11, 2023

Frassle left a comment

Frassle Apr 19, 2023

dixler Apr 21, 2023

dixler May 11, 2023

Frassle May 31, 2023

dixler May 31, 2023

justinvp May 31, 2023

Frassle Jun 1, 2023

Frassle left a comment

dixler commented Jun 2, 2023

bors bot commented Jun 2, 2023

	// ReadResourceRequest contains enough information to uniquely qualify and read a resource's state.
	type ReadResourceRequest struct {
	state protoimpl.MessageState
	sizeCache protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` // the ID of the resource to read.
	Type string `protobuf:"bytes,2,opt,name=type,proto3" json:"type,omitempty"` // the type of the resource object.
	Name string `protobuf:"bytes,3,opt,name=name,proto3" json:"name,omitempty"` // the name, for URN purposes, of the object.
	Parent string `protobuf:"bytes,4,opt,name=parent,proto3" json:"parent,omitempty"` // an optional parent URN that this child resource belongs to.
	Properties *structpb.Struct `protobuf:"bytes,5,opt,name=properties,proto3" json:"properties,omitempty"` // optional state sufficient to uniquely identify the resource.
	Dependencies []string `protobuf:"bytes,6,rep,name=dependencies,proto3" json:"dependencies,omitempty"` // a list of URNs that this read depends on, as observed by the language host.
	Provider string `protobuf:"bytes,7,opt,name=provider,proto3" json:"provider,omitempty"` // an optional reference to the provider to use for this read.
	Version string `protobuf:"bytes,8,opt,name=version,proto3" json:"version,omitempty"` // the version of the provider to use when servicing this request.
	AcceptSecrets bool `protobuf:"varint,9,opt,name=acceptSecrets,proto3" json:"acceptSecrets,omitempty"` // when true operations should return secrets as strongly typed.
	AdditionalSecretOutputs []string `protobuf:"bytes,10,rep,name=additionalSecretOutputs,proto3" json:"additionalSecretOutputs,omitempty"` // a list of output properties that should also be treated as secret, in addition to ones we detect.
	AcceptResources bool `protobuf:"varint,12,opt,name=acceptResources,proto3" json:"acceptResources,omitempty"` // when true operations should return resource references as strongly typed.
	PluginDownloadURL string `protobuf:"bytes,13,opt,name=pluginDownloadURL,proto3" json:"pluginDownloadURL,omitempty"` // the server url of the provider to use when servicing this request.
	}

Resources now inherit the DeletedWith Resource Option across SDKs. #12572

Resources now inherit the DeletedWith Resource Option across SDKs. #12572

Conversation

dixler commented Mar 30, 2023 • edited

Description

Checklist

pulumi-bot commented Mar 30, 2023 • edited

Changelog

[uncommitted] (2023-06-02)

Features

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frassle 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

Frassle left a comment

Choose a reason for hiding this comment

dixler commented Jun 2, 2023

bors bot commented Jun 2, 2023

dixler commented Mar 30, 2023 •

edited

pulumi-bot commented Mar 30, 2023 •

edited