Skip to content

iso: Fix console for vfkit/krunkit #20832

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nirs
Copy link
Contributor

@nirs nirs commented May 24, 2025

iso: Fix console for vfkit/krunkit

The serial console name depends on the driver. We had setting for qemu
that does not work for vfkit and krunkit, breaking boot from minikube
iso.

Fixed by using 2 console= options, one is known to work for qemu, and
one for vfkit and krunkit. With this we can use the same iso image with
qemu, vfkit, and krunkit.

This will allow simplifying vfkit driver. Previously we had to extract
the kernel and initrd and start it using the legacy --kernel,
--kernel-cmdline and --initrd options.

I tested this by building the iso with this fix and running with
--iso-url.

Example run with qemu

% minikube start -p qemu --driver qemu --container-runtime containerd \
    --iso-url file://$PWD/minikube-arm64.iso
😄  [qemu] minikube v1.36.0 on Darwin 15.5 (arm64)
✨  Using the qemu2 driver based on user configuration
🌐  Automatically selected the socket_vmnet network
👍  Starting "qemu" primary control-plane node in "qemu" cluster
🔥  Creating qemu2 VM (CPUs=2, Memory=6000MB, Disk=20000MB) ...
📦  Preparing Kubernetes v1.33.1 on containerd 1.7.23 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  Done! kubectl is now configured to use "qemu" cluster and "default" namespace by default

Example run with krunkit

% minikube start -p krunkit --driver krunkit --container-runtime containerd \
    --iso-url file://$PWD/minikube-arm64.iso
😄  [krunkit] minikube v1.36.0 on Darwin 15.5 (arm64)
✨  Using the krunkit (experimental) driver based on user configuration
👍  Starting "krunkit" primary control-plane node in "krunkit" cluster
🔥  Creating krunkit VM (CPUs=2, Memory=6000MB, Disk=20000MB) ...
📦  Preparing Kubernetes v1.33.1 on containerd 1.7.23 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔗  Configuring bridge CNI (Container Networking Interface) ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: default-storageclass, storage-provisioner
🏄  Done! kubectl is now configured to use "krunkit" cluster and "default" namespace by default

The serial console name depends on the driver. We had setting for qemu
that does not work for vfkit and krunkit, breaking boot from minikube
iso.

Fixed by using 2 console= options, one is known to work for qemu, and
one for vfkit and krunkit. With this we can use the same iso image with
qemu, vfkit, and krunkit.

This will allow simplifying vfkit driver. Previously we had to extract
the kernel and initrd and start it using the legacy --kernel,
--kernel-cmdline and --initrd options.

I tested this by building the iso with this fix and running with
--iso-url.

Example run with qemu:

    % minikube start -p qemu --driver qemu --container-runtime containerd \
        --iso-url file://$PWD/minikube-arm64.iso
    😄  [qemu] minikube v1.36.0 on Darwin 15.5 (arm64)
    ✨  Using the qemu2 driver based on user configuration
    🌐  Automatically selected the socket_vmnet network
    👍  Starting "qemu" primary control-plane node in "qemu" cluster
    🔥  Creating qemu2 VM (CPUs=2, Memory=6000MB, Disk=20000MB) ...
    📦  Preparing Kubernetes v1.33.1 on containerd 1.7.23 ...
        ▪ Generating certificates and keys ...
        ▪ Booting up control plane ...
        ▪ Configuring RBAC rules ...
    🔗  Configuring bridge CNI (Container Networking Interface) ...
    🔎  Verifying Kubernetes components...
        ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
    🌟  Enabled addons: default-storageclass, storage-provisioner
    🏄  Done! kubectl is now configured to use "qemu" cluster and "default" namespace by default

