-
Notifications
You must be signed in to change notification settings - Fork 122
Fixed cgroups issue and added "user" in machine spec #277
base: master
Are you sure you want to change the base?
Conversation
@@ -273,10 +273,10 @@ func (c *Cluster) CreateMachine(machine *Machine, i int) error { | |||
} | |||
|
|||
// Initial provisioning. | |||
if err := containerRunShell(name, initScript); err != nil { | |||
if err := containerRunShell(name, f(initScript, machine.User())); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't distinguish between the username and the user's home folder, which are different things. Same on line 279.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the original code, it's hardcoded to be /root/.ssh/
for the authorized_keys
.
Once the user is different, say ubuntu
, this path won't be valid any more to host the authorized_keys
, that's why we need to make it dynamic since the user could be configurable, by combining the changes made here within the same PR: https://github.com/weaveworks/footloose/blob/master/ssh.go#L44-L50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. I wanted to outline that, to my understanding, this will probably fail in all cases where the user is not "root". Almost all other users won't have their home directories directly below the root, but most likely below /home
or /var
, depending if those users are humans or machine accounts. The mapping between user and user directory is maintained inside /etc/passwd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, for the path issue, you're right that I didn't fix that properly and now I've made another push with it fixed.
Thanks for pointing that out.
How to test?
We can create such a user, say ubuntu
, and make sure it works properly through the footloose ssh ubuntu@xxx
or footloose ssh xxx
if the config file has already configured.
Dockerfile:
<OMITTED THE REST>
RUN >/etc/machine-id
RUN >/var/lib/dbus/machine-id
# Test with a dedicated user say "ubuntu" for the container
RUN useradd -ms /bin/bash ubuntu
<OMITTED THE REST>
_test_user.yaml:
cluster:
name: test
privateKey: cluster-key
machines:
- count: 1
spec:
image: brightzheng100/footloose-ubuntu22-ubuntu:arm64
name: vm-%d
user: ubuntu # <--- this does the trick
networks:
- footloose-cluster
portMappings:
- containerPort: 22
privileged: true
volumes:
- type: volume
destination: /var
And the experience must be the same:
footloose create -c _test_user.yaml
footloose show -c _test_user.yaml
# by this
footloose ssh vm-0 -c _test_user.yaml
# or this
footloose ssh ubuntu@vm-0 -c _test_user.yaml
Two changes were made in this PR:
cgroups
v2 enabled Docker, say inv20.10+
, and maybecontainerd
;Actually, there are some stories behind this PR. I recently changed my Macbook Pro from Intel to M1 chip and replaced Docker Desktop with Rancher Desktop. When I was trying to run
footloose
again, it failed infootloose ssh
command so I dug into the code and found 2 issues:1. The
systemd
was broken so thesshd
was not up and running.This worried me as there might be a couple of possibilities:
systemd
has issues running on Mac with M1 chip.So I reached out with this issue reported: #274
But it turns out that it's because of
cgroups
compatibility related issue: My Docker is onv20.10.16
andcgroups
should be on v2 already.Somebody had mentioned it here: systemd/systemd#19245
The fix, or workaround, is very simple: remove the read-only
/sys/fs/cgroup
mount in the startup command which is hardcoded incluster.go
and it then works.For those who want to continue the backward compatibility, they may need to define it explicitly in the
machines[].spec.volumes
.We may need to document it if there is a need.
2. The
footloose ssh
needs to specify the user explicitly.The current code's logic is to get the "current user" of OS where
footloose
commands are run, which may not make sense in most of the cases.For example, in my Mac the default user is my name while the container might be using
root
.So specifying
footloose ssh root@node
might not be the best UX.I raised it here: #276
What this PR provides is to extend a new (but optional) element, namely
user
, to specify the machine's user, if there is a need, and it defaults to the commonly used userroot
if nothing is set.So it has backward compatibility while offering more flexibility.