Enable tag drift 03 #1187
Enable tag drift 03 #1187
Conversation
|
Travis tests failing. Investigating now. |
| @@ -379,6 +379,12 @@ func (l *localState) setSyncState() error { | |||
| } | |||
|
|
|||
| // If our definition is different, we need to update it | |||
| if existing.EnableTagDrift { | |||
| l.logger.Printf("[DEBUG] Tag drift enabled.") | |||
| existing.Tags = service.Tags | |||
There was a problem hiding this comment.
At first I was a little weirded out that the local service is changing right here, but you have the lock and I'm not sure doing it elsewhere would be any cleaner. This looks ok.
There was a problem hiding this comment.
This does seem a bit odd though. I think it might be better to have an equality checker function rather than leaning on reflect.DeepEqual here. The current logic will also become problematic when switching to an in-memory state store (which is on our short-term roadmap), since the service is a pointer and you are actually swapping out part of the data here. Having an equality check function would also make this easier to test.
There was a problem hiding this comment.
@ryanuber That makes sense, but I believe it's a little out of scope for this particular pull request. I would be happy to contribute independent of this change.
There was a problem hiding this comment.
@ryanuber don't we require that the local data changes to match the server if drift is enabled so that we don't sync back to the old tags if the service definition changes for some other reason? Also, this is the localState which just has maps vs. memdb. Want to make sure I understand your concern before we pull the trigger on this one.
There was a problem hiding this comment.
@slackpad if we are doing a DeepEqual, then you are right, the definitions have to be perfectly equivalent. What I was suggesting was having like a ServiceEquals(s1, s2 *structs.NodeService, drift bool) bool kind of function, which would just skip the tag check if drift is enabled.
You're right about the local state, my bad.
One other thing though - If we are modifying the local agent state then we aren't enabling drift, we are instead flipping it so that the remote state always wins, unless I'm missing something else here.
There was a problem hiding this comment.
@ryanuber I don't think I explained myself very well re-reading what I wrote :-)
I was thinking that it's important we do the copy here because if DeepEqual detects a change after that (a real change we want to sync), we want the tags from the server going back up to the catalog, not the local ones as if we had the compare function. I think if we didn't copy then we'd revert the drifted tags once the service changes for any other reason.
|
@sfncook the basic logic looks ok to me - can you please add a unit test that exercises the new functionality? |
|
@slackpad @ryanuber - Thanks for the review, guys. @slackpad - I have added a couple test cases in TestAgentAntiEntropy_EnableTagDrift. But perhaps you can see in this conversation that the Travis failing with errors that don't appear to be related to my updates. I'm not sure why they are failing - they seems to be complaining that an interface is being used. (in code that I did not modify) Perhaps you guys have seen these errors before? Known issue? Travis errors: |
|
@sfncook can you please update the docs along with this change? |
| @@ -379,6 +379,12 @@ func (l *localState) setSyncState() error { | |||
| } | |||
|
|
|||
| // If our definition is different, we need to update it | |||
| if existing.EnableTagDrift { | |||
| l.logger.Printf("[DEBUG] Tag drift enabled.") | |||
There was a problem hiding this comment.
Should blow away these debug prints.
|
@sfncook - looking good! I noted a few small things but I think this is almost ready to go. |
|
@slackpad - Thanks so much for the thoughtful and thorough review! I will make these updates tomorrow. |
|
@slackpad I just pushed updates in response to your comments. (thank again) Note that the title of this pull request and branch still refer to the old config item name "EnableTagDrift", though the code has been updated per your request. |
|
Hang on just another day. I'd like to add a reference to the anti-entropy.html page in the docs. |
|
Okay, all done. |
| @@ -73,6 +74,25 @@ from `1`. | |||
|
|
|||
| Note: there is more information about [checks here](/docs/agent/checks.html). | |||
|
|
|||
| The `enableTagOverride` can optionally specified to disable the antientropy feature for | |||
There was a problem hiding this comment.
can optionally be specified
anti-entropy
|
Super tiny nits in the doc and then I think we can |
.
|
@slackpad Awesome, just pushed those changes. Thanks. |
|
Sweet - thanks for putting this together! It'll go out with Consul 0.6. |
|
Woo! Thanks for the feedback. What's a general ETA on 0.6? |
|
np - It's hard to say right now but we are pushing for this month. |
|
Perfect |
This is just another attempt to add the 'tag drift' feature in. Issue #1102 Originally I had this config item for the entire Consul agent. This iteration makes it a service-level config so individual services can be configured to allow tag drift.
"tag drift" = Disabling the anti-entropy sync agent behavior so that external agents can modify a service's tags. Useful for interactions like Redis and Redis Sentinel.