-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow to specify total memory on agent configuration #3778
Conversation
d201e46
to
0156db7
Compare
if memInfo.Total > 0 { | ||
node.Attributes["memory.totalbytes"] = fmt.Sprintf("%d", memInfo.Total) | ||
if memInfo.Total > 0 { | ||
node.Attributes["memory.totalbytes"] = fmt.Sprintf("%d", memInfo.Total) |
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.
Also need to set memory.totalbytes
as node attributes when its configured via MemoryMB, and it should be converted to bytes
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.
done, see above
client/config/config.go
Outdated
@@ -91,6 +91,10 @@ type Config struct { | |||
// dynamically. It should be given as Cores * MHz (2 Cores * 2 Ghz = 4000) | |||
CpuCompute int | |||
|
|||
// MemoryMB is the default node total memory if it cannot be determined |
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.
Replace comment with MemoryMB is the default node total memory in megabytes if it..
, would rather make the documentation explicit though the variable name is a clue.
command/agent/config.go
Outdated
@@ -175,6 +175,9 @@ type ClientConfig struct { | |||
// CpuCompute is used to override any detected or default total CPU compute. | |||
CpuCompute int `mapstructure:"cpu_total_compute"` | |||
|
|||
// MemoryMB is used to override any detected or default total memory. | |||
MemoryMB int `mapstructure:"total_memory"` |
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.
Rename the config var memory_total_mb
here for the property to make it really explicit what the unit is, and to match cpu_total_compute
@@ -40,6 +40,7 @@ client { | |||
network_interface = "eth0" | |||
network_speed = 100 | |||
cpu_total_compute = 4444 | |||
total_memory = 5555 |
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.
Instead of modifying the base test fixture, add a new hcl test case file that has this property set. This is so that we can exercise the default(total_memory not set) in the basic tests.
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 is only a parsing test. It checks that the hlc file is properly converted to the data structure. This is never going to run the detection of memory on the platform.
If the memory_total_mb
is not set however, we should see a value set to 0
here instead.
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 what I mean - let it assert 0 when total_memory is not set for basic.hcl.
command/agent/config_test.go
Outdated
@@ -84,6 +84,7 @@ func TestConfig_Merge(t *testing.T) { | |||
}, | |||
NetworkSpeed: 100, | |||
CpuCompute: 100, | |||
MemoryMB: 200, |
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.
where is this coming from? Same with line 231
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 checks merging two configuration data structure works. Here we check that the values (thaty are arbitrary) are overriden by the other values (those below at line 231 probably)
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.
👍
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.
@mildred thanks for the PR. I added some review comments
0156db7
to
2ff2cde
Compare
Thank you for the quick review. Here is the pull request updated. Changes:
The test case in command/agent/config_test.go is modified because this test checks that two configuration structure can be merged together and that one set of value will replace the other set of values. Here the MemoryMB key was added so it is checked as well and the values are arbitrary for that test. |
Allow to set the total memory of an agent in its configuration file. This can be used in case the automatic detection doesn't work or in specific environments when memory overcommit (using swap for example) can be desirable.
2ff2cde
to
2be24f0
Compare
Closing this in favor of PR #4052 which addresses the merge conflict |
thank you |
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. |
Allow to set the total memory of an agent in its configuration file. This can be used in case the automatic detection doesn't work or in specific environments when memory overcommit (using swap for example) can be desirable.