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

If a rollout fails to be created, any subsequently created rollouts will also fail to create #1049

Open
a-sayyed opened this issue Nov 19, 2020 · 0 comments

Comments

@a-sayyed
Copy link
Contributor

a-sayyed commented Nov 19, 2020

Due to the current way of handling Rollouts creation, the RolloutScheduler calls JpaRolloutManagement::handleRollouts which simply iterates over all active Rollouts of the current tenant

rollouts.forEach(rolloutId -> DeploymentHelper.runInNewTransaction(txManager, handlerId + "-" + rolloutId,
                    status -> executeFittingHandler(rolloutId)));

The problem happens if an exception is thrown inside the executeFittingHandler() call. The forEach loop is then broken, and any subsequent Rollouts that are in an active state (CREATING, DELETING, STARTING, READY, or RUNNING) will not be handled. This is because the runInNewTransaction() still runs on the same (main) thread

To reproduce:

    @Mock
    private TransactionCallback<String> problematicCallback;

    @Autowired
    private PlatformTransactionManager txManager;

    @Before
    public void setup() {
        when(problematicCallback.doInTransaction(any()))
                .thenThrow(new RuntimeException("oops"))
                .thenReturn("success");
    }
    
    @Test
    public void test() {
        try {
            IntStream.range(0, 3)
                    .mapToObj(String::valueOf)
                    .forEach(rolloutId -> {
                        DeploymentHelper.runInNewTransaction(txManager, rolloutId, problematicCallback);
                        fail("Execution should not reach here");
                    });
        } catch (RuntimeException e) {
            // expected
        }

       // invoked only once because the loop was broken
        verify(txManager, times(1)).commit(any());
    }

We should instead make sure that any Rollout handling is executed in a try-catch block, so that we don't block any subsequent Rollouts from being handled

    @Test
    public void test() {
        IntStream.range(0, 3).mapToObj(String::valueOf).forEach(rolloutId -> {
            try {
                final String result = DeploymentHelper.runInNewTransaction(txManager, rolloutId, problematicCallback);
                assertThat(result).isEqualTo("success");
            } catch (RuntimeException e) {
                System.err.println("skipping " + rolloutId);
            }
        });

        verify(txManager, times(3)).commit(any());
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants