Force single arch support via env var #9535
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
/retest |
|
Feels like this should be a list of archs to support, but then I don't really think we'll support more archs than these two anytime soon. /lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
added
size/S
|
Thanks :) |
k8s-ci-robot
added
the
lgtm
| func GetSupported() []Architecture { | ||
| // Force support for AMD64 only if env var is set | ||
| arch := os.Getenv("KOPS_ARCH_AMD64") | ||
| if arch != "" { | ||
| return []Architecture{ArchitectureAmd64} | ||
| } |
There was a problem hiding this comment.
Nice! Appreciate the fix!
Any idea whether we need to update the documentation?
There was a problem hiding this comment.
Np. This is for internal use at the moment, so I don't see much value in adding docs for it.
There was a problem hiding this comment.
I wonder if we could setup this env var so its a bit more flexible in the future if we do ever support additional architectures? Perhaps move the architecture into the env var value:
KOPS_ARCH=amd64
KOPS_ARCH=amd64,arm64
or something more descriptive since it is specifically for downloading k8s components and kops images (I don't have a good suggestion)
There was a problem hiding this comment.
I think in the future we may want to make it a spec option or even a create cluster flag.
At the moment, there is very little value to allow various values.
This function really should be used for anything cpu arch related in case there will be something like that, and it touches cni plugins and maybe other types of assets.
If we ever want to, we could add also KOPS_ARCH_ARM64 if we only want ARM64 support, but I don't have a better idea either.
Or, as you said KOPS_ARCH=amd64 and KOPS_ARCH=arm64, but single option for now. I think I like this most.
There was a problem hiding this comment.
Another dump question. :)
I assume it was too late to rely on FindArchitecture() to provide the correct arch at runtime probably because we want to download binary/images upfront to save time?
There was a problem hiding this comment.
You would have to know what arch is on each node. That is not possible or unreliable when the cluster is created.
We only retrieve the hashes, not the binaries. Each node decides on its own what arch it needs.
|
/cc @rifelpet |
k8s-ci-robot
removed
the
lgtm
hakman
changed the title
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman, rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Force AMD64 or ARM64 only support via env var to address the issues with Kubernetes PR binaries being AMD64 only.
Fixes: #9526