Allow to set the 'InfraID' with an environment variable. #5101
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @mmartinv. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
needs-ok-to-test
Can you elaborate? It seems like this a dependency on something that is designed to be non-deterministic. |
pkg/asset/installconfig/clusterid.go
Outdated
| @@ -74,6 +75,15 @@ func generateInfraID(base string, maxLen int) string { | |||
| } | |||
| base = strings.TrimRight(base, "-") | |||
|
|
|||
| // Return the InfraID provided by the "INFRA_ID" environment variable if valid | |||
| if envInfraID, ok := os.LookupEnv("INFRA_ID"); ok && envInfraID != "" { | |||
| validInfraIDString := fmt.Sprintf("^%s-([b-df-hj-np-tv-z0-9]){%d}$", base, randomLen) | |||
There was a problem hiding this comment.
Maybe move this pattern to a constant and explain what are you looking for
There was a problem hiding this comment.
I tried to explain more in the comment, moving the pattern to a constant make the code less readable IMO.
Sure in the installer the InfraID is used to set the names of the VMs and for oVirt it is also used to set a cluster tag (in oVirt). Having a hook in the installer to set the InfraID prior to the installation will help us overcome does issues. |
mmartinv
force-pushed
the
allow-to-set-the-infra-id-with-an-env-variable
branch
from
07f4b28 to
62f4e42
Compare
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold Has this been discussed in an enhancement/arch call? I am concerned about the dependencies we are introducing here. These topics have been discussed with regard to the cluster ID eg. #761 & #783 but I am not certain how this relates to the infra id. |
Added it to my enhancement on assisted installer: https://github.com/openshift/assisted-service/pull/2272/files#diff-988c4e5d655472b2174837171f5d89978ca0f86ec8554329b386dccb9990ea83R134-R147
Those two seem unrelated to me (unless I missed something) anyway I don't think we are adding dependencies, we keep the situation as is, but add a hook to preset it (while making sure that the structure is identical to the current state) |
Thanks I left a comment in the enhancement. The dependency is that we are creating reliable behavior through deterministic naming conventions so consumers may start to depend on resources with certain names. Although this is an environment variable it is essentially creating an API. |
Thanks now I understand your concerns a bit better, but I think that as long as the env var(or API) follows the same deterministic naming conventions then it shouldn't be a problem. |
mmartinv
force-pushed
the
allow-to-set-the-infra-id-with-an-env-variable
branch
2 times, most recently
from
d7015da to
ec8b292
Compare
This change allows the user to set the "OPENSHIFT_INSTALL_INFRA_ID_OVERRIDE" environment variable to modify the "InfraID". The contents of the "OPENSHIFT_INSTALL_INFRA_ID_OVERRIDE" correspond to the random part of the InfraID so it must be formed by 5 random lowercase alphanumeric chars with no vowels, otherwise an error will be returned. An usage example would be: OPENSHIFT_INSTALL_INFRA_ID_OVERRIDE="xxxxx" openshift-install create manifests --dir=. This change is needed to implement external provider support in the assisted-installer project.
mmartinv
force-pushed
the
allow-to-set-the-infra-id-with-an-env-variable
branch
from
ec8b292 to
5ecddff
Compare
|
In general, the use of environment variables to drive configuration of the cluster is discouraged in the installer. The install-config.yaml is the place where configuration should live. We have allowed the use of environment variables in a few rare cases, where the alternatives have been onerous or where the environment variable is only for testing and not supported for use by the end user. I am not yet convinced that this is one of the cases where an exception is justified. If we want to provide the option for the user to be able to use a pre-determine Infra ID, then we should add that to the install-config.yaml. However, It seems reasonable enough to me to have a user of the installer that needs the infra ID get it from the infrastructure manifest rather than taking the approach of the user supplying the infra ID. |
|
@mmartinv: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close |
|
@barbacbd: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This change allows the user to set the
OPENSHIFT_INSTALL_INFRA_ID_OVERRIDEenvironmentvariable to modify the
InfraID.The contents of the
OPENSHIFT_INSTALL_INFRA_ID_OVERRIDEcorrespond to the random part ofthe
InfraIDso it must be formed by 5 random lowercase alphanumeric chars with no vowels,otherwise an error will be returned.
An example on how to use it:
This change is needed to implement external provider support in the assisted-installer project.