Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception thrown in stack results in resources marked for deletion #7050

Closed
dferretti opened this issue May 14, 2021 · 16 comments · Fixed by #7299
Closed

Exception thrown in stack results in resources marked for deletion #7050

dferretti opened this issue May 14, 2021 · 16 comments · Fixed by #7299
Assignees
Labels
impact/panic This bug represents a panic or unexpected crash language/dotnet
Milestone

Comments

@dferretti
Copy link
Contributor

dferretti commented May 14, 2021

If an exception happens while executing my stack code, it causes the program to exit, and any resource that would have been defined after the exception gets marked for deletion (or worse actually deleted). I have only tried this in the dotnet SDK.

Expected behavior

I would have expected the program exiting on error would cause pulumi to not move forward with the preview/up operation.

Current behavior

Currently, pulumi does see the exception, and logs it as a diagnostic. But it then continues as if your program was complete and any resources that weren't yet created act as if you were removing them from your source code / stack.

Steps to reproduce

public class Program
{
    public static async Task Main(string[] args)
        => await Deployment.RunAsync<MyStack>();
}

public class MyStack : Stack
{
    public MyStack()
    {
        var a = new Pulumi.Random.RandomInteger("a", new Pulumi.Random.RandomIntegerArgs { Min = 1, Max = 100 });

        if (Environment.GetEnvironmentVariable("oops") == "yes") throw new Exception("oops");

        var b = new Pulumi.Random.RandomInteger("b", new Pulumi.Random.RandomIntegerArgs { Min = 1, Max = 100 });

        var c = new Pulumi.Random.RandomInteger("c", new Pulumi.Random.RandomIntegerArgs { Min = 1, Max = 100 });
    }
}

successful pulumi up :

root@5ecb18015b1a:/app# oops=no pulumi up -y --non-interactive
Previewing update (test):
    pulumi:pulumi:Stack test-test  running 'dotnet build -nologo .'
    pulumi:pulumi:Stack test-test    Restore completed in 36.76 ms for /app/PulumiIssue.csproj.
    pulumi:pulumi:Stack test-test    PulumiIssue -> /app/bin/Debug/netcoreapp3.1/PulumiIssue.dll
    pulumi:pulumi:Stack test-test
    pulumi:pulumi:Stack test-test      0 Warning(s)
    pulumi:pulumi:Stack test-test  'dotnet build -nologo .' completed successfully

 +  pulumi:pulumi:Stack test-test create 'dotnet build -nologo .' completed successfully
 +  random:index:RandomInteger a create
 +  random:index:RandomInteger c create
 +  random:index:RandomInteger b create
 +  pulumi:pulumi:Stack test-test create

Resources:
    + 4 to create

Updating (test):
    pulumi:pulumi:Stack test-test  running 'dotnet build -nologo .'
    pulumi:pulumi:Stack test-test    Restore completed in 34.75 ms for /app/PulumiIssue.csproj.
    pulumi:pulumi:Stack test-test    PulumiIssue -> /app/bin/Debug/netcoreapp3.1/PulumiIssue.dll
    pulumi:pulumi:Stack test-test
    pulumi:pulumi:Stack test-test      0 Warning(s)
    pulumi:pulumi:Stack test-test  'dotnet build -nologo .' completed successfully

 +  pulumi:pulumi:Stack test-test creating 'dotnet build -nologo .' completed successfully
 +  random:index:RandomInteger c creating
 +  random:index:RandomInteger a creating
 +  random:index:RandomInteger b creating
 +  random:index:RandomInteger c created
 +  random:index:RandomInteger a created
 +  random:index:RandomInteger b created
 +  pulumi:pulumi:Stack test-test created

Resources:
    + 4 created

Duration: 3s

followed by a failed preview attempt:

root@5ecb18015b1a:/app# oops=yes pulumi preview --non-interactive
Previewing update (test):
    pulumi:pulumi:Stack test-test  running 'dotnet build -nologo .'
    pulumi:pulumi:Stack test-test    Restore completed in 38.84 ms for /app/PulumiIssue.csproj.
    pulumi:pulumi:Stack test-test    PulumiIssue -> /app/bin/Debug/netcoreapp3.1/PulumiIssue.dll
    pulumi:pulumi:Stack test-test
    pulumi:pulumi:Stack test-test      0 Warning(s)
    pulumi:pulumi:Stack test-test  'dotnet build -nologo .' completed successfully

    pulumi:pulumi:Stack test-test running 'dotnet build -nologo .' completed successfully
    random:index:RandomInteger a
    pulumi:pulumi:Stack test-test running error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
 -  random:index:RandomInteger c delete
 -  random:index:RandomInteger b delete
    pulumi:pulumi:Stack test-test  1 error

