Skip to content

HDDS-7341. EC: Close pipelines with unregistered nodes#3850

Merged
sodonnel merged 3 commits intoapache:masterfrom
sodonnel:ec-HDDS-7341
Oct 19, 2022
Merged

HDDS-7341. EC: Close pipelines with unregistered nodes#3850
sodonnel merged 3 commits intoapache:masterfrom
sodonnel:ec-HDDS-7341

Conversation

@sodonnel
Copy link
Contributor

What changes were proposed in this pull request?

A datanode is stopped and before the stale node handler is triggered, SCM is restarted. When SCM restarts its loads all the only pipelines and nodes from RocksDB, and then all the nodes will register again.

In the case of EC pipelines, there is nothing to trigger the close of a pipeline (and the containers on it) except:

  1. The Container getting full and the DN triggering the close
  2. The stale / dead node handlers noticing a node on it has gone dead.

In the case above, the EC pipeline will sit forever in an Open state, but any attempt to write to it will likely result in errors on the client due to one of the nodes not being available. These errors still will not trigger it to close.

A solution to this problem, is to add logic to the pipeline scrubber to close any pipelines that have unregistered nodes. Stale / Dead nodes should be handled by the existing stale / dead node handlers.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7341

How was this patch tested?

New unit test and validated on a docker compose cluster.

@swagle swagle requested a review from errose28 October 18, 2022 15:41
Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @sodonnel. LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit. I see other tests doing this as well but I'm not sure it's necessary since createPipelineManager(boolean) is passing this variable to the PipelineManagerImpl constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I just copied the new test from an old one and adapted. I have removed that line from all the tests and they all pass locally.

Copy link
Member

Choose a reason for hiding this comment

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

At that time with Junit4, I had some issues with the test running, So I added it. I just checked now it is safe to remove. Thanks for cleaning it up @sodonnel.

@sodonnel sodonnel merged commit 977ab59 into apache:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments