Skip to content

upi/metal: Add pxe_kernel_args, stop using pxe_os_image_url #3865

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 5 commits into from
Aug 6, 2020
Merged

upi/metal: Add pxe_kernel_args, stop using pxe_os_image_url #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4008d35
upi/metal: Add pxe_kernel_args, stop using pxe_os_image_url
sdodson Jul 8, 2020
1f9aaac
upi/metal: Docs updates for liveimage
sdodson Jul 10, 2020
a53180b
fix local.kernel_args
sdodson Jul 11, 2020
e408a4d
docs suggestions
sdodson Jul 13, 2020
4bc5a88
update w/ Suggestions
sdodson Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/user/metal/install_upi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ TODO RHEL CoreOS does not have assets for bare-metal.

* `rd.neednet=1`: [CoreOS Installer][coreos-installer] needs internet access to fetch the OS image that needs to be installed on the machine.

* `coreos.inst.image_url`: Required for versions <= 4.5 only; should be the URL to a CoreOS "metal" image. In 4.6+ the metal image is included in the Live images.

* CoreOS Installer [arguments][coreos-installer-args] are required to be configured to install RHCOS and setup the Ignition config file for that machine.

## Watching your installation
Expand Down
8 changes: 4 additions & 4 deletions upi/metal/config.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,13 @@ EOF

}

