How to deal with the Default Setting variables for cli configure

[lifupan]

Description of problem

When I try to refactor the cli codes, especially the codes for the "config", and I want to
put those codes into a separated package such as "clilib", but I found those defaults variables
are dynamically produced by Makefile, my questions is how to deal with those defaults variables,
still make them dynamically produced by Makefile or just defined them in a separated file in the package?

If we still want to make them dynamically produced into the "clilib" package, then this package cannot
not be vendored into other projects; In addition, for those dynamically produced variables, some will be referenced by "clilib" and some will be referenced by cli, thus we should produce two files, one is located in "clilib", and the other one is located in cli, thus would it be too complex?

Expected result

(replace this text with an explanation of what you thought would happen)

Actual result

(replace this text with details of what actually happened)

---

(replace this text with the output of the kata-collect-data.sh script, after
you have reviewed its content to ensure it does not contain any private
information).

[jodh-intel]

Hi @lifupan - thanks for raising this!

History

The reason the Makefile generates config-generated.go is that golang's support for setting values at build time is arguably poor. Yes, you can use something like:

$ go build -ldflags "-X main.Var1 = $(VALUE1)"

But that is rather horrid:

• The variables must be strings.
• The variables must not contain spaces (fwics atleast).

If the previous restrictions weren't bad enough, you can shoot yourself in the foot rather easily:

$ cat >foo.go<<EOT
package main

import "fmt"

var var1 string
var var2 string
var var3 string

func main() {
        fmt.Printf("var1: %q\n", var1)
        fmt.Printf("var2: %q\n", var2)
        fmt.Printf("var3: %q\n", var3)
}
EOT
$ go build -ldflags "-X main.var1=foo" -o foo foo.go && ./foo
var1: "foo"
var2: ""
var3: ""
$ go build -ldflags "-X main.var1=foo" -ldflags "-X main.var2=bar" -o foo foo.go && ./foo
var1: ""
var2: "bar"
var3: ""

Oops! That's bad - we hit this issue back in Clear Containers and silently broke the proxy. the problem is that go build only honours the last -ldflags arg so the correct solution for the last go build above is:

$ go build -ldflags "-X main.var1=foo -X main.var2=bar" -o foo foo.go && ./foo         
var1: "foo"
var2: "bar"
var3: ""

The generated config

That explains why we gave up on go build method and just made the build create config-generated.go. Let's look at that file:

// WARNING: This file is auto-generated - DO NOT EDIT!
//
// Note that some variables are "var" to allow them to be modified
// by the tests.
package main

import (
	"fmt"
)

// name is the name of the runtime
const name = "kata-runtime"

// name of the project
const project = "Kata Containers"

// prefix used to denote non-standard CLI commands and options.
const projectPrefix = "kata"

// systemdUnitName is the systemd(1) target used to launch the agent.
const systemdUnitName = "kata-containers.target"

// original URL for this project
const projectURL = "https://github.com/kata-containers"

// commit is the git commit the runtime is compiled from.
var commit = "304ec7e23115cd094e3dec1cd72f5d6f35a963f3"

// version is the runtime version.
var version = "1.3.0-rc1"

// project-specific command names
var envCmd = fmt.Sprintf("%s-env", projectPrefix)
var checkCmd = fmt.Sprintf("%s-check", projectPrefix)

// project-specific option names
var configFilePathOption = fmt.Sprintf("%s-config", projectPrefix)
var showConfigPathsOption = fmt.Sprintf("%s-show-default-config-paths", projectPrefix)

var defaultHypervisorPath = "/usr/bin/qemu-lite-system-x86_64"
var defaultImagePath = "/usr/share/kata-containers/kata-containers.img"
var defaultKernelPath = "/usr/share/kata-containers/vmlinuz.container"
var defaultInitrdPath = "/usr/share/kata-containers/kata-containers-initrd.img"
var defaultFirmwarePath = ""
var defaultMachineAccelerators = ""
var defaultShimPath = "/usr/libexec/kata-containers/kata-shim"

const defaultKernelParams = ""
const defaultMachineType = "pc"
const defaultRootDirectory = "/var/run/kata-containers"

const defaultVCPUCount uint32 = 1
const defaultMaxVCPUCount uint32 = 0
const defaultMemSize uint32 = 2048 // MiB
const defaultMemSlots uint32 = 10
const defaultBridgesCount uint32 = 1
const defaultInterNetworkingModel = "macvtap"
const defaultDisableBlockDeviceUse bool = false
const defaultBlockDeviceDriver = "virtio-scsi"
const defaultEnableIOThreads bool = false
const defaultEnableMemPrealloc bool = false
const defaultEnableHugePages bool = false
const defaultEnableSwap bool = false
const defaultEnableDebug bool = false
const defaultDisableNestingChecks bool = false
const defaultMsize9p uint32 = 8192
const defaultHotplugVFIOOnRootBus bool = false
const defaultEntropySource = "/dev/urandom"

