Uh oh!
There was an error while loading. Please reload this page.
Externalize kvm agent storage timeout configuration - #4585
Conversation
Created a class of constant agent's properties available to configure on "agent.properties". Created a class to provides a facility to read the agent's properties file and get its properties.
There was a problem hiding this comment.
Thanks for the PR @GutoVeronezi.
Is there any reason for not adding the new variable at agent.properties?
Normally, a PR adding a new variable would also add it on the properties file.
Example:
# commented lines describing variable
# commented (or not) line with the property name and default value
@GabrielBrascher sorry, my bad. I forgot to add the new variable to the file. I'll add it later. |
ravening
commented
Jan 19, 2021
@ustcweizhou do we have this param already? |
GabrielBrascher
commented
Jan 19, 2021
@ravening this parameter exists already but it is hardcoded in |
ravening
commented
Jan 19, 2021
@GabrielBrascher true but I know that Wei added several global settings to configure the values as well the preferred action as well in a much simpler way. I see in this PR that some new files are added to handle the properties which are not needed |
GutoVeronezi
commented
Jan 20, 2021
@ravening can you show me where this is externalized (if it is)? As far as we have seen in the code, there is no way for operators to configure this right now. Therefore, we need this patch to allow operators to configure the storage check timeout. |
@RodrigoDLopez@GabrielBrascher@DaanHoogland@sureshanaparti I'm pinging you here because you all reviewed PR 4586. First I want to apologize for this inconvenience. This PR and 4586 are some of my first PRs and I messed up they a little bit. Both PRs proposes differents changes, but they are linked by the new classes I marked 4586 as draft and applied yours suggestions in this PR. Also I added unit tests to the handler. If it be merged, I will rebase 4586. Could you review this PR? |
DaanHoogland
left a comment
There was a problem hiding this comment.
looks good, one question and one remark towards design;
q: properties are always read from file. Is that always what we want?
r: the use of the AgentProperties<> (plural) is to instantiate a single property. I think it makes sense to split the class in a container AgentProperties, and an element AgentProperty<>
code looks good
| # This parameter specifies the heartbeat update timeout in ms. | ||
| # Depending on the use case, this timeout might need increasing/decreasing. | ||
| # heartbeat.update.timeout=60000 No newline at end of file |
There was a problem hiding this comment.
I suppose 60000 is the default (1 min) wouldn't hurt to make that explicit in here for operator convenience.
| * Not all available agent properties are defined here, but we should work to | ||
| * migrate them on demand to this class. | ||
| * | ||
| * @param <T> type of the default value. |
There was a problem hiding this comment.
👍 useful javadoc, nice to see
| private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>(); | ||
| private final String _hostIP; /* private ip address */ | ||
| private final String hostPrivateIp; |
GutoVeronezi
commented
May 31, 2021
About the remark: About the question: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
GabrielBrascher
left a comment
There was a problem hiding this comment.
Thanks for the PR @GutoVeronezi, code LGTM.
DaanHoogland
commented
Jun 4, 2021
I don't know but I could immagine there could be properties that require a restart on change and forbid a re-read from file. things like connection settings for instance. I am not sure. It was a legitimate question. |
GutoVeronezi
commented
Jun 4, 2021
@DaanHoogland It's a good use case. |
DaanHoogland
commented
Jun 8, 2021
You are right and also, let's solve the issue when it really arises. this is good for now at least. |
blueorangutan
commented
Jun 8, 2021
Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 188 |
blueorangutan
commented
Jun 9, 2021
Trillian test result (tid-897)
|
GabrielBrascher
commented
Jun 10, 2021
There are some errors on Marvin's tests. Not sure if they are related to this PR, though. |
DaanHoogland
commented
Jun 11, 2021
started a second run, we'll see if the errors are consistent. |
blueorangutan
commented
Jun 11, 2021
Trillian test result (tid-915)
|
GutoVeronezi
commented
Jun 14, 2021
@DaanHoogland@GabrielBrascher is there anything else to do? |
DaanHoogland
commented
Jun 15, 2021
@GabrielBrascher regression tests look ok, did you test? |
yadvr
commented
Jun 17, 2021
LGTM do we need any manual testing? (automated tests LGTM but they may not cover the specific case) |
DaanHoogland
commented
Jun 18, 2021
Note that the threading model has changed @rhtyd , so yes manual testing would be needed, i'd say. I do not know any if automated tests exist for storage timeouts. |
GutoVeronezi
commented
Jun 28, 2021
Any update about this? |
RodrigoDLopez
commented
Jul 16, 2021
Hey @GutoVeronezi @DaanHoogland@GabrielBrascher @rhtyd I ran some manual tests on this branch, and as far as I could verify everything works correctly. After updating, I ran some tests changing the parameter value and checking if the default value would be used.
|
)" This reverts commit 05a978c.
I found an issue after merging this. addHost fails in several environments: I think we'll need to revert (which I'm testing now in a lab env) |
nvazquez
commented
Jul 19, 2021
Thanks @DaanHoogland seems like reverting on the internal branch made addHost work again. Please update once tests are completed so this can be reverted |
blueorangutan
commented
Jul 19, 2021
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 589 |
DaanHoogland
commented
Jul 19, 2021
the revert indeed works, I re-tested this PR with this ^^ latest build but am reverting this as other work is being blocked. @GutoVeronezi please rebase your code and submit a new PR after testing it. sorry about this. |
This reverts commit 05a978c.
blueorangutan
commented
Jul 19, 2021
Trillian Build Failed (tid-1318) |
GutoVeronezi
commented
Jul 19, 2021
@DaanHoogland np, let's get this working 😊. |
Description
On KVMHAMonitor, the heartbeat timeout is hardcoded to
60000ms; therefore, operators do not have options to choose which timeout they want/need.This PR intends to externalize the heartbeat timeout configuration on KVMHAMonitor.
Depending on the use case, this timeout might need increasing/decreasing. Therefore, it is interesting to externalize such configurations for cloud operators. The hardcoded value is taken as the default one. Therefore, the current behavior is maintained.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
It has been tested locally on a test lab.
hearbeat.update.timeoutsetting toagent.propertiesfile several times.