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

(WIP) Make controller go through attribute manager to clear transient attributes. #1699

Closed
wants to merge 3 commits into from
Closed

Conversation

HideoYamauchi
Copy link
Contributor

@HideoYamauchi HideoYamauchi commented Feb 21, 2019

Hi All,

This is for discussion only.
I have confirmed the rough behavior, but the source has not been completed yet.
I think there are many missing fixes.

This PR is based on the following Ken's PR as a base.

I made the following changes as the first realization method.

It is unknown whether it can cope with clusters with many nodes or when there are many attributes.
In my modification, since clearing of attributes is handled in smaller units, improvement may be necessary.


  • Mark when receiving the ATTRD_OP_PEER_CLEAR message.
    In order to cope with crmd restarting, we will not immediately delete the attribute.
    For remote node attributes, delete them immediately.
    Deletes the attribute when the mark of the request to delete the attribute from crmd and the event of the node leaving attrd are collected.(It should be noted that the order in which this mark and the event occur may be reversed.)
    With this fix, attributes are always deleted via the ATTRD_OP_PEER_CLEAR message.
    Although this fix alone does not solve it, it seems to be effective even if only attrd is restarted in the future.

  • Add ATTRD_OP_ATTR_REMOVE as a new message.
    After completing the update of cib, the Writer is delete the memory attribute of attrd.
    At this time, send ATTRD_OP_ATTR_REMOVE to the attrd member of the cluster.
    Other attrds that received ATTRD_OP_ATTR_REMOVE also delete the memory attributes.
    In order to reliably synchronize the attributes in the cluster, we have made such a mechanism.

  • It does not incorporate the correspondence of the old CRM_ATTR_PROTOCOL version.

  • When the attrd leaving the cluster once again participates, the processing flag is cleared.
    It is when attrd received the CRM_ATTR_PROTOCOL message
    Actually, you should clear the processing flag when crmd comes in, but attrd can not detect it at the moment.

  • Forced write decision by force_write has been moved to write_attributes () write_attribute ().
    This is because writing from write_attribute () may occur....

  • For now it seems to be difficult to delete the attribute of an isolated attrd if crmd does not restart.

  • etc..


This PR has already found the next problem.

  • The flag is cleared incompletely, after crmd restarts, the attribute is deleted when attrd restarts.(Need something good way)
  • ATTRD_OP_PEER_CLEAR will not be sent from the crmd of the new DC node after the cluster without STONITH is broken.
    To @kgaillot : Correction to send ATTRD_OP_PEER_CLEAR message from new DC node is necessary. Do you know where you need to make corrections?

The problem of attrd remaining after this correspondence.

  • However, if Election is delayed, problems remain. (https://bugs.clusterlabs.org/show_bug.cgi?id=5358#c21).
    Currently, setting the "transition-delay" only prevents the problem.

  • Unlike this problem, we must also take into consideration that the attribute will be lost if only attrd is restarted in the future.
    The current attrd deletes the attribute when it starts up.
    Even if we exchange sync_response messages with other nodes, we reset them with NULL.
    As a result, when attrd restarts, its attributes disappear.
    Especially when configuring a cluster with a single node, the attribute is completely lost.
    For example, when restarting attrd, it may be effective not to erase the attribute by pacemakerd passing the parameter.


Best Regards,
Hideo Yamauchi.

@HideoYamauchi
Copy link
Contributor Author

HideoYamauchi commented Feb 28, 2019

IMO,I have not confirmed the correction method, but I think that it is better for Writer to erase it in a stroke in the same way as it did with erase_status_tag ().

@aleksei-burlakov
Copy link

@HideoYamauchi , @kgaillot may we merge this PR, or at least the #1693 ?

@HideoYamauchi
Copy link
Contributor Author

HideoYamauchi commented Feb 13, 2020

Hi aleksei,

This fix is fairly old and experimental and not completed.
Therefore, this fix cannot be merged.

I think we need to reconsider again.

Best Regards,
Hideo Yamauchi.

@aleksei-burlakov
Copy link

Hi aleksei,

This fix is fairly old and experimental and not completed.
Therefore, this fix cannot be merged.

I think we need to reconsider again.

Best Regards,
Hideo Yamauchi.

@HideoYamauchi , @kgaillot what if we merge the #1693 ? It should not make any regression.

@kgaillot
Copy link
Contributor

@HideoYamauchi , @kgaillot what if we merge the #1693 ? It should not make any regression.

Unfortunately it would reintroduce a crash at scale. For the time being it's a choice of lesser evils, normal operation of pacemaker at scale being more important than corner cases when daemons respawn.

This is a tricky problem. Moving the functionality to attrd makes sense and avoids issues with the controller respawning, and should have fewer corner cases, but we do have to watch out for the corner cases that do remain, and we have to handle backward compatibility in mixed-version clusters where some nodes have the new feature and some don't.

@HideoYamauchi
Copy link
Contributor Author

Hi Ken,

If there is a lot of demand for improvement of this problem, I think it is good to have the opportunity to tackle this problem again somewhere.

Best Regards,
Hideo Yamauchi.

aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Feb 27, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Feb 28, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c

* update_attrd_helper in the controld_delete_node_state

- I want to hit the case 'c' in the pcmk__node_attr_request
in the lib/common/attrd_client.c
I don't it expect to happen, so simply DO:
1) Understand why/how/from where we get other commands, e.g. for update.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 2, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c

