-
Notifications
You must be signed in to change notification settings - Fork 148
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
lvmd: fix Watch RPC to notify VG info periodically when embedded #843
Conversation
bc5a3b0
to
bb7c415
Compare
The Watch RPC of lvmd's VGService is expected to notify VG information periodically to its watchers. However, when lvmd is embedded into topolvm-node, Watch RPC sends VG info only once on startup time and does nothing after that unless LVService invokes LVM commands for RPCs. This commit addresses the issue. Fixes: c5f17a4 ("feat: implement embed-lvmd") Signed-off-by: Daichi Mukai <daichi-mukai@cybozu.co.jp>
bb7c415
to
911cde5
Compare
@@ -29,6 +31,20 @@ func NewEmbeddedServiceClients(ctx context.Context, dcmapper *DeviceClassManager | |||
log.FromContext(ctx).Error(err, "embedded channel watch error") | |||
} | |||
}() | |||
|
|||
go func() { | |||
ticker := time.NewTicker(10 * time.Minute) |
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.
Thanks for the fix idea! QQ: How does the regular trigger for notify work on non-embedded mode? Im wondering how its able to do this refresh in the non-embedded mode
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.
NVM, just found
Lines 105 to 117 in baaea34
go func() { | |
ticker := time.NewTicker(10 * time.Minute) | |
for { | |
select { | |
case <-ctx.Done(): | |
ticker.Stop() | |
grpcServer.GracefulStop() | |
return | |
case <-ticker.C: | |
notifier() | |
} | |
} | |
}() |
Should we extract this into a common method and also make the time interval modifiable now?
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.
Yes, that's a great idea. But I would like to leave that as a separate task, as my priority is to merge this fix and include it in next month's release.
The Watch RPC of lvmd's VGService is expected to notify VG information periodically to its watchers. However, when lvmd is embedded into topolvm-node, Watch RPC sends VG info only once on startup time and does nothing after that unless LVService invokes LVM commands for RPCs. This commit addresses the issue.
Fixes: c5f17a4 ("feat: implement embed-lvmd")