-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Edge trigger node updates #3873
Conversation
1313d88
to
f4ba5f6
Compare
f4ba5f6
to
7b0f18b
Compare
client/client.go
Outdated
|
||
var nodeHasChanged bool | ||
|
||
for name, new_val := range response.Attributes { |
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.
Go style would be newVal
if first.IOPS != second.IOPS { | ||
return false | ||
} | ||
if len(first.Networks) != len(second.Networks) { |
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.
This just checks the length?
client/client.go
Outdated
// watchNodeUpdates blocks until it is edge triggered. Once triggered, | ||
// it will update the client node copy and re-register the node. | ||
func (c *Client) watchNodeUpdates() { | ||
c.logger.Printf("[DEBUG] client: starting process to watch for node updates.") |
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.
Remove
client/client_test.go
Outdated
if changed, _, _ := c.hasNodeChanged(attrHash, metaHash); changed { | ||
t.Fatalf("Unexpected hash change.") | ||
} | ||
// these constants are only defined when nomad_test is enabled, so these fail |
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.
I fixed that so the comment isn't accurate and you probably don't need the nolint
client/client_test.go
Outdated
go c1.watchNodeUpdates() | ||
c1.updateNode() | ||
// This needs to be directly called as otherwise the client hangs on | ||
// attempt to register with a server. S[ecifically, retryRegisterNode is |
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.
Spelling error
client/client_test.go
Outdated
// attempt to register with a server. S[ecifically, retryRegisterNode is | ||
// blocking | ||
|
||
// test that the client's copy of the node is also updated |
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.
Capitalize the start of your comments
client/fingerprint_manager_test.go
Outdated
|
||
err := fm.Run() | ||
require.Nil(err) | ||
require.NotEqual(0, node.Resources.CPU) |
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.
NotZero
client/client.go
Outdated
@@ -1506,6 +1517,9 @@ func (c *Client) watchNodeUpdates(attrHash, metaHash uint64) { | |||
|
|||
c.retryRegisterNode() |
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.
IIUC, If this retryRegister method returns without registering because it timed out, it won't try again until the next node update comes through triggerNodeUpdate. Is that desirable?
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.
@dadgar and I talked about this. The node should be registered before watching for updates and the functionality that comes below, if I'm understanding this comment correctly.
test update config copy trigger
8950da5
to
21fa3cd
Compare
21fa3cd
to
ac91ee4
Compare
ac91ee4
to
cd5be3b
Compare
client/client.go
Outdated
@@ -209,6 +212,7 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic | |||
allocUpdates: make(chan *structs.Allocation, 64), | |||
shutdownCh: make(chan struct{}), | |||
triggerDiscoveryCh: make(chan struct{}), | |||
triggerNodeUpdate: make(chan struct{}, 64), |
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.
I would drop to like 8
client/client.go
Outdated
return false | ||
} | ||
f := second.Networks[i] | ||
if e != f { |
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.
This comparison is a pointer comparison and not a value comparison?
client/client.go
Outdated
|
||
hasChanged = false | ||
syncTicker.Stop() | ||
syncTicker = time.NewTicker(c.retryIntv(nodeUpdateRetryIntv)) |
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.
Use a timer and reset
client/client_test.go
Outdated
// our linter without explicit disabling. | ||
c1 := TestClient(t, func(c *config.Config) { | ||
c.Options = map[string]string{ | ||
driver.ShutdownPeriodicAfter: "true", // nolint: varcheck |
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.
Don't need the nolint
remove impossible test case
client/client.go
Outdated
hasChanged = false | ||
timer.Reset(c.retryIntv(nodeUpdateRetryIntv)) | ||
case <-c.triggerNodeUpdate: | ||
hasChanged = true |
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.
Why not put all timer logic in this case and then remove the if statement at the top of the timer case and the reset at the bottom:
case <-c.triggerNodeUpdate:
if hasChanged {
continue
}
hasChanged = true
timer.Reset(c.retryIntv(nodeUpdateRetryIntv))
client/client.go
Outdated
c.logger.Printf("[DEBUG] client: state changed, updating node.") | ||
case <-timer.C: | ||
if !hasChanged { | ||
continue |
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.
Right now this code could never fire even if there is a node update
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Refactor to move client update node mechanism to be edge triggered instead of periodic.
See here for original comment: #3781 (diff)