-
Notifications
You must be signed in to change notification settings - Fork 290
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
[FLOC-4452] Delay invariant checks until all diff operations have been applied #2839
Conversation
…r so as not to trigger the invariant check until all attributes have been set.
Well I think I've fixed one version of this error,
http://ci-live.clusterhq.com:8080/job/ClusterHQ-flocker/job/invariant-error-FLOC-4452/view/cron/job/_run_acceptance_on_AWS_CentOS_7_with_EBS/ does still have the "Either all or none of" invariant error, but the stack trace suggests that those instances were not using the updated code....which is worrying and inexplicable right now.
(note that there's no sign of the new |
There's another example of the invariant error in a different PClass
I need to look into that one. |
…ns in such a way as to trigger an invariant error
…ariant-error-FLOC-4452
Well I don't fully understand it, but there still seem to be invariant failures in some of our acceptance test environments:
And judging by the stack trace it seems that the acceptance test runner is installing the wrong package version
NB no sign of the new Which is a bit worrying! |
Ah! I know why. I think this is our test_upgrade test striking again The errors are occurring during the short period while the cluster is running the downgraded version of flocker. (1.13.0) |
@@ -341,7 +371,7 @@ def deployment_strategy( | |||
draw( | |||
lease_strategy( | |||
dataset_id=st.just(dataset_id), | |||
node_uuid=st.just(node_uuid) | |||
node_id=st.just(node_uuid) |
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.
Was this simply broken before? It looks like yes but it went unnoticed because with no "stateful" applications there were never any leases.
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.
Yep, it was broken and the node_strategy is also broken in the way it supplies UUID(...)
versus unicode(UUID(...))
to Dataset.dataset_id
....I need to investigate that.
I just added the new stateful_application
parameter to avoid triggering the bug in the existing tests.
Thanks Richard. Left my comments inline. Looks good for the most part though I wasn't able to follow all of the implementation (left comments where I was lost). The test suite looks good though so I'm happy to have you address the noted points to your satisfaction and then merge - apart from the issue with the upgrade test? What's the story there? Does that test just need to be made more robust as a separate unit of work? Thanks again. |
…r the special case where the root object is replaced with a new, unrelated object.
…rm is not _IEvolvable.
…parate proxies for set and record types
Ignoring:
|
|
||
class _IRecordType(Interface): | ||
""" | ||
The operations that can be performed when transforming a ``PSet`` object. |
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.
s/PSet/PMap/ probably
@wallrj Richard, sorry it took me this long to review this change and to produce just a few minor comments. It took me quite a while to understand all the changed and related code. I didn't realize that you merged the PR some hours ago, so this is a belated LGTM from me. |
Fixes: https://clusterhq.atlassian.net/browse/FLOC-4452