// Default config file used by stateless systems.
var defaultRuntimeConfiguration = "/usr/share/defaults/kata-containers/configuration.toml"

// Alternate config file that takes precedence over
// defaultRuntimeConfiguration.
var defaultSysConfRuntimeConfiguration = "/etc/kata-containers/configuration.toml"

var defaultProxyPath = "/usr/libexec/kata-containers/kata-proxy"
var defaultNetmonPath = "/usr/libexec/kata-containers/kata-netmon"

Analysis of generated config

All of those variables that define paths (excluding the config file ones) could be removed I think. That would have the effect of making the runtime fail if it was not valid (did not contain paths to shim, proxy, etc). But we guarantee it is valid when first installed as the build generates configuration.toml so there is no need to provide default values for the runtime.

Also, most of the other default* variables can be dropped as they also appear in configuration.toml. That leaves the following:

• Variables that must still be generated by the build:

  version
  commit
  

• Variables that probably should still be generated by the build:

  systemdUnitName
  defaultRuntimeConfiguration
  defaultSysConfRuntimeConfiguration
  defaultRootDirectory
  

• Variables that are unlikely to change.

  The following probably won't change again, so they could be hard-coded into config.go I think:

  const name = "kata-runtime"                                                                    
  const project = "Kata Containers"                                                              
  const projectURL = "https://github.com/kata-containers"                                        
  const projectPrefix = "kata" 
  

• More variables that can be hard-coded into config.go

  The following were handled this way because at the start of the project, the runtime could be built as either cc-runtime or kata-runtime. That is no longer required though :)

  var envCmd = fmt.Sprintf("%s-env", projectPrefix)                                              
  var checkCmd = fmt.Sprintf("%s-check", projectPrefix)                                          
  var configFilePathOption = fmt.Sprintf("%s-config", projectPrefix)                             
  var showConfigPathsOption = fmt.Sprintf("%s-show-default-config-paths", projectPrefix) 
  

Summary

If we are happy that the runtime will rely entirely on the configuration file (I don't see why it shouldn't), the only variables that need to be generated at built time are:

version
commit
systemdUnitName
defaultRuntimeConfiguration
defaultSysConfRuntimeConfiguration
defaultRootDirectory

That's manageable I think. I'd still prefer we have a config-generated.go if possible for the reasons already mentioned, but if that still won't work, the list of variables is probably small enough to handle with the -ldflags technique if we are careful.

/cc @grahamwhaley, @bergwolf, @egernst, @sboeuf.

[lifupan]

Hi @jodh-intel, IMO, only the "version" and "commit" can be dynamically produced by Makefile since they will be changed often and those two variables will only be referenced by cli main package, for the other variables I don't think they will be changed often and even if some of them need to be changed, I don't think there is a difference between changing in Makefile and in go source file.

[jodh-intel]

I guess we could hard-code systemdUnitName, but the following I don't think we can:

• defaultRuntimeConfiguration: uses PREFIX which needs to be set for building packages:

  • https://github.com/kata-containers/packaging/blob/master/obs-packaging/runtime/debian.rules-template#L36..L40
  • https://github.com/kata-containers/packaging/blob/master/obs-packaging/runtime/kata-runtime.spec-template#L90..L95

  Admittedly, we set it to /usr, but... ;)

• defaultSysConfRuntimeConfiguration: hard-coded in the Makefile to use a /etc prefix, but can be overridden at build time (we don't do this, but it could still be useful for others).

• defaultRootDirectory: hard-coded in the Makefile to use a /var prefix, but can be overridden at build time (we don't do this, but it could still be useful for others).

Random thought: we could provide a config-settings.go [*] which contains all these variables in their hard-coded form along with a build rule that will only recreate that file iff the build is run with something like:

# recreate "config-settings.go"
$ make FORCE_CUSTOM_CONFIG=1 PREFIX=/weird ...

@jcvenegas will probably have more insight here.

/cc @chavafg, @egernst.

---

[*] - config-settings.go would be the same as config-generated.go but with a better name given that for most users, they won't need to touch it and it will already exist :)

[lifupan]

Hi @jodh-intel, based on your input, I agree to keep defaultSysConfRuntimeConfiguration and defaultRootDirectory in Makefile, since defaultSysConfRuntimeConfiguration will be used by configure file parsing codes, which I'd like to put into a separated package, thus I think we can extract the configure file path finding codes from config.go and put it into main package, and make the file path
as an input to configure parsing function, thus we could separate all of those dynamically created variables from the library package.

[jodh-intel]

wdyt @bergwolf, @sboeuf, @grahamwhaley?

[bergwolf]

I agree that we should put the default config options in a .go file instead of regenerating all of them dynamically as of now. For those need dynamically generation, we can still keep them in Makefile but there should be only a small amount of them (config file path, commit version etc.).