Example run with krunkit:

    % minikube start -p krunkit --driver krunkit --container-runtime containerd \
        --iso-url file://$PWD/minikube-arm64.iso
    😄  [krunkit] minikube v1.36.0 on Darwin 15.5 (arm64)
    ✨  Using the krunkit (experimental) driver based on user configuration
    👍  Starting "krunkit" primary control-plane node in "krunkit" cluster
    🔥  Creating krunkit VM (CPUs=2, Memory=6000MB, Disk=20000MB) ...
    📦  Preparing Kubernetes v1.33.1 on containerd 1.7.23 ...
        ▪ Generating certificates and keys ...
        ▪ Booting up control plane ...
        ▪ Configuring RBAC rules ...
    🔗  Configuring bridge CNI (Container Networking Interface) ...
    🔎  Verifying Kubernetes components...
        ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
    🌟  Enabled addons: default-storageclass, storage-provisioner
    🏄  Done! kubectl is now configured to use "krunkit" cluster and "default" namespace by default
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2025
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 24, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @nirs. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 24, 2025
@nirs nirs mentioned this pull request May 24, 2025
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@nirs
Copy link
Contributor Author

nirs commented May 24, 2025

@afbjorklund can you review?

@nirs nirs mentioned this pull request May 25, 2025
Copy link
Collaborator

@afbjorklund afbjorklund left a comment

Choose a reason for hiding this comment

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

I can't see that hvc0 is documented anywhere (buildroot only uses it for ppc64le) but it sounds reasonable.

It seems to only be mentioned here: https://github.com/crc-org/vfkit/blob/main/doc/usage.md

EDIT: OK, it is also upstream. "Use the first virtio console device as system console."

https://developer.apple.com/documentation/virtualization/running-linux-in-a-virtual-machine

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afbjorklund, nirs
Once this PR has been reviewed and has the lgtm label, please assign spowelljr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@afbjorklund
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2025
@afbjorklund
Copy link
Collaborator

ok-to-build-image

@minikube-bot
Copy link
Collaborator

Hi @nirs, we have updated your PR with the reference to newly built kicbase image. Pull the changes locally if you want to test with them or update your PR further.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 25, 2025
@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot

This comment has been minimized.

@nirs
Copy link
Contributor Author

nirs commented May 25, 2025

@afbjorklund it is also mentioned in the kernel documentation:
https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html

        console=        [KNL] Output console device and options.

                tty<n>  Use the virtual console device <n>.

                ttyS<n>[,options]
                ttyUSB0[,options]
                        Use the specified serial port.  The options are of
                        the form "bbbbpnf", where "bbbb" is the baud rate,
                        "p" is parity ("n", "o", or "e"), "n" is number of
                        bits, and "f" is flow control ("r" for RTS or
                        omit it).  Default is "9600n8".

                        See Documentation/admin-guide/serial-console.rst for more
                        information.  See
                        Documentation/networking/netconsole.txt for an
                        alternative.

                uart[8250],io,<addr>[,options]
                uart[8250],mmio,<addr>[,options]
                uart[8250],mmio16,<addr>[,options]
                uart[8250],mmio32,<addr>[,options]
                uart[8250],0x<addr>[,options]
                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address,
                        switching to the matching ttyS device later.
                        MMIO inter-register address stride is either 8-bit
                        (mmio), 16-bit (mmio16), or 32-bit (mmio32).
                        If none of [io|mmio|mmio16|mmio32], <addr> is assumed
                        to be equivalent to 'mmio'. 'options' are specified in
                        the same format described for ttyS above; if unspecified,
                        the h/w is not re-initialized.

                hvc<n>  Use the hypervisor console device <n>. This is for
                        both Xen and PowerPC hypervisors.

                If the device connected to the port is not a TTY but a braille
                device, prepend "brl," before the device type, for instance
                        console=brl,ttyS0
                For now, only VisioBraille is supported.

Not sure why it is documented only for Xen and PowerPC, maybe someone forgot to updated this.

@nirs
Copy link
Contributor Author

nirs commented May 25, 2025

@cfergeau can you review?

@afbjorklund
Copy link
Collaborator

afbjorklund commented May 25, 2025

I meant to say "iso", now it built a useless kicbase instead (feel free to remove that commit)

@nirs
Copy link
Contributor Author

nirs commented May 25, 2025

I meant to say "iso", now it built a useless kicbase instead (feel free to remove that commit)

I was wondering why the bot added kicbase instead of iso :-)

@nirs nirs force-pushed the iso-console-fix branch from c99e020 to 2c25580 Compare May 25, 2025 13:37
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2025
@afbjorklund
Copy link
Collaborator

