RetainOnDelete NOT Work ON chart sub resource

xzxiong commented 3 months ago

What happened?

create new chart with options pulumi.RetainOnDelete(true). when i delete the chart, only retain the chart resource, all its sub resources have been delete.

     Type                                                              Name                                       Plan               Info
     pulumi:pulumi:Stack                                               ob-component-qa                                               6 messages
 ~   ├─ pulumi:providers:kubernetes                                    example-provider                           update             [diff: ~kubeconfig,version
 -   └─ kubernetes:helm.sh/v3:Chart                                    agent-demo                                 delete[retain]
 -      ├─ kubernetes:rbac.authorization.k8s.io/v1:ClusterRoleBinding  agent-demo-scale-agent-demo-rolebinding    delete
 -      ├─ kubernetes:core/v1:Service                                  mo-ob/agent-demo-scale-agent-demo          delete
 -      ├─ kubernetes:rbac.authorization.k8s.io/v1:ClusterRole         agent-demo-scale-agent-demo                delete
 -      ├─ kubernetes:core/v1:ServiceAccount                           mo-ob/agent-demo-scale-agent-demo          delete
 -      └─ kubernetes:apps/v1:Deployment                               mo-ob/agent-demo-scale-agent-demo          delete

Example

import (
        "github.com/pulumi/pulumi-kubernetes/sdk/v3/go/kubernetes"
    helmv3 "github.com/pulumi/pulumi-kubernetes/sdk/v3/go/kubernetes/helm/v3"
        "github.com/pulumi/pulumi/sdk/v3/go/pulumi"
)

    if _, err := helmv3.NewChart(ctx, "myChart", helmv3.ChartArgs{
        Path:      pulumi.String(chartPath),
        Namespace: pulumi.String(namespace),
        Values:    values,
    }, pulumi.Provider(provider), pulumi.RetainOnDelete(true)); err != nil {
        return err
    }

Output of `pulumi about`

CLI Version 3.103.0 Go Version go1.21.5 Go Compiler gc

Plugins NAME VERSION alicloud 3.37.0 go unknown kubernetes 4.13.1 kubernetes 3.29.0

Host OS darwin Version 14.4.1 Arch arm64

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

blampe commented 3 months ago

Agree this is surprising! Looks like it's also the case with Chart v4.

https://github.com/pulumi/pulumi-kubernetes/blob/a8ca7385ca056d6a150d3fc1c00143c93b85c5b0/sdk/go/kubernetes/yaml/yaml.go#L95

https://github.com/pulumi/pulumi-kubernetes/blob/a8ca7385ca056d6a150d3fc1c00143c93b85c5b0/provider/pkg/provider/helm/v4/chart.go#L263

@EronWright I see you did a big overhaul of this stuff in #2720. I'm not sure where "does not apply to component resources" came from but I think it would be reasonable to propagate retainOnDelete, what do you think?

xzxiong commented 3 months ago

quote "The default is to inherit this value from the parent resource" from https://www.pulumi.com/docs/concepts/options/retainondelete/#resource-option-retainondelete

But actually no, parent's retainOnDelete NO affect its children.

EronWright commented 3 months ago

Thanks @xzxiong for the report, and you are correct that the retainOnDelete option doesn't propagate to the children, when applied to Chart v4, ConfigGroup v2, ConfigFile v2, or Directory v2. These are "component resources" and the option simply doesn't propagate.

The retainOnDelete resource option does not apply to component resources, and will not have the intended effect.

Here's a related issue to better document this limitation: https://github.com/pulumi/docs/issues/11663

I would suggest the following workaround, use the transforms option to add the retainOnDelete option to each of the children. Here's some information that might be useful.

xzxiong commented 3 months ago

Thanks @xzxiong for the report, and you are correct that the retainOnDelete option doesn't propagate to the children, when applied to Chart v4, ConfigGroup v2, ConfigFile v2, or Directory v2. These are "component resources" and the option simply doesn't propagate.

The retainOnDelete resource option does not apply to component resources, and will not have the intended effect.

Here's a related issue to better document this limitation: pulumi/docs#11663

I would suggest the following workaround, use the transforms option to add the retainOnDelete option to each of the children. Here's some information that might be useful.

thx @EronWright, good to know. i also workaround with pulumi.Transformations option like this:

pulumi.Transformations([]pulumi.ResourceTransformation{
    func(args *pulumi.ResourceTransformationArgs) *pulumi.ResourceTransformationResult {
        return &pulumi.ResourceTransformationResult{
            Props: args.Props,
            Opts:  append(args.Opts, pulumi.RetainOnDelete(true)),
        }
    },
}),

blampe commented 3 months ago

I looked into this some more and can confirm the engine does not automatically propagate retainOnDelete to children. I verified this with a dynamic node provider, so I don't think this issue is unique to k8s.

The amusingly inconsistent docs:

The default is to inherit this value from the parent resource, and false for resources without a parent. The retainOnDelete resource option does not apply to component resources, and will not have the intended effect.

seem to be making a statement about how we would hope for this to behave, and a caveat that this isn't actually the case.

However! The provider can still explicitly propagate the option for you. I have https://github.com/pulumi/pulumi-kubernetes/pull/3078 which starts to do that, and it seems to behave as expected:

Previewing destroy (dev):
     Type                                                 Name                                 Plan               Info
 -   pulumi:pulumi:Stack                                  pulumi-kubernetes-3065-dev           delete             3 warnings
 -   └─ kubernetes:helm.sh/v4:Chart                       p-k-3065                             delete[retain]
 -      ├─ kubernetes:core/v1:Service                     p-k-3065:default/p-k-3065-nginx      delete[retain]
 -      ├─ kubernetes:apps/v1:Deployment                  p-k-3065:default/p-k-3065-nginx      delete[retain]
 -      ├─ kubernetes:core/v1:Secret                      p-k-3065:default/p-k-3065-nginx-tls  delete[retain]
 -      ├─ kubernetes:policy/v1:PodDisruptionBudget       p-k-3065:default/p-k-3065-nginx      delete[retain]
 -      ├─ kubernetes:networking.k8s.io/v1:NetworkPolicy  p-k-3065:default/p-k-3065-nginx      delete[retain]
 -      └─ kubernetes:core/v1:ServiceAccount              p-k-3065:default/p-k-3065-nginx      delete[retain]

although the CLI doesn't render any diffs when the option's value changes on children, so this still feels very much in the "not fully supported" category.

blampe commented 2 months ago

We discussed this internally and decided the recommendation for now is to use transformations to apply the desired resource options to your chart's resources. We can't guarantee that implementing a workaround in pulumi/kubernetes won't lead to more surprises down the road.

pulumi / pulumi-kubernetes