hypriot / flash

Command line script to flash SD card images of any kind
MIT License
1.01k stars 175 forks source link

Set date/time in cloud-init runcmd module as part on initial boot #152

Closed eloots closed 4 years ago

eloots commented 5 years ago

As the Raspberry Pi's don't have a Real Time Clock (RTC), it takes a while before ntpd sets the time to the current (real) time. This creates a problem as apt-get update relies on the date/time being not too far off the mark. This commit will add a command as part of the runcmd section of cloud-init that sets the date/time to the date/time at which the ssd was flashed. Even though this will not set the date/time accurately, the skew is small enough to make apt-get update work.

Legion2 commented 5 years ago

This is the better solution to the problem described in #151. This PR will close #151

eloots commented 5 years ago

No clue about why CI fails with this message:

Step 4/10 : RUN npm install &&     mv node_modules ..
 ---> Running in 6ac25338c9c8
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN flash-test@0.0.1 No repository field.

A fluke that can be ignored?

Legion2 commented 5 years ago

this is only a warning and can be ignored here

Legion2 commented 5 years ago

not ok 5 cloud-init: flash --userdata replaces user-data this is the real error here

eloots commented 5 years ago

The only remark I could add is that making this optional (by, say, adding a new option to flash like -t) may be a good idea. Although, tbh, adding the setting of date/time doesn't hurt...

eloots commented 5 years ago

@Legion2

not ok 5 cloud-init: flash --userdata replaces user-data this is the real error here

No clue how/what to fix here...

Legion2 commented 5 years ago

If an option is added, it should also optionally be possible to pass a date on the command line. So if i flash the image now, but want to boot it the first time next week.

@eloots There are some tests that must be updated, also a new test should be added for this feature

eloots commented 5 years ago

If an option is added, it should also optionally be possible to pass a date on the command line. So if i flash the image now, but want to boot it the first time next week.

So - I would suggest not to add an option then. WDYT?

Legion2 commented 5 years ago

OK, but we should change from runcmd: to bootcmd: with cloud-init-per once. So the date is set before packages are installed with the packages: config.

eloots commented 5 years ago

@Legion2

not ok 5 cloud-init: flash --userdata replaces user-data this is the real error here

I failed to find this error in the output of CI. In any case, I have no real experience with the test framework used here. I presume that it detects that the user-data file that was flashed has a different content than the file passed in via the --userdata option?

eloots commented 5 years ago

Will check this later - need to run now.

Legion2 commented 5 years ago

@eloots i looked at the ci output again and now discovered the source of the problem. ./flash: line 490: ed: command not found ed is not installed in the test environment and also not installed on all user environments, so you should use another command to update the user-data file.

eloots commented 5 years ago

@Legion2

OK, but we should change from runcmd: to bootcmd: with cloud-init-per once. So the date is set before packages are installed with the packages: config.

Great idea. I've implemented this in the mean time, and it works (see 4fd5334). I had some trouble figuring out the meaning of the 3rd in the cloud-init-per, once list. The cloud-init isn't clear about that. In this specific case:

  - [ cloud-init-per, once, sdate, date, -s, "$current_datetime" ]

sdate seems to be a "label". In any case, it seems to work pretty well now...

The doc section in cloud-init for this is not very helpful...

Finally, as you noted in your comment, installing packages in the packages: section now work too!

eloots commented 5 years ago

@Legion2

i looked at the ci output again and now discovered the source of the problem. ./flash: line 490: ed: command not found ed is not installed in the test environment and also not installed on all user environments, so you should use another command to update the user-data file.

I'm pretty sure that MacOS comes with ed by default and that anyone using flash on some linux distro has ed installed by default as well. I think this is specific to this CI environment and I would just add it... (My 2 cents).

I agree that, in the rare case that ed indeed isn't present, it should not blow-up a flash run. I propose to add tests for that in flash: if it is detected that ed isn't present we either:

or

WDYT?

By the way, if you have an alternative for ed that is omnipresent on MacOS and Linux, let me know, but ed is as basic as an editor gets and I've never seen it missing on *nix-es.

Legion2 commented 5 years ago

@eloots Hypriot itself don't have ed, so you have to install ed when you want to use flash on the raspberry pi to create sd images for other raspberry pis.

Legion2 commented 5 years ago

Alternative for ed is sed and it's already used in flash.

eloots commented 5 years ago

As I suggested, flash could warn you that ed is missing. Installing it on a RPi is as simple as doing an

sudo apt-get install ed

Just curious why you would generate an image on a RPi though...

Op 10 aug. 2019 om 17:55 heeft Leon Kiefer notifications@github.com het volgende geschreven:

@eloots Hypriot itself don't have ed, so you have to install ed when you want to use flash on the raspberry pi to create sd images for other raspberry pis.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

eloots commented 5 years ago

Yes, it would be possible to reach the same result, but you’d have to create an temporary file as you can’t edit a file in-place. After that you’d move the temp file over the original one.

Op 10 aug. 2019 om 18:05 heeft Leon Kiefer notifications@github.com het volgende geschreven:

alternative for ed is sed

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Legion2 commented 5 years ago

you can use the -i option of sed to edit files in place

eloots commented 5 years ago

Aha, didn’t know that. TIL!

Op 10 aug. 2019 om 20:03 heeft Leon Kiefer notifications@github.com het volgende geschreven:

you can use the -i option of sed to edit files in place

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

eloots commented 5 years ago

CircleCI reports:

./flash: line 490: /usr/bin/sed: No such file or directory
Legion2 commented 5 years ago

@eloots use sed instead of absolute path, because it should be in the PATH. On my distro:

$ type sed
sed is /bin/sed
muelli commented 5 years ago

This whole change seems very brittle. Can you check whether the modification of the YAML file breaks YAML syntax and conditionally revert the change? You could use the existing verify yaml function. Otherwise, the user may be left wondering why their cloud init does not work. If the script assumes ruby anyway, then I wonder whether it's more robust to use that yaml library for inserting the list item rather than sed.

eloots commented 5 years ago

@Legion2 I had noticed the presence of the sed_i alias(es) but hadn't looked at it in detail. Indeed and I will change that: the sed command is in /usr/bin, whereas on linux, it's in /bin. Also, and that's the reason of aliasing it differently between MacOS and Linux: the -i option on MacOS requires one to explicitly pass in an extension (which is ~and~ an empty string in sed_i).

eloots commented 5 years ago

@muelli

This whole change seems very brittle. Can you check whether the modification of the YAML file breaks YAML syntax and conditionally revert the change? You could use the existing verify yaml function. Otherwise, the user may be left wondering why their cloud init does not work. If the script assumes ruby anyway, then I wonder whether it's more robust to use that yaml library for inserting the list item rather than sed.

Using sed to manipulate file content on the flashed image is done in quite a few places throughout the flash script (in fact, as mentioned above, it has a special alias defined for convenience (sed_i). Also, the produced YAML is checked when executing it on MacOS.

eloots commented 5 years ago

@StefanScherer @DieterReuter @Legion2 @muelli CI passing... On top of this, I actually verified the proper functioning of flash both on MacOS & Linux in the following scenarios:

In all four combinations, the generated ssd booted successfully with the intended effect (fixing the issue that was to be solved).

eloots commented 5 years ago

@Legion2 Just wondering:

Legion2 commented 5 years ago

Squash the commits can be done while merging. I have no write permission on this repo, so we have to wait for someone who can merge this.

Legion2 commented 5 years ago

@eloots maybe we should add some documentation about it @StefanScherer can you have a look at this one

troyfontaine commented 5 years ago

I wonder if this will also fix my issue with ssh-import-id not working-it is currently failing as well.

eloots commented 5 years ago

@troyfontaine

I wonder if this will also fix my issue with ssh-import-id not working-it is currently failing as well.

Have you tried it yet?

troyfontaine commented 5 years ago

@troyfontaine

I wonder if this will also fix my issue with ssh-import-id not working-it is currently failing as well.

Have you tried it yet?

No, I ran into an error because the Kernel had been updated and have been busy since then. I'll try and make time to verify-but it is very likely the cause because of the sensitivity to timestamps with TLS connections.

It looks like I can't use at least the last two versions of flash with Ubuntu Budgie at all for some reason. The flashed images are corrupt in some way that breaks cloud-init. However, using this PR's version from a MacBook Pro on Mojave it flashes without issue and boots on the Pi4B without complaint and the system updates perfectly.

eloots commented 5 years ago

@StefanScherer

Seems like a good approach to the lack of hardware features of the RPi.

We also need a test for this to check that Mac and Linux works in CI. Please have a look at the test/ folder.

I'll have a look at the tests folder (just got back from vacation, so didn't your feedback until now).

manicmonkey commented 5 years ago

I've also had trouble due to the delayed date update. I tried using the solution in this PR, but it seems there is a problem with date -u producing the correct output. Not sure if this is caused by any local configuration, but I'm sure it won't just affect me:

james@james-xps13:~$ date -u
Sat 28 Sep 11:29:04 UTC 2019
HypriotOS/armv7: james@boat-pi in ~
$ sudo date -s "Sat 28 Sep 11:29:04 UTC 2019"
date: invalid date ‘Sat 28 Sep 11:29:04 UTC 2019’

Versions:

james@james-xps13:~$ date --version
date (GNU coreutils) 8.28
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by David MacKenzie.
HypriotOS/armv7: james@boat-pi in ~
$ date --version
date (GNU coreutils) 8.30
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by David MacKenzie.

Perhaps better to use date -Iseconds -u?

james@james-xps13:~$ date -Iseconds -u
2019-09-28T11:30:51+00:00
HypriotOS/armv7: james@boat-pi in ~
$ sudo date -s "2019-09-28T11:30:51+00:00"
Sat 28 Sep 2019 09:30:51 PM AEST
manicmonkey commented 5 years ago

I followed the coreutils source to nl_langinfo and it's probably because my pi has a US locale and my laptop has a UK locale:

james@james-xps13:~$ locale
LANG=en_GB.UTF-8
LANGUAGE=en_GB:en
...
LC_TIME="en_GB.UTF-8"
james@james-xps13:~$ locale -k LC_TIME
...
date_fmt="%a %e %b %H:%M:%S %Z %Y"
HypriotOS/armv7: james@boat-pi in ~
$ locale
LANG=en_US.UTF-8
LANGUAGE=
...
LC_TIME="en_US.UTF-8"
HypriotOS/armv7: james@boat-pi in ~
$ locale -k LC_TIME
...
date_fmt="%a %d %b %Y %r %Z"
Legion2 commented 5 years ago

we should use LANG=C date ...

eloots commented 5 years ago

@manicmonkey

I've also had trouble due to the delayed date update. I tried using the solution in this PR, but it seems there is a problem with date -u producing the correct output. Not sure if this is caused by any local configuration, but I'm sure it won't just affect me:

Thanks for digging into this. Will update PR according to @Legion2 ‘a suggestion.

StefanScherer commented 4 years ago

Thank you all for that idea. We'll prefer PR #162 to directly write the current date for fake-hwclock if the SD card image has it symlinked to the /boot partition. We just merged hypriot/image-builder-rpi#334 and will draft a new release soon.

StefanScherer commented 4 years ago

The new combo flash v2.5.0 and HypriotOS v1.12.0-rc2 solve this issue.

eloots commented 4 years ago

@StefanScherer Great that this is fixed. So, from a user perspective, all that is needed is to add a -F fake-hwclock.data option when using flash (and create an empty fake-hwclock.data file in the folder from which you're flashing), correct?

StefanScherer commented 4 years ago

@eloots You don‘t need a new option, the new flash v2.5.0 automatically looks if that file exists in the /boot partition after flashing and updates the timestamp. You do not need to create any file manually. The pre-release 1.12.0-rc2 has this file prepared. Older versions of HypriotOS don‘t support setting the timestamp this way, so creating the file wouldn‘t help.

eloots commented 4 years ago

@StefanScherer I wanted to give pre-release 1.12.0-rc2 a try, but got a 404 when passing it as the image using flash 2.5.0.

StefanScherer commented 4 years ago

@eloots oops, I forgot to prepend a v in the version tag, so it's 1.12.0-rc2 instead of v1.12.0-rc2 🤦‍♂

flash https://github.com/hypriot/image-builder-rpi/releases/download/1.12.0-rc2/hypriotos-rpi-1.12.0-rc2.img.zip
eloots commented 4 years ago

@StefanScherer Ok - I got it now; the file is created as part of the 1.12.0 image. That confused me. In the mean time, I gave this a try and it works like a charm!

Duvel commented 4 years ago

I just flashed this version (and noted the missing v) and cloud-init already did have the time from the file it seems. I booted without network and it had the time of the flash.