ok-to-build-iso

@minikube-pr-bot

This comment has been minimized.

@cfergeau
Copy link

@cfergeau can you review?

hvc0 looks good to me, ttyAMA0 is apparently when you have a UART serial port, which vfkit unfortunately does not have (this would allow for early boot messages)

@nirs
Copy link
Contributor Author

nirs commented May 26, 2025

@medyagh do you know why the ISO PR Build Push failed? it seems to be accessible only to Googlers, unlike the other Jenkins jobs.

I built and tested this iso on arm64, and built it for x86_64, so it should be ok.

@medyagh
Copy link
Member

medyagh commented May 27, 2025

@medyagh do you know why the ISO PR Build Push failed? it seems to be accessible only to Googlers, unlike the other Jenkins jobs.

I built and tested this iso on arm64, and built it for x86_64, so it should be ok.

See the logs at:
https://storage.googleapis.com/minikube-builds/logs/PRNumber/7DigitSHA/iso_build.txt

somehow for this PR the commit changed, I wonder if you force-pushed and squashed commits after build command?

@nirs
Copy link
Contributor Author

nirs commented May 29, 2025

@medyagh do you know why the ISO PR Build Push failed? it seems to be accessible only to Googlers, unlike the other Jenkins jobs.
I built and tested this iso on arm64, and built it for x86_64, so it should be ok.

See the logs at: https://storage.googleapis.com/minikube-builds/logs/PRNumber/7DigitSHA/iso_build.txt

somehow for this PR the commit changed, I wonder if you force-pushed and squashed commits after build command?

@afbjorklund asked to build and image instead of an iso, so I forced push to drop the image commit added by the boot. It seems that @afbjorklund asked to build an iso after I pushed but maybe it was in parallel.

Should I rebase on master and ask to build an iso again? I don't want waste hours of build time if it is going to fail again.

@medyagh
Copy link
Member

medyagh commented May 29, 2025

ok-to-build-iso

@minikube-bot
Copy link
Collaborator

Hi @nirs, we have updated your PR with the reference to newly built ISO. Pull the changes locally if you want to test with them or update your PR further.

@nirs
Copy link
Contributor Author

nirs commented May 29, 2025

ok-to-test

@minikube-pr-bot

This comment has been minimized.

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 20832) |
+----------------+----------+---------------------+
| minikube start | 51.5s    | 52.9s               |
| enable ingress | 14.9s    | 15.1s               |
+----------------+----------+---------------------+

Times for minikube ingress: 14.5s 14.5s 15.0s 15.0s 15.5s
Times for minikube (PR 20832) ingress: 14.5s 15.0s 15.5s 15.5s 15.0s

Times for minikube start: 52.1s 52.2s 48.2s 52.4s 52.7s
Times for minikube (PR 20832) start: 53.2s 55.0s 52.8s 51.1s 52.5s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 20832) |
+----------------+----------+---------------------+
| minikube start | 23.4s    | 23.6s               |
| enable ingress | 12.3s    | 12.4s               |
+----------------+----------+---------------------+

Times for minikube start: 25.8s 21.8s 26.0s 21.7s 21.6s
Times for minikube (PR 20832) start: 22.3s 22.0s 24.8s 23.7s 25.2s

Times for minikube ingress: 10.3s 13.8s 12.2s 12.8s 12.3s
Times for minikube (PR 20832) ingress: 10.7s 12.8s 13.3s 12.3s 12.8s

docker driver with containerd runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 20832) |
+----------------+----------+---------------------+
| minikube start | 21.8s    | 22.3s               |
| enable ingress | 45.4s    | 39.4s               |
+----------------+----------+---------------------+

Times for minikube start: 20.5s 22.2s 20.5s 24.2s 21.8s
Times for minikube (PR 20832) start: 20.8s 22.1s 21.4s 24.2s 23.0s

Times for minikube (PR 20832) ingress: 39.3s 40.3s 38.8s 39.3s 39.3s
Times for minikube ingress: 38.3s 38.8s 39.3s 39.3s 71.3s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants