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
upi/metal: Add pxe_kernel_args, stop using pxe_os_image_url #3865
upi/metal: Add pxe_kernel_args, stop using pxe_os_image_url #3865
Conversation
/hold for the blocker |
/retest |
@sohankunkerkar Need to call out the specific test, it's not included by default right now because we don't have context specific jobs yet. |
ah, thanks for the pointer. |
@@ -132,13 +132,12 @@ EOF | |||
|
|||
} | |||
|
|||
variable "pxe_os_image_url" { | |||
variable "pxe_kernel_args" { |
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.
can the user pass multiple args? if so can we include an example of that? and make sure it will actually work?
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.
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.
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.
right now they could, I assume by using a space delimited string, I assume there's a better type for this in tfvars?
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 it is space separated, then can we show that as example in the description.
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.
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
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.
@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.
/test e2e-metal |
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.
I think after looking at https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/3865/pull-ci-openshift-installer-master-e2e-metal/1281237439557406720#1:build-log.txt%3A328, it looks like you'd need s/"${var.pxe_kernel_args}"/var.pxe_kernel_args
Looks like CI is still complaining about this failure.
How about adding pxe_kernel_args = var.pxe_kernel_args
here and change s/var.pxe_kernel_args/local.pxe_kernel_args
/test e2e-metal |
It's atleast made it that far, going to squash out my tf newbness now that it's working. |
/test e2e-metal |
1 similar comment
/test e2e-metal |
As long as a PR is opened by a member it will re-run tests on push. So it shouldn't be necessary to trigger it explicitly. |
Of course as soon as I say that the job gets jammed up and doesn't run. |
/retest |
1 similar comment
/retest |
/test e2e-metal |
/retest |
1 similar comment
/retest |
d3f8212
to
d89eaa0
Compare
/test e2e-metal |
|
0884932
to
4bc5a88
Compare
/test e2e-metal |
/retest |
Now we've got https://bugzilla.redhat.com/show_bug.cgi?id=1861828 or https://bugzilla.redhat.com/show_bug.cgi?id=1860789
|
So it looks like the nodes are booting and getting the ignition from the cluster and the kubelet runs to move to send CSRs but it not getting approved due to oc bug. So i'm fine moving ahead with this PR. /approve /hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I'm fine, this really doesn't change anything until we land the release repo changes so moving forward. |
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@sdodson: The following tests failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
/override ci/prow/e2e-aws |
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aws In response to this:
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/test-infra repository. |
/override ci/prow/e2e-libvirt |
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aws-fips, ci/prow/e2e-aws-workers-rhel7, ci/prow/e2e-crc, ci/prow/e2e-libvirt, ci/prow/e2e-metal-ipi In response to this:
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/test-infra repository. |
Requires openshift/release#9960