* update_attrd_helper in the controld_delete_node_state

- I want to hit the case 'c' in the pcmk__node_attr_request
in the lib/common/attrd_client.c
I don't it expect to happen, so simply DO:
1) Understand why/how/from where we get other commands, e.g. for update.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 2, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c

* update_attrd_helper in the controld_delete_node_state

- I want to hit the case 'c' in the pcmk__node_attr_request
in the lib/common/attrd_client.c
I don't it expect to happen, so simply DO:
1) Understand why/how/from where we get other commands, e.g. for update.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 17, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c

* update_attrd_helper in the controld_delete_node_state

- I want to hit the case 'c' in the pcmk__node_attr_request
in the lib/common/attrd_client.c
I don't it expect to happen, so simply DO:
1) Understand why/how/from where we get other commands, e.g. for update.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 23, 2020
TODO:
1) Merge the update_attrd_clear_node
2) Port the daemons/attrd/attrd_commands.c

* update_attrd_helper in the controld_delete_node_state

- I want to hit the case 'c' in the pcmk__node_attr_request
in the lib/common/attrd_client.c
I don't it expect to happen, so simply DO:
1) Understand why/how/from where we get other commands, e.g. for update.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 23, 2020
- It is under development. There are things to fix and to discuss.
aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 24, 2020
- It is under development. There are things to fix and to discuss.
@aleksei-burlakov
Copy link

I would like to take over this PR in the #2020. @HideoYamauchi , I would appreciate your comments or any help with it=)

aleksei-burlakov pushed a commit to aleksei-burlakov/pacemaker that referenced this pull request Mar 25, 2020
- It is under development. There are things to fix and to discuss.
@HideoYamauchi
Copy link
Contributor Author

Hi aleksei,

I would like to take over this PR in the #2020. @HideoYamauchi , I would appreciate your comments or any help with it=)

Okay!
If you need my comments, please contact me.

Best Regards,
Hideo Yamauchi.

@kgaillot
Copy link
Contributor

I have a new attempt at this in progress and will submit a PR when it is ready.

@kgaillot kgaillot closed this Dec 13, 2023
@gao-yan
Copy link
Member

gao-yan commented Sep 10, 2024

I have a new attempt at this in progress and will submit a PR when it is ready.

Hi @kgaillot , @HideoYamauchi , is there a PR about this already, or am I missing any further progress? Thanks.

@kgaillot
Copy link
Contributor

I have a new attempt at this in progress and will submit a PR when it is ready.

Hi @kgaillot , @HideoYamauchi , is there a PR about this already, or am I missing any further progress? Thanks.

I'm about to pick this up again, had to divert to 3.0.0 compatibility breaks for a while. FYI I'm planning to release 2.1.9 in October and 3.0.0 after that.

@HideoYamauchi
Copy link
Contributor Author

@gao-yan

At present, I have left the consideration of this matter to Ken.

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.

4 participants