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 deploy of node in group #2872

Merged

Conversation

HiroyasuNishiyama
Copy link
Member

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Proposed changes

As described in this issue, changing node's group make the node restarted on deploy regardless of node deployment mode.
This PR tries to prevent nodes that only change groups from restarting.

Checklist

  • I have read the contribution guidelines
  • For non-bugfix PRs, I have discussed this change on the forum/slack team.
  • I have run grunt to verify the unit tests pass
  • I have added suitable unit tests to cover the new/changed functionality

@knolleary
Copy link
Member

The original issue describes the problem that:

  1. start with NodeA in a Group
  2. Add NodeB to Group
  3. Do modified nodes type deploy
  4. NodeA and NodeB are restarted

The expected behaviour is that only NodeB would get restarted in that case.

Can you clarify what this PR does? From your description, it sounds like you have changed it so that neither NodeA or NodeB will get restarted.

@HiroyasuNishiyama
Copy link
Member Author

As described in the original issue, NodeB (delay/trigger) is not restarted.
They continue running.

@coveralls
Copy link

coveralls commented Feb 12, 2021

Coverage Status

Coverage decreased (-0.05%) to 67.628% when pulling 12c7238 on node-red-hitachi:fix-node-deploy-with-group-change into f5da2eb on node-red:dev.

@knolleary
Copy link
Member

knolleary commented Feb 13, 2021

As described in the original issue, NodeB (delay/trigger) is not restarted.

In the original issue, NodeA is the delay/trigger node that is being restarted when NodeB is added to the same group.

The fix you are proposing will ignore any changes of a node's group when doing a nodes type deploy. That is not what is needed.

If a node changes the group it is in, then it should be restarted with all types of deploy.

The reported issues claims that all of the other nodes in the group are also being restarted - and that should not be happening.

@HiroyasuNishiyama
Copy link
Member Author

I seem to have misunderstood the request.
I will consider a fix.

@HiroyasuNishiyama
Copy link
Member Author

I think I have updated the fix according to your suggestion.

@kuema
Copy link

kuema commented Feb 15, 2021

Indeed, there are some cases when all nodes in a group get re-deployed, even when just changing the group's size. I didn't know if this was the expected behaviour.

If have posted my findings on Discourse last week, where this problem arised in a discussion about the inject node.
https://discourse.nodered.org/t/this-flow-is-generating-interesting-files/40637/7?u=kuema

@HiroyasuNishiyama
Copy link
Member Author

If have posted my findings on Discourse last week, where this problem arised in a discussion about the inject node.
https://discourse.nodered.org/t/this-flow-is-generating-interesting-files/40637/7?u=kuema

In the current implementation, changing the properties of a group is considered indirectly propagate to contained nodes.
I think this PR will fix the problems.

@knolleary
Copy link
Member

Thanks @HiroyasuNishiyama

I think this fix is good for now, but we may need to look again in the future.

At the moment, groups do not have any affect on the runtime. In the future, we will allow groups to hold meta-data about the nodes inside the group. Changing the meta-data of a group should mark the contents of the group as dirty and needing redeploying.

@knolleary knolleary merged commit 644da0b into node-red:dev Feb 16, 2021
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.

None yet

4 participants