builder: add more options for CPU topology #94
Conversation
By configuration do you mean Packer template configuration? If so, I take this to mean that it will change behavior for a user, and that we will need to bump the minor version for this change. Is that correct? |
|
With the new code it will indeed potentially change the behaviour for clients, since what we specified with
So depending on the version of qemu, this may not change the topology reported by the system, and more fundamentally, the number of vCPUs exposed to the guest OS will remain the same. |
|
I guess it could mean a minor bump though because of the behaviour change, you are right on that. |
| return smpStr | ||
| } | ||
|
|
||
| func (c QemuSMPConfig) getMaxCPUs() int { |
There was a problem hiding this comment.
Even though this is a unexported method I recommend adding comment to explain how getMaxCPUs works. More specifically that it calculates the max cpu based off the QemuSMPConfig values. At first I thought that it was making a call to the underlying system to determine max allowed cpu.
builder/qemu/config.go
Outdated
| totalVCPUs := c.SocketCount | ||
|
|
||
| if c.CoreCount > 0 { | ||
| if totalVCPUs == 0 { | ||
| totalVCPUs = c.CoreCount | ||
| } else { | ||
| totalVCPUs *= c.CoreCount | ||
| } | ||
| } | ||
|
|
||
| if c.ThreadCount > 0 { | ||
| if totalVCPUs == 0 { | ||
| totalVCPUs = c.ThreadCount | ||
| } else { | ||
| totalVCPUs *= c.ThreadCount | ||
| } | ||
| } | ||
|
|
||
| return totalVCPUs |
There was a problem hiding this comment.
nit: The nested if statements is a little confusing. Does something like the following make it clearer?
| totalVCPUs := c.SocketCount | |
| if c.CoreCount > 0 { | |
| if totalVCPUs == 0 { | |
| totalVCPUs = c.CoreCount | |
| } else { | |
| totalVCPUs *= c.CoreCount | |
| } | |
| } | |
| if c.ThreadCount > 0 { | |
| if totalVCPUs == 0 { | |
| totalVCPUs = c.ThreadCount | |
| } else { | |
| totalVCPUs *= c.ThreadCount | |
| } | |
| } | |
| return totalVCPUs | |
| totalVCPUs := c.SocketCount | |
| if totalVCPUs > 0 && c.CoreCount > 0 { | |
| totalVCPUs *= c.CoreCount | |
| } | |
| // If number of sockets were not provided take the number of cores | |
| if totalVCPUs == 0 { | |
| totalVCPUs = c.CoreCount | |
| } | |
| // CPUCount, SocketCount, and ThreadCount were set return the final product of all. | |
| if totalVCPUs > 0 && c.ThreadCount > 0 { | |
| return totalVCPUs *= c.ThreadCount | |
| } | |
| // If nothing else was set take the value set for thread count | |
| if totalVCPUs == 0 { | |
| return c.ThreadCount | |
| } | |
| return totalVCPUs |
builder/qemu/config.go
Outdated
| } | ||
|
|
||
| if cpuCount > totalVCpus && totalVCpus != 0 { | ||
| log.Printf("CPU count greater than what topology allows, setting to %d", totalVCpus) |
There was a problem hiding this comment.
| log.Printf("CPU count greater than what topology allows, setting to %d", totalVCpus) | |
| log.Printf("CPU count is greater than what topology allows, setting to CPU count to the allowed topology %d", totalVCpus) |
builder/qemu/config.go
Outdated
| // If both any topology element, or this is set, the final value will be the | ||
| // largest of the two. |
There was a problem hiding this comment.
It is not clear to me me which topology elements are being referred to here. Could you reword to call out which elements you are referring to.
c2fa609 to
bd0b371
Compare
In order to mirror more closely the options offered by qemu when it comes to expressing the topology of the SMP architecture we are building an image upon, we add more options to the configuration supported by the plugin. In addition to cpus, we now support sockets, cores and threads. These may be used in conjunction with the cpus, or left to be computed by the plugin directly. By default, any configuration which only defined cpus will work potentially differently from before, since this is supported by qemu, but the behaviour changes depending on the version.
bd0b371 to
4f00072
Compare
In order to mirror more closely the options offered by qemu when it comes to expressing the topology of the SMP architecture we are building an image upon, we add more options to the configuration supported by the plugin.
In addition to cpus, we now support sockets, cores and threads. These may be used in conjunction with the cpus, or left to be computed by the plugin directly.
By default, any configuration which only defined cpus will work potentially differently from before, since this is supported by qemu, but the behaviour changes depending on the version.
Closes #55