-
Notifications
You must be signed in to change notification settings - Fork 372
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
Do not update goal state when refreshing the host plugin #1741
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1741 +/- ##
===========================================
+ Coverage 68.39% 68.44% +0.04%
===========================================
Files 81 81
Lines 11741 11734 -7
Branches 1648 1645 -3
===========================================
+ Hits 8030 8031 +1
+ Misses 3369 3362 -7
+ Partials 342 341 -1
Continue to review full report at Codecov.
|
tests/protocol/mockwiredata.py
Outdated
# For the use of "(?<=" "(?=" see 7.2.1 in https://docs.python.org/3.1/library/re.html | ||
# For the use of "\g<1>" see backreferences in https://docs.python.org/3.1/library/re.html#re.sub | ||
# | ||
# Note that these regular expressions are not enough to _parse all valid XML documents (e.g. they do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "_parse"
tests/protocol/mockwiredata.py
Outdated
# | ||
# Note that these regular expressions are not enough to _parse all valid XML documents (e.g. they do | ||
# not account for metacharacters like < or > in the values) but they are good enough for the test | ||
# data. There are some basic checks, but the functions could could not match valid XML or produce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "could could not match" -> "may not match"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments but the major changes look good. Thanks for doing this change, it was much needed 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Do not update goal state when refreshing the host plugin * Fixing typos and undoing bad rename
We are updating the goal state too frequently and that is part of the reason different components are getting out of sync. One of the reasons we were forcing updates of the goal state was to retry calls to the host plugin when the container id or the role config change. In this case updating the full goal state is not needed and it can actually introduce consistency issues.
The main change in this PR is to define an API specifically to update the host plugin without changing the goal state.
Other small changes are minor cleanup of the update goal state in the metadata protocol and the parse code of the goal state; I'll add comments in the PR to explain those changes.
This change is