Diagnostics:
  pulumi:pulumi:Stack (test-test):
    error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
    System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
     ---> System.Exception: oops
       at PulumiIssue.MyStack..ctor() in /app/Program.cs:line 21
       --- End of inner exception stack trace ---
       at System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean wrapExceptions, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& hasNoDefaultCtor)
       at System.RuntimeType.CreateInstanceDefaultCtorSlow(Boolean publicOnly, Boolean wrapExceptions, Boolean fillCache)
       at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
       at System.Activator.CreateInstance[T]()
       at Pulumi.Deployment.Runner.<>c__4`1.<RunAsync>b__4_0()
       at Pulumi.Deployment.Runner.RunAsync[TStack](Func`1 stackFactory)

or if you aren't careful, a problematic up:

root@5ecb18015b1a:/app# oops=yes pulumi up -y --non-interactive
Previewing update (test):
    pulumi:pulumi:Stack test-test  running 'dotnet build -nologo .'
    pulumi:pulumi:Stack test-test    Restore completed in 35.49 ms for /app/PulumiIssue.csproj.
    pulumi:pulumi:Stack test-test    PulumiIssue -> /app/bin/Debug/netcoreapp3.1/PulumiIssue.dll
    pulumi:pulumi:Stack test-test
    pulumi:pulumi:Stack test-test      0 Warning(s)
    pulumi:pulumi:Stack test-test  'dotnet build -nologo .' completed successfully

    pulumi:pulumi:Stack test-test running 'dotnet build -nologo .' completed successfully
    random:index:RandomInteger a
    pulumi:pulumi:Stack test-test running error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
 -  random:index:RandomInteger c delete
 -  random:index:RandomInteger b delete
    pulumi:pulumi:Stack test-test  1 error

Diagnostics:
  pulumi:pulumi:Stack (test-test):
    error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
    System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
     ---> System.Exception: oops
       at PulumiIssue.MyStack..ctor() in /app/Program.cs:line 21
       --- End of inner exception stack trace ---
       at System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean wrapExceptions, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& hasNoDefaultCtor)
       at System.RuntimeType.CreateInstanceDefaultCtorSlow(Boolean publicOnly, Boolean wrapExceptions, Boolean fillCache)
       at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
       at System.Activator.CreateInstance[T]()
       at Pulumi.Deployment.Runner.<>c__4`1.<RunAsync>b__4_0()
       at Pulumi.Deployment.Runner.RunAsync[TStack](Func`1 stackFactory)


Updating (test):
    pulumi:pulumi:Stack test-test  running 'dotnet build -nologo .'
    pulumi:pulumi:Stack test-test    Restore completed in 37.44 ms for /app/PulumiIssue.csproj.
    pulumi:pulumi:Stack test-test    PulumiIssue -> /app/bin/Debug/netcoreapp3.1/PulumiIssue.dll
    pulumi:pulumi:Stack test-test
    pulumi:pulumi:Stack test-test      0 Warning(s)
    pulumi:pulumi:Stack test-test  'dotnet build -nologo .' completed successfully

    pulumi:pulumi:Stack test-test running 'dotnet build -nologo .' completed successfully
    random:index:RandomInteger a
    pulumi:pulumi:Stack test-test running error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
 -  random:index:RandomInteger c deleting
 -  random:index:RandomInteger b deleting
 -  random:index:RandomInteger c deleted
 -  random:index:RandomInteger b deleted
    pulumi:pulumi:Stack test-test **failed** 1 error

Diagnostics:
  pulumi:pulumi:Stack (test-test):
    error: Running program '/app/bin/Debug/netcoreapp3.1/PulumiIssue.dll' failed with an unhandled exception:
    System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
     ---> System.Exception: oops
       at PulumiIssue.MyStack..ctor() in /app/Program.cs:line 21
       --- End of inner exception stack trace ---
       at System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean wrapExceptions, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& hasNoDefaultCtor)
       at System.RuntimeType.CreateInstanceDefaultCtorSlow(Boolean publicOnly, Boolean wrapExceptions, Boolean fillCache)
       at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, Boolean wrapExceptions)
       at System.Activator.CreateInstance[T]()
       at Pulumi.Deployment.Runner.<>c__4`1.<RunAsync>b__4_0()
       at Pulumi.Deployment.Runner.RunAsync[TStack](Func`1 stackFactory)

Resources:
    - 2 deleted
    2 unchanged

Duration: 3s

Context (Environment)

This tripped me up as I was iterating on a stack I have been working on (luckily no real resources were harmed). Towards the beginning of my stack I usually config.Require all of my inputs. When I misspelled one of my config names, it failed super early and therefore pretty much every resource was marked for deletion.

This test was run in the pulumi/pulumi:v3.2.1 docker container.

@dferretti dferretti added the kind/bug Some behavior is incorrect or out of spec label May 14, 2021
@orionstudt
Copy link
Contributor

@vipentti curious what you think of this.

The first thing I see is traditional generic TStack programs run via this function:

public Task<int> RunAsync<TStack>(Func<TStack> stackFactory) where TStack : Stack

And notice they have a try/catch and pass exceptions into HandleExceptionAsync.

While the function that inline programs would use, directly beneath it:

public Task<int> RunAsync(Func<Task<IDictionary<string, object?>>> func, StackOptions? options)

