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

fix: Termination of pull transfer process from consumer side not success #4688

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

AndrYurk
Copy link
Contributor

What this PR changes/adds

Start termination process from data flow manager for provider when termination initiated by consumer

Why it does that

During the termination of PULL transfer process from consumer, system does not call method terminate() from DataFlowManager on provider side.
As a result process not terminated in real but transfer process had state "TERMINATED", and then it is not possible to initiate terminating process again not from consumer neither form provider side.

Further notes

After provider moved to TERMINATED state, it also should move to the DEPROVISIONING automatically

Linked Issue(s)

Closes #4592

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@AndrYurk AndrYurk requested review from ndr-brt and removed request for ndr-brt December 20, 2024 12:23
@AndrYurk AndrYurk changed the title Dos 1442 termination of pull transfer process from consumer side fix: Termination of pull transfer process from consumer side not success in real but stated as "TERMINATED" Dec 20, 2024
@AndrYurk AndrYurk changed the title fix: Termination of pull transfer process from consumer side not success in real but stated as "TERMINATED" fix: Termination of pull transfer process from consumer side not success Dec 20, 2024
@AndrYurk AndrYurk added the bug Something isn't working label Dec 20, 2024
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of tests changed in a critical area of the codebase. Can you explain what those changes are and also, why @Nested is being used?

@AndrYurk
Copy link
Contributor Author

AndrYurk commented Dec 20, 2024

@jimmarino, I used @Nested to combine all the tests related to the termination process (similar to what was done for Suspended, StartedResumed, and Started).

Actually, only one new test, provider_shouldTerminateDataFlowAndTransitionToDeprovisioning, was added to check the termination of the data flow and the transition to the "DEPROVISIONING" state for the provider transfer process.
The other three tests were left unchanged, they were simply moved and renamed.

@AndrYurk AndrYurk requested a review from jimmarino December 20, 2024 13:25
@jimmarino
Copy link
Contributor

@jimmarino, I used @Nested to combine all the tests related to the termination process (similar to what was done for Suspended, StartedResumed, and Started).

Actually, only one new test, provider_shouldTerminateDataFlowAndTransitionToDeprovisioning, was added to check the termination of the data flow and the transition to the "DEPROVISIONING" state for the provider transfer process. The other three tests were left unchanged, they were simply moved and renamed.

Please do not do that, as it makes it difficult to review this PR. A PR should be limited and focused on the change, particularly in a critical part of the codebase. Please refocus this PR only on the change and tests to verify it, maintaining the test structure that is currently in place.

@AndrYurk
Copy link
Contributor Author

@jimmarino structure of tests was rollbacked

Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to step back and go through what the expected behavior is when termination does not happen cleanly. The current PR will leave the connector in an inconsistent state if the data plane (or 'DataFlowManager`) returns a failure. For example, by leaving provisioned resources in place and not updating the transfer process.

Ideally, this would be handled by continuing the cleanup on the controller side and sending a data plane failure event to be reconciled by a listener. However, I'd prefer to discuss and design this before running into an implementation.

I'm going to mark the associated issue as triage so the committers can discuss this at our next meeting and come up with a plan for how to proceed.

@AndrYurk
Copy link
Contributor Author

@jimmarino thank you, I look forward to the decision.
I have one question about quality gate, currently one of the End-To-End-Tests pullFromHttp_httpProvision() is failing, but I did not touch it at all. Looks like MockServer try to use Address that already in use. Could run test be initiated again manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Termination of pull transfer process from consumer side not success in real but stated as "TERMINATED".
2 participants