variable "pxe_os_image_url" {
variable "pxe_kernel_args" {
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 9, 2020

Choose a reason for hiding this comment

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

can the user pass multiple args? if so can we include an example of that? and make sure it will actually work?

Copy link
Member

@cgwalters cgwalters Jul 9, 2020

Choose a reason for hiding this comment

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

I think we should support providing multiple arguments yes; there are various use cases for that, especially debugging. But I wouldn't block on it.

Copy link
Member Author

@sdodson sdodson Jul 9, 2020

Choose a reason for hiding this comment

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

right now they could, I assume by using a space delimited string, I assume there's a better type for this in tfvars?

Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 9, 2020

Choose a reason for hiding this comment

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

If it is space separated, then can we show that as example in the description.

Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 9, 2020

Choose a reason for hiding this comment

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

also since you are changing the input variable, can you also update https://github.com/openshift/installer/blob/fa2d5bbe2ba00b36c697de7ff0658f51df36a20a/upi/metal/terraform.tfvars.example

and also updating docs on how to provide the os image url with this new input variable, maybe here https://github.com/openshift/installer/blob/fa2d5bbe2ba00b36c697de7ff0658f51df36a20a/docs/user/metal/install_upi.md#required-kernel-parameters-during-pxe

Copy link
Member Author

@sdodson sdodson Jul 10, 2020

Choose a reason for hiding this comment

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

@abhinavdahiya Thanks, 0402482 should have docs updates. If there's a more backwards compatible manner to have made this change let me know if you think I should pursue other options.

type = string
default = ""

description = <<EOF
URL to the OS image for RHCOS that should be installed on machines.

For more info: https://github.com/coreos/coreos-installer#kernel-command-line-options-for-coreos-installer-running-in-the-initramfs
Arbitrary kernel arguments, space delimited ie:
coreos.inst.image_url=http://example.com/image.gz coreos.color=blue
EOF

}
Expand Down
9 changes: 6 additions & 3 deletions upi/metal/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ locals {

# "rd.break=initqueue"
"coreos.inst=yes",
Copy link
Contributor

@bgilbert bgilbert Jul 27, 2020

Choose a reason for hiding this comment

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

This is now a no-op.


"coreos.inst.image_url=${var.pxe_os_image_url}",
"coreos.inst.install_dev=sda",
Copy link
Contributor

@bgilbert bgilbert Jul 27, 2020

Choose a reason for hiding this comment

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

This will still work, but we generally spell it coreos.inst.install_dev=/dev/sda now.

"coreos.inst.skip_media_check",
Copy link
Contributor

@bgilbert bgilbert Jul 27, 2020

Choose a reason for hiding this comment

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

Also a no-op.

Copy link
Member Author

@sdodson sdodson Jul 27, 2020

Choose a reason for hiding this comment

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

Right, this is only here for now until we land openshift/release#10121

]
Expand Down Expand Up @@ -52,6 +50,7 @@ resource "matchbox_profile" "master" {
args = concat(
local.kernel_args,
["coreos.inst.ignition_url=${var.matchbox_http_endpoint}/ignition?cluster_id=${var.cluster_id}&role=master"],
[var.pxe_kernel_args],
)

raw_ignition = file(var.master_ign_file)
Expand All @@ -68,6 +67,7 @@ resource "matchbox_profile" "worker" {
args = concat(
local.kernel_args,
["coreos.inst.ignition_url=${var.matchbox_http_endpoint}/ignition?cluster_id=${var.cluster_id}&role=worker"],
[var.pxe_kernel_args],
)

raw_ignition = file(var.worker_ign_file)
Expand Down Expand Up @@ -134,7 +134,10 @@ module "bootstrap" {

pxe_kernel = local.pxe_kernel
pxe_initrd = local.pxe_initrd
pxe_kernel_args = local.kernel_args
pxe_kernel_args = concat(
local.kernel_args,
[var.pxe_kernel_args],
)
matchbox_http_endpoint = var.matchbox_http_endpoint
igntion_config_content = file(var.bootstrap_ign_file)

Expand Down
24 changes: 13 additions & 11 deletions upi/metal/terraform.tfvars.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ bootstrap_ign_file = ""
cluster_domain = ""

// The number of control plane machines required.
//
//
// Since etcd is colocated on control plane machines, suggested number is 3 or 5.
// Default: 1
master_count = "1"
Expand All @@ -17,22 +17,22 @@ master_count = "1"
master_ign_file = ""

// Client certificate used to authenticate with the matchbox RPC API.
//
//
// For more info: https://github.com/coreos/matchbox/blob/master/Documentation/api.md
matchbox_client_cert = "matchbox/tls/client.crt"

// Client certificate's key used to authenticate with the matchbox RPC API.
//
//
// For more info: https://github.com/coreos/matchbox/blob/master/Documentation/api.md
matchbox_client_key = "matchbox/tls/client.key"

// HTTPS endpoint for matchbox. This must include the scheme
//
//
// For more info: https://github.com/coreos/matchbox/blob/master/Documentation/api.md
matchbox_http_endpoint = ""

// RPC endpoint for matchbox.
//
//
// For more info: https://godoc.org/github.com/coreos/matchbox/matchbox/client
matchbox_rpc_endpoint = ""

Expand All @@ -46,22 +46,24 @@ packet_project_id = ""
public_r53_zone = ""

// URL to the initrd image that should be used to PXE machines.
//
//
// This can be a fully-qualified URL or URL relative to matchbox_http_endpoint to use Matchbox assets (https://github.com/coreos/matchbox/blob/master/Documentation/matchbox.md#assets).
pxe_initrd_url = ""

// URL to the kernel image that should be used to PXE machines.
//
//
// This can be a fully-qualified URL or URL relative to matchbox_http_endpoint to use Matchbox assets (https://github.com/coreos/matchbox/blob/master/Documentation/matchbox.md#assets).
pxe_kernel_url = ""

// URL to the OS image for RHCOS that should be installed on machines.
//
// PXE Boot Kernel args
// Note, earlier versions of this code relied on a url via pxe_os_image_url, this has been replaced
// with arbitrary kernel arguments which can include coreos.inst.image_url
//
// For more info: https://github.com/coreos/coreos-installer#kernel-command-line-options-for-coreos-installer-running-in-the-initramfs
pxe_os_image_url = ""
pxe_kernel_args = "coreos.inst.image_url=https://example.com/image.gz "

// The number of worker machines required.
//
//
// Default: 1
worker_count = "1"

Expand Down