Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force single arch support via env var #9535

Merged
merged 3 commits into from
Jul 9, 2020
Merged

Force single arch support via env var #9535

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f296e5
Force AMD64 only support via env var
Jul 9, 2020
9b77f37
Fix typo in function name
Jul 9, 2020
3c84d83
Address review comments
Jul 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions upup/pkg/fi/cloudup/apply_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ func (c *ApplyClusterCmd) addFileAssets(assetBuilder *assets.AssetBuilder) error
c.Assets = make(map[architectures.Architecture][]*MirroredAsset)
c.NodeUpSource = make(map[architectures.Architecture]string)
c.NodeUpHash = make(map[architectures.Architecture]string)
for _, arch := range architectures.GetSupprted() {
for _, arch := range architectures.GetSupported() {
c.Assets[arch] = []*MirroredAsset{}
c.NodeUpSource[arch] = ""
c.NodeUpHash[arch] = ""
Expand Down Expand Up @@ -1177,7 +1177,7 @@ func (c *ApplyClusterCmd) newNodeUpConfigBuilder(assetBuilder *assets.AssetBuild
components = append(components, "kube-apiserver", "kube-controller-manager", "kube-scheduler")
}

for _, arch := range architectures.GetSupprted() {
for _, arch := range architectures.GetSupported() {
for _, component := range components {
baseURL, err := url.Parse(cluster.Spec.KubernetesVersion)
if err != nil {
Expand All @@ -1203,7 +1203,7 @@ func (c *ApplyClusterCmd) newNodeUpConfigBuilder(assetBuilder *assets.AssetBuild
// `docker load` our images when using a KOPS_BASE_URL, so we
// don't need to push/pull from a registry
if os.Getenv("KOPS_BASE_URL") != "" && isMaster {
for _, arch := range architectures.GetSupprted() {
for _, arch := range architectures.GetSupported() {
// TODO: Build multi-arch Kops images
if arch != architectures.ArchitectureAmd64 {
continue
Expand Down Expand Up @@ -1287,7 +1287,7 @@ func (n *nodeUpConfigBuilder) BuildConfig(ig *kops.InstanceGroup, apiserverAddit
config.Tags = append(config.Tags, n.nodeUpTags...)

config.Assets = make(map[architectures.Architecture][]string)
for _, arch := range architectures.GetSupprted() {
for _, arch := range architectures.GetSupported() {
config.Assets[arch] = []string{}
for _, a := range n.Assets[arch] {
config.Assets[arch] = append(config.Assets[arch], a.CompactString())
Expand Down
9 changes: 8 additions & 1 deletion util/pkg/architectures/architectures.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package architectures

import (
"fmt"
"os"
"runtime"

"k8s.io/klog"
Expand All @@ -41,7 +42,13 @@ func FindArchitecture() (Architecture, error) {
}
}

func GetSupprted() []Architecture {
func GetSupported() []Architecture {
// Force support for AMD64 only if env var is set
arch := os.Getenv("KOPS_ARCH_AMD64")
if arch != "" {
return []Architecture{ArchitectureAmd64}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Appreciate the fix!

Any idea whether we need to update the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Np. This is for internal use at the moment, so I don't see much value in adding docs for it.

Copy link
Member

@rifelpet rifelpet Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@hakman hakman Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return []Architecture{
ArchitectureAmd64,
ArchitectureArm64,
Expand Down