Add provider interface versioning #278
Conversation
| if e.cfg == nil { | ||
| return false | ||
| switch cfg.External.InterfaceVersion { | ||
| case "v0.1.0": |
There was a problem hiding this comment.
We should probably define these as a constant or variable somewhere. We can also reference that constant/variable in the config package where we validate the external provider config, when we make sure that InterfaceVersion is valid.
There was a problem hiding this comment.
Also, this can probably be:
case "v0.1.0", "":Which will mean an explicit 0.1.0 or if not set, default to 0.1.0
There was a problem hiding this comment.
Defined "v0.1.0" and "v0.1.1" as constants in runner/common/params.go (where are also the ActionInstanceParams)
runner/providers/v0.1.1/external.go
Outdated
| fmt.Sprintf("GARM_POOL_ID=%s", bootstrapParams.PoolID), | ||
| fmt.Sprintf("GARM_PROVIDER_CONFIG_FILE=%s", e.cfg.External.ConfigFile), | ||
| fmt.Sprintf("GARM_POOL_IMAGE=%s", createInstanceParams.CreateInstanceV011.PoolInfo.Image), | ||
| fmt.Sprintf("GARM_POOL_EXTRASPECS=%s", string(extraspecsValue)), |
There was a problem hiding this comment.
a json may have all sorts of characters that may break shells. Have this be a base64 encoded string. That will ensure no weirdness will happen.
runner/providers/v0.1.1/external.go
Outdated
| fmt.Sprintf("GARM_CONTROLLER_ID=%s", e.controllerID), | ||
| fmt.Sprintf("GARM_POOL_ID=%s", bootstrapParams.PoolID), | ||
| fmt.Sprintf("GARM_PROVIDER_CONFIG_FILE=%s", e.cfg.External.ConfigFile), | ||
| fmt.Sprintf("GARM_POOL_IMAGE=%s", createInstanceParams.CreateInstanceV011.PoolInfo.Image), |
There was a problem hiding this comment.
I think this is already set in the bootstrap we send to stdin.
There was a problem hiding this comment.
Indeed, removed it for CreateInstance. For the moment, changed it to CreateInstance(ctx context.Context, bootstrapParams commonParams.BootstrapInstance, _ common.CreateInstanceParams) since we don't use (for now) anything from there
runner/providers/v0.1.1/util.go
Outdated
| // lifecycle state that it can influence. The sole purpose of a provider is to | ||
| // manage the lifecycle of an instance. Statuses that indicate an instance should | ||
| // be created or removed, will be set by the controller. | ||
| func IsValidProviderStatus(status commonParams.InstanceStatus) bool { |
There was a problem hiding this comment.
This can probably live in a common package considering this validates what GARM expects from any provider (external or otherwise)
There was a problem hiding this comment.
Created a new util package in runner/providers/util for this common part
fabi200123
changed the title
| @@ -77,6 +77,10 @@ func GetEnvironment() (Environment, error) { | |||
| if err := json.Unmarshal(data.Bytes(), &bootstrapParams); err != nil { | |||
| return Environment{}, fmt.Errorf("failed to decode instance params: %w", err) | |||
| } | |||
| if bootstrapParams.ExtraSpecs == nil { | |||
There was a problem hiding this comment.
I am guessing these files come from a replace statement in the go.mod file.
runner/providers/v0.1.0/external.go
Outdated
| environmentVariables []string | ||
| } | ||
|
|
||
| func (e *external) validateResult(inst commonParams.ProviderInstance) error { |
There was a problem hiding this comment.
This should be common. This validates that the provider returns proper values and this is valid for any version.
| asEnv := []string{ | ||
| fmt.Sprintf("GARM_COMMAND=%s", commonExecution.CreateInstanceCommand), | ||
| fmt.Sprintf("GARM_CONTROLLER_ID=%s", e.controllerID), | ||
| fmt.Sprintf("GARM_POOL_ID=%s", bootstrapParams.PoolID), |
There was a problem hiding this comment.
to be consistent with the rest of the functions, I think we can have the GARM_POOL_EXTRASPECS variable set here as well. I know the CreateInstance command will look for extra specs in the bootstrap instance stdin body, but we should be consistent. The fewer differences in terms of env variables, the better.
runner/pool/pool.go
Outdated
| poolInstances, err = provider.ListInstances(r.ctx, pool.ID) | ||
| listInstancesParams := common.ListInstancesParams{ | ||
| ListInstancesV011: common.ListInstancesV011Params{ | ||
| ProviderBaseParams: common.ProviderBaseParams{ |
There was a problem hiding this comment.
At this point, the base params will always contain the same thing. Please create a function in the basePoolManager{} that returns the ProviderBaseParams{} struct. And you can just call it.
The function should be protected by r.mux.Lock() as the pool info can be changed and updated by the DB watcher.
There was a problem hiding this comment.
We might also want to switch on the interface version in the future and return the appropriate ListInstancesParams{}
runner/pool/pool.go
Outdated
| startParams := common.StartParams{ | ||
| StartV011: common.StartV011Params{ | ||
| ProviderBaseParams: common.ProviderBaseParams{ | ||
| PoolInfo: pool, |
There was a problem hiding this comment.
Perhaps r.getProviderBaseParams(pool). Here and everywhere else?
fabi200123
force-pushed
the
version-v3
branch
from
5e30530
to
dcff6f9
Compare
This PR adds provider interface versioning:
Interface Version,Pool InfoandController Info