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.

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

Set ETCD_UNSUPPORTED_ARCH on ARM controller nodes #184

Merged
merged 17 commits into from
Aug 23, 2021

Conversation

erdii
Copy link
Contributor

@erdii erdii commented Aug 9, 2021

This PR explores a way to enable cluster creation on arm nodes.

Alternative to this implementation: Add systemd drop-in file via hosts[].files but this is hardcoding node architectures in the cluster configuration

# arm64/override.conf
[Service]
Environment=ETCD_UNSUPPORTED_ARCH=arm64
# arm/override.conf
[Service]
Environment=ETCD_UNSUPPORTED_ARCH=arm
apiVersion: k0sctl.k0sproject.io/v1beta1
kind: Cluster
metadata:
  name: k0s-cluster
spec:
  hosts:
  - ssh:
      address: 192.168.35.12
      user: root
    files:
    # on arm64: use arm64/override.conf
    # on arm: use arm/override.conf
    - src: arm/override.conf
      dstDir: /etc/systemd/system/k0scontroller.service.d
      perm: 0644

@erdii erdii marked this pull request as ready for review August 9, 2021 17:02
@erdii erdii mentioned this pull request Aug 9, 2021
func (p *InjectARMFixes) Prepare(config *config.Cluster) error {
p.Config = config

hosts := p.Config.Spec.Hosts.Filter(func(h *cluster.Host) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is only needed for controllers

Suggested change
hosts := p.Config.Spec.Hosts.Filter(func(h *cluster.Host) bool {
hosts := p.Config.Spec.Hosts.Filter(func(h *cluster.Host) bool {
if h.Role == "worker" {
return false
}

`, h.Metadata.Arch)

name, err := func() (string, error) {
d, err := ioutil.TempDir("", "k0scontroller.service.d.")
Copy link
Contributor

Choose a reason for hiding this comment

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

ioutil package is being phased out, os.MkdirTemp replaces ioutil.TempDir.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Maybe just host.Configurer.WriteFile(....) it instead of wrestling with tempfiles and host.Files.

@erdii
Copy link
Contributor Author

erdii commented Aug 10, 2021

Heyo @kke 👋
Good catches! I've (again! 😀) incorporated all of your suggestions. :)

Disclaimer: I don't have time to test h.Configurer.WriteFile right now.

@kke
Copy link
Contributor

kke commented Aug 11, 2021

Another problem, only works for systemd.

@kke
Copy link
Contributor

kke commented Aug 11, 2021

Ok I broke it. Fixing it for openrc and relatives seems to require some more changes to rig.


p.hosts = p.Config.Spec.Hosts.Filter(func(h *cluster.Host) bool {
arch := h.Metadata.Arch
return h.Role != "worker" && (arch == "arm" || arch == "arm64")
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: I think this needs aarch64 as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly armv7l and some others... maybe just strings.HasPrefix with arm and aarch, then figure out 32/64bit separately to choose if etcd_unsupported_arch should be set to arm or arm64.

https://etcd.io/docs/v3.5/op-guide/supported-platform/ here's the list of supported platforms and which ones need the env.

I think it needs to print a warning too.

@kke
Copy link
Contributor

kke commented Aug 12, 2021

I think it needs a ServiceEnvironment(s string, map[string]string) in rig.

For systemd it can create such override.conf thing as done here and for openrc it can create a /etc/conf.d/<service-name> file (syntax just simple FOO="bar") which openrc will eval before starting a service.

@kke
Copy link
Contributor

kke commented Aug 12, 2021

I think this also needs to be considered when doing a k0sctl reset.

@@ -0,0 +1,58 @@
package phase
Copy link
Contributor

Choose a reason for hiding this comment

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

I just do not understand what is wrong with this file now.

Vim gives me:
image

go vet gives me:

# github.com/k0sproject/k0sctl/cmd
cmd/apply.go:79:5: undefined: phase.PrepareArm
# github.com/k0sproject/k0sctl/cmd
vet: cmd/apply.go:79:11: PrepareArm not declared by package phase

any ideas, @erdii @jnummelin @jasmingacic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmmmmmmmmmm, is it the _arm in filename? maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is.

Files with os and architecture specific suffixes automatically follow those same constraints, e.g. name_linux.go will only build on linux, name_amd64.go will only build on amd64. This is the same as having a //+build amd64 line at the top of the file

@kke kke changed the title enable ETCD_UNSUPPORTED_ARCH on arm nodes Set ETCD_UNSUPPORTED_ARCH on arm nodes Aug 16, 2021
@kke kke changed the title Set ETCD_UNSUPPORTED_ARCH on arm nodes Set ETCD_UNSUPPORTED_ARCH on ARM controller nodes Aug 16, 2021
@kke kke added the enhancement New feature or request label Aug 16, 2021
@kke
Copy link
Contributor

kke commented Aug 16, 2021

Maybe it works now. @erdii ptal

@erdii
Copy link
Contributor Author

erdii commented Aug 17, 2021

Wow thanks! I'll take a look in the next couple of days. Please ping me if I forget to do so! 😅

Copy link
Contributor Author

@erdii erdii left a comment

Choose a reason for hiding this comment

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

Just tried this on my local setup with 3 odroid-hc4 and 3 raspberry pi 4 boards and it worked like a breeze! :)

Thank you very much @kke !

Edit: all my machines are on archlinuxarm so i can't speak for non-systemd distributions

func (p *PrepareArm) etcdUnsupportedArch(h *cluster.Host) error {
var arch string
switch h.Metadata.Arch {
case "aarch32", "arm32", "armv7l", "armhfp", "arm-32":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is that list from? Maybe it makes sense to link the source here in a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Several sources, I googled around, there seems to be a huge load of different ones, it's possible this is still incomplete. I think the most important ones here are aarch32 and armv7l which I think are commonly seen in raspberries.

}

log.Warnf("%s: enabling ETCD_UNSUPPORTED_ARCH=%s override - you may encounter problems with etcd", h, arch)
h.Environment["ETCD_UNSUPPORTED_ARCH"] = arch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What did h.Environment previously do? Will this also set /etc/environment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

It goes both ways, this way also all the HTTP_PROXY stuff etc that user might have configured will get into the env override file, there's a possibility it will make them end up in k0s process more consistently.

@erdii
Copy link
Contributor Author

erdii commented Aug 18, 2021

Just had these questions - other than that LGTM!

@erdii
Copy link
Contributor Author

erdii commented Aug 18, 2021

Maybe h.Environment needs a documentation update to clarify what exactly it does after this change? 🤔

@erdii
Copy link
Contributor Author

erdii commented Aug 19, 2021

Actually now that i think of it I'm afraid that the service override file could have stayed in place during my run of k0sctl reset - I'll have to test another time while ensuring that the file is actually gone.

@erdii
Copy link
Contributor Author

erdii commented Aug 19, 2021

I did just let it run in the background of a meeting and there are two issues:

  1. Due to the use of backtick-delimited strings the ServiceEnvironmentContent functions in rig return strings with verbatim \n in them. -> This one can be fixed by replacing the backticks with double-quotes which I verified on my local system.
  2. A systemctl daemon-reload is needed when systemd already knew about the k0scontroller service from earlier. I'm not quite sure where to add it. I'd tend to add this logic inside the initsystem package. 🤔

What do you think?

@erdii
Copy link
Contributor Author

erdii commented Aug 19, 2021

UpdateServiceEnvironment could call DaemonReload as the final step

@erdii
Copy link
Contributor Author

erdii commented Aug 19, 2021

Proposed fix in rig: k0sproject/rig#40

@kke
Copy link
Contributor

kke commented Aug 23, 2021

The rig fixes are available as v0.4.4-beta.2 currently, I'm in the middle of fixing the sudo stuff in #192 so it will bring in some other changes, should have the v0.4.4 out any time now.

@erdii
Copy link
Contributor Author

erdii commented Aug 23, 2021

Observation: when the controller init phase fails, a subsequent reset it not cleaning up service environment

@erdii
Copy link
Contributor Author

erdii commented Aug 23, 2021

Another observation: the empty folder /etc/systemd/system/k0scontroller.service.d stays in place after k0sctl reset

@kke
Copy link
Contributor

kke commented Aug 23, 2021

Clean-up improved

@kke
Copy link
Contributor

kke commented Aug 23, 2021

The empty dir can be fixed separately k0sproject/rig#42

@kke
Copy link
Contributor

kke commented Aug 23, 2021

Merging to roll out a beta

@kke kke merged commit d97cc1f into k0sproject:main Aug 23, 2021
@erdii erdii deleted the add-arm-support branch August 23, 2021 12:30
@erdii
Copy link
Contributor Author

erdii commented Aug 23, 2021

Thank you! I will test it asap :)

@erdii
Copy link
Contributor Author

erdii commented Aug 23, 2021

works 👍 thank you very much @kke

@dwarf-king-hreidmar
Copy link

Is there a binary for linux arm or should i build it myself? Attempting to try on RPis

@kke
Copy link
Contributor

kke commented Sep 13, 2021

Is there a binary for linux arm or should i build it myself? Attempting to try on RPis

I thought there was, but looks like there isn't. I'll add it to the build scripts.

@kke kke mentioned this pull request Sep 13, 2021
@kke
Copy link
Contributor

kke commented Sep 13, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants