Adding support to raspbian 32/64 deploy #563

rodrigosiqueira · 2022-02-20T18:09:15Z

This PR introduces support for RPI deploy. I tested it with RPI with Rasbian 32 bits installed on it.

codecov-commenter · 2022-02-20T18:13:24Z

Codecov Report

Merging #563 (1a00054) into unstable (5e63954) will increase coverage by 0.35%.
The diff coverage is 87.86%.

@@             Coverage Diff              @@
##           unstable     #563      +/-   ##
============================================
+ Coverage     72.97%   73.32%   +0.35%     
============================================
  Files            35       36       +1     
  Lines          5414     5508      +94     
============================================
+ Hits           3951     4039      +88     
- Misses         1463     1469       +6

Impacted Files	Coverage Δ
src/kw_config_loader.sh	`53.78% <0.00%> (-0.46%)`	⬇️
src/plugins/kernel_install/utils.sh	`82.98% <79.41%> (+2.39%)`	⬆️
src/deploy.sh	`66.72% <88.42%> (+2.15%)`	⬆️
src/plugins/kernel_install/debian.sh	`69.56% <90.00%> (+2.06%)`	⬆️
src/plugins/kernel_install/rpi_bootloader.sh	`96.29% <96.29%> (ø)`
src/plugins/kernel_install/arch.sh	`72.88% <100.00%> (+0.95%)`	⬆️
src/plugins/kernel_install/grub.sh	`89.58% <0.00%> (-4.17%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4b9857...1a00054. Read the comment docs.

melissawen · 2022-02-27T20:33:00Z

src/deploy.sh

  if [[ ! -f "$config_path" ]]; then
-    warning "$complain_msg"
-    return
+    warning 'Undefined .config file for the target kernel. Consider using kw bd'


I don't understand this guidance of "using kw bd" to fix the missing config file.

That is a long topic, but in summary, if we just use kw d we can copy the .config file, but we are not sure if the current build corresponds to the current config file. Think about this scenario:

You build the kernel

You replace the config file

You deploy your kernel

Notice that we are deploying a kernel in which the config file does not match the build kernel. However, we don't have this problem with kw bd because right after build the kernel; we deploy it, which does not give time for users to change the config file.

Hmm.. got it. But I started from the point that I don't have any .config file in the folder and, therefore, this message showed up. From the situation you explained, the statement makes sense. However, I'm unsure if users can figure out what this guidance means if they don't handle any .config file.

src/deploy.sh

melissawen · 2022-02-28T20:15:36Z

src/deploy.sh

+  # 2. Copy kernel image
+  case "$arch" in
+    'arm')
+      kernel_name_arch="kernel-${name}.img"


I think kernel-version+.img is not friendly, to be more specific, the +.img sufix that is generated when joining kernel name with .img. Is it possible to drop the + or the .img?

hmmm... I'm not sure if this is kw responsibility. I think this is .config change.

I mean, the name of the kernel image when you copy it, I would remove the .img suffix and use only kernel-${name}. Btw, I tested the last version and the image now is copied as vmlinuz-{name} that is weird too :) I guess it is happening because only arch=arm is considered in this switch case, so I think arch=arm64 should be included here too, right?

src/plugins/kernel_install/rpi_bootloader.sh

src/deploy.sh

rodrigosiqueira

I'm going to prepare a new version, and I'll probably add one extra commit to support rpi 64.

src/deploy.sh

rodrigosiqueira · 2022-03-05T14:01:08Z

src/deploy.sh

  if [[ ! -f "$config_path" ]]; then
-    warning "$complain_msg"
-    return
+    warning 'Undefined .config file for the target kernel. Consider using kw bd'


That is a long topic, but in summary, if we just use kw d we can copy the .config file, but we are not sure if the current build corresponds to the current config file. Think about this scenario:

You build the kernel

You replace the config file

You deploy your kernel

Notice that we are deploying a kernel in which the config file does not match the build kernel. However, we don't have this problem with kw bd because right after build the kernel; we deploy it, which does not give time for users to change the config file.

rodrigosiqueira · 2022-03-05T14:02:23Z

src/deploy.sh

+  # 2. Copy kernel image
+  case "$arch" in
+    'arm')
+      kernel_name_arch="kernel-${name}.img"


hmmm... I'm not sure if this is kw responsibility. I think this is .config change.

src/deploy.sh

melissawen · 2022-03-20T16:33:30Z

Thanks for adding this feature; kw will help me even more in daily development tasks.
I pointed out some minor fixes, but in general, LGTM. I still need to check back 32bit, but I didn't see anything that could cause regression from the previous version.

It was difficult to understand some points when configuring kworkflow.config because I had some "bias" from my previous setup.

I needed to give ownership fo /opt/kw (in my RPi) to my user to be able to copy kw scripts there. But good chances to be something that I missed in my auth setup.
dtb_copy_pattern: from what I understood by reading comments, I started with broadcom; but I realized it was copying the entire folder, then I switched to broadcom/* and it works as expected.
Maybe a tutorial (with some examples) would help new users with this feature.

Thanks again

Some config files that do not enable CONFIG_MODULE_SIG do not have the extra SIGN output, making the progress bar display only half of the progress. This commit fixes this issue by first checking the CONFIG_MODULE_SIG=y option in the config; If it is not enabled, it drops the 2 factor multiply. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

When deploying a kernel to a target machine, we have multiple copy steps spread in different parts of the code, which is a problem since it makes kw deploy code every trick. This commit creates a function responsible for packing all the required files for the boot folder in a single place and send to the target device. This change represents a code refactor in the deploy, which required multiple changes in the test. Since various tests broke during the development of this feature, I also took this opportunity to refactor our tests to make them more manageable. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

This commit is the final piece for enabling kernel deploy for a Raspberry PI that uses Raspbian. This commit refactors the bootloader update part to remove unnecessary steps for non-grub-based systems and add the file responsible for manipulating the config.txt file. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

This commit is responsible for fine-tuning the kernel removal for raspberry pi. Related to kworkflow#354 Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

This commit introduces the final adjustment to enable local deploy. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

This commit ensures that we do not pollute the config.txt file by removing kernels name duplications and adding the new kernel to the end. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

For deploying a new kernel to a Raspberry Pi with 32 bits, we need to copy the dtb files by using: cp arch/arm/boot/dts/*.dtb <PATH> In the 64 bits scenario we have to use: cp arch/arm64/boot/dts/broadcom/*.dtb <PATH> This tiny difference raised the attention that we need to handle dtb files deploy a little bit more carefully since any vendor can specify any boot layout (at least, this is what I understood). For this reason, this commit introduces a new config option named dtb_copy_pattern, which gives the developers the flexibility to tell kw how it should copy these dtbs files. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

In the kworkflow.config file, we have a variable named kernel_img_name used to define the binary kernel prefix used during the deploy. Even though we have this variable, we were ignoring it. As a result, when we deploy a new kernel image to a rasp system, we see a kernel image with a vmlinuz prefix. This commit fixes this issue by stopping to ignore that config variable and only set vmlinuz if that variable is empty. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

melissawen · 2022-03-26T21:27:26Z

LGTM.
I also tested on Pi4 32bits and 64bits, everything went well.
Thanks a lot!

rodrigosiqueira · 2022-03-27T12:44:40Z

Hi @melissawen , thanks a lot for your review and test. I merged this PR in the unstable branch.

rodrigosiqueira added New feature request deploy labels Feb 20, 2022

rodrigosiqueira added this to the kworkflow-beta milestone Feb 20, 2022

rodrigosiqueira requested a review from melissawen February 20, 2022 18:09

rodrigosiqueira assigned melissawen Feb 20, 2022

rodrigosiqueira force-pushed the adding-support-to-rpi-deploy branch 2 times, most recently from 037ab3a to 20f87b2 Compare February 26, 2022 03:59

rodrigosiqueira changed the title ~~Adding support to rpi deploy~~ Adding support to rpi 32 deploy Feb 27, 2022

melissawen reviewed Mar 1, 2022

View reviewed changes

rodrigosiqueira commented Mar 5, 2022

View reviewed changes

rodrigosiqueira force-pushed the adding-support-to-rpi-deploy branch from c53e4de to 679378e Compare March 16, 2022 11:27

rodrigosiqueira changed the title ~~Adding support to rpi 32 deploy~~ Adding support to raspbian 32/64 deploy Mar 16, 2022

rodrigosiqueira force-pushed the adding-support-to-rpi-deploy branch from 679378e to e93ea54 Compare March 19, 2022 19:12

rodrigosiqueira force-pushed the adding-support-to-rpi-deploy branch 9 times, most recently from 1a00054 to 4bd8b79 Compare March 26, 2022 18:58

rodrigosiqueira added 6 commits March 26, 2022 15:54

src: plugin: kernel_install: utils: Fine-tuning RPI kernel removal

f869ae9

This commit is responsible for fine-tuning the kernel removal for raspberry pi. Related to kworkflow#354 Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

src: plugins: kernel_install: rpi: Add local deploy support

a93b050

This commit introduces the final adjustment to enable local deploy. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

src: deploy: Add new kernels to the end of the file

2d222f8

This commit ensures that we do not pollute the config.txt file by removing kernels name duplications and adding the new kernel to the end. Signed-off-by: Rodrigo Siqueira <siqueirajordao@riseup.net>

rodrigosiqueira added 2 commits March 26, 2022 15:54

rodrigosiqueira force-pushed the adding-support-to-rpi-deploy branch from 4bd8b79 to f02d3b3 Compare March 26, 2022 19:56

rodrigosiqueira closed this Mar 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support to raspbian 32/64 deploy #563

Adding support to raspbian 32/64 deploy #563

rodrigosiqueira commented Feb 20, 2022

codecov-commenter commented Feb 20, 2022 •

edited

melissawen Feb 27, 2022

rodrigosiqueira Mar 5, 2022

melissawen Mar 20, 2022

melissawen Feb 28, 2022

rodrigosiqueira Mar 5, 2022

melissawen Mar 20, 2022

rodrigosiqueira left a comment

rodrigosiqueira Mar 5, 2022

rodrigosiqueira Mar 5, 2022

melissawen commented Mar 20, 2022

melissawen commented Mar 26, 2022

rodrigosiqueira commented Mar 27, 2022

Adding support to raspbian 32/64 deploy #563

Adding support to raspbian 32/64 deploy #563

Conversation

rodrigosiqueira commented Feb 20, 2022

codecov-commenter commented Feb 20, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigosiqueira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissawen commented Mar 20, 2022

melissawen commented Mar 26, 2022

rodrigosiqueira commented Mar 27, 2022

codecov-commenter commented Feb 20, 2022 •

edited