Doesn't do that. I don't know if that is important or not.

@vipentti
Copy link
Contributor

@orionstudt I created two Automation.Api.Tests in #7057 which both fail similarly as reported here. So it happens with both inline program as well as the TStack version

@orionstudt
Copy link
Contributor

@vipentti you are correct it is both. @dferretti is a coworker of mine and when he initially described the issue to me I incorrectly interpreted that he was using Automation API and that it had been a bug I introduced with that implementation. But you're right, it's both, so that is probably worse 😬

@vipentti
Copy link
Contributor

I ran the tests against earlier versions of the cli and automation API and the tests fail there as well. Looks like this bug may have been there for a while.

@gitfool
Copy link
Contributor

gitfool commented May 16, 2021

I think I hit this while messing with my stack outputs. I inadvertently introduced an exception and the whole stack got deleted.

@lukehoban lukehoban added p1 Bugs severe enough to be the next item assigned to an engineer impact/panic This bug represents a panic or unexpected crash language/dotnet labels May 16, 2021
@lukehoban lukehoban added this to the 0.57 milestone May 27, 2021
@lukehoban lukehoban added emergent and removed kind/bug Some behavior is incorrect or out of spec labels May 27, 2021
@mikhailshilkov
Copy link
Member

mikhailshilkov commented May 31, 2021

I believe this behavior has existed since the very first release of the .NET plugin.

The problem is that you return Task from the Main method and not Task<int>. It should be:

public static async Task<int> Main(string[] args)
{
  return await Deployment.RunAsync<MyStack>();
}

With this code, RunAsync would return 32 for your example and the deployment would fail appropriately. I believe we use the Task<int> variant in all templates and examples.

I agree this is a confusing and dangerous behavior, so I welcome any improvement suggestions. cc @t0yv0 for awareness.

@mikhailshilkov mikhailshilkov removed the p1 Bugs severe enough to be the next item assigned to an engineer label May 31, 2021
@orionstudt
Copy link
Contributor

Ah that is a tricky gotchya. That makes sense for the CLI / local programs. We should probably make sure that this is not happening with Automation API then, when we are controlling the program lifecycle

@mikhailshilkov
Copy link
Member

Indeed! Do you have a repro for this issue with the automation API?

@dferretti
Copy link
Contributor Author

dferretti commented Jun 1, 2021

Some off-the-wall ideas that could be helpful:

  • In the exception handler that returns 32 here
    return _processExitedAfterLoggingUserActionableMessage;
    it could also set Environment.ExitCode to the same value
    • don't do this, would break automation api
  • There could be a roslyn analyzer that could detect a Task Main and throw a compiler warning/error for the user

@orionstudt
Copy link
Contributor

@mikhailshilkov I believe @vipentti wrote some tests in #7057 verifying that this behavior is occurring in Automation API as well

@leezen leezen modified the milestones: 0.57, 0.58 Jun 7, 2021
@gitfool
Copy link
Contributor

gitfool commented Jun 10, 2021

Probably from doing silly things while playing around but I'm still hitting this every day. If I wasn't running preview first, or if I wasn't paying attention, then resources would be deleted. 😬

@mikhailshilkov
Copy link
Member

@gitfool Are you using the problematic pattern described in #7050 (comment) or what is your case?

@gitfool
Copy link
Contributor

gitfool commented Jun 14, 2021

@mikhailshilkov I'm using the dotnet automation api via a generic host:

public static class Program
{
    public static Task<int> Main(string[] args) =>
        CreateHostBuilder(args).RunCommandAsync(args);
}

...

protected override async Task<int> OnExecuteAsync(CommandContext context, Settings settings)
{
...
    var stackName = $"{Config.Pulumi.Organization.Name}/{settings.Environment.ToLower()}";
    var stackArgs = new InlineProgramArgs(info.ProjectName, stackName, PulumiFn.Create(ServiceProvider, info.StackType))
    {
        Logger = LoggerFactory.CreateLogger<Pulumi.Deployment>()
    };
    var stack = await LocalWorkspace.CreateOrSelectStackAsync(stackArgs);
...
    var result = await stack.UpAsync(
        new UpOptions
        {
            Diff = !settings.NoDiff,
            ExpectNoChanges = settings.ExpectNoChanges,
            OnStandardOutput = AnsiConsole.WriteLine
        });
    if (result.Summary.Result != UpdateState.Succeeded)
    {
        return -1;
    }
...
    return 0;
}

@mikhailshilkov
Copy link
Member

mikhailshilkov commented Jun 14, 2021

@gitfool thanks for the confirmation and the code.

@orionstudt @vipentti It sounds like this issue is especially bad for inline programs? Any thoughts on why?

@orionstudt
Copy link
Contributor

Possibly the LocalRuntimeService isn't returning to the CLI correctly

@orionstudt
Copy link
Contributor

@mikhailshilkov OK so #7299 should resolve this. I brought in one the tests that @vipentti wrote to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/panic This bug represents a panic or unexpected crash language/dotnet
Projects
None yet
7 participants