Add an OV re-registration window option when using DB storage #643
Conversation
7flying
force-pushed
the
re-run-to0-before-expiration
branch
2 times, most recently
from
48cf742 to
eb44859
Compare
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
eb44859 to
c65a922
Compare
This adds the required function signature to the trait which will query ovs in the trait implementors. Signed-off-by: Irene Diez <idiez@redhat.com>
Implements the `query_ovs_db_to2_performed_to0_less_than` trait in the Owner database types. Signed-off-by: Irene Diez <idiez@redhat.com>
In the directory this trait is not available, so we return the specific `StoreError::MethodNotAvailable` error. Signed-off-by: Irene Diez <idiez@redhat.com>
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
c65a922 to
0f47b00
Compare
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
0f47b00 to
54cbc0e
Compare
There was a problem hiding this comment.
Having to implement query_ovs_db_to2_performed_to0_less_than function 6 times in which only 2 of them are "real" implementations and 4 just throw an error feels very wrong to me. Imagine we need to implement another database backend (e.g. mysql), now we have 9 implementations: 3 real, 6 throwing errors. I think we need to revisit the whole thing sooner than later.
owner-onboarding-server/src/main.rs
Outdated
| // Voucher registration times | ||
| let ov_registration_period = match settings.ov_registration_period { | ||
| Some(value) => { | ||
| if value == 0 { | ||
| bail!("ov_registration_period cannot be 0"); | ||
| } | ||
| value | ||
| } | ||
| None => { | ||
| log::info!( | ||
| "Setting a default ov_registration_period of {DEFAULT_REGISTRATION_PERIOD} seconds" | ||
| ); | ||
| DEFAULT_REGISTRATION_PERIOD | ||
| } | ||
| }; | ||
| let ov_re_registration_window = match settings.ov_re_registration_window { | ||
| Some(value) => { | ||
| if value == 0 { | ||
| bail!("ov_re_registration_window cannot be 0"); | ||
| } | ||
| value | ||
| } | ||
| None => { | ||
| log::info!("Setting a default ov_re_registration_window of {DEFAULT_RE_REGISTRATION_WINDOW} seconds"); | ||
| DEFAULT_RE_REGISTRATION_WINDOW | ||
| } | ||
| }; | ||
|
|
||
| if ov_re_registration_window >= ov_registration_period { | ||
| bail!( | ||
| "ov_re_registration_window ({ov_re_registration_window}) must be smaller than ov_registration_period ({ov_registration_period})"); | ||
| } else { | ||
| log::info!("Server configured with an OV registration period of {ov_registration_period} seconds, OV re-registration window set to {ov_re_registration_window} seconds") | ||
| } | ||
|
|
There was a problem hiding this comment.
As an enhancement this might be moved to a validation function so it can be used to validate clap arguments also.
There was a problem hiding this comment.
@mmartinv I'm guessing that this is it, let me explain since I did not comment.
I have a pending issue to add sqlite support as an optional argument when running aio (#642 ), as part of that I would have to change the passed arguments to the aio command, and it makes sense to add a way to configure other things (such as this new registration window there), so, yeah I made a mental note of this but did not comment. My apologies.
I do agree, we should get rid of the |
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
54cbc0e to
e01aebc
Compare
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
e01aebc to
240e267
Compare
This adds the ability to trigger a rendezvous re-registration when an OV is close to its de-registration timeout. A re-registration window is added so that when the remaining registration period of the OV reaches that window, a re-registration is triggered. This feature is only available when using database storage drivers. Signed-off-by: Irene Diez <idiez@redhat.com>
This adds the configuration options for OV re-registration window checks in aio. Signed-off-by: Irene Diez <idiez@redhat.com>
Signed-off-by: Irene Diez <idiez@redhat.com>
7flying
force-pushed
the
re-run-to0-before-expiration
branch
from
240e267 to
626488d
Compare
There was a problem hiding this comment.
Looks good to me. Just a nitpick: you marked a conversation as resolved after an ack but you didn't take action on it. If you are not willing to implement the suggested change because it was described as an enhancement and you considered it as not important just say it, do not just resolve the conversation without further explanation, it feels awkard.
I missed that, let me review and create an issue so that it is not forgotten |
OK, commented in the review since I missed to comment what I was thinking |
This allows the users to specify an OV re-registration window in the Owner server. When an OV is about to be de-registered in the rendezvous server within the specified time window, the owner will trigger a re-registration.
Fixes #193
Obsoletes #207