-
Notifications
You must be signed in to change notification settings - Fork 1.5k
OCPBUGS-65566: Report new error while updating bootstrap ign #10139
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
base: main
Are you sure you want to change the base?
Conversation
|
@sadasu: This pull request references Jira Issue OCPBUGS-65566, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-gcp-custom-dns |
|
/jira refresh |
|
@sadasu: This pull request references Jira Issue OCPBUGS-65566, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/retest-required |
|
/test e2e-gcp-custom-dns |
| // Reset error state | ||
| updateError = nil | ||
|
|
||
| break |
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.
it seems more intuitive to me, that we would return nil here and return error at the end of the function
|
/approve In the bug, this was listed as one of the options for an acceptable fix, but it's unclear to me with this path how we will resolve the CI failures. With this code the job would start to throw an error. Are we planning on disabling promtail in this job? |
@patrickdillon, I am not sure why/how enabling promtail would cause us not to be able to parse the bootstrap ignition and update the infrastructure manifest. That is essentially what is happening here. |
Yes, promtail is already disabled in customer dns job via release PR.
What I observed is that the data in bootstrap ignition file is based64 encoded and gzip compressed when enabling promtail. For the normal bootstrap ignition file, it is text plain. pre-merge tested, job with enabled promtail failed with error. I guess the fix should be acceptable. @sadasu @patrickdillon wdyt? |
|
/test e2e-azure-custom-dns |
|
@jinyunma: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn 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-sigs/prow repository. |
|
/test e2e-azure-custom-dns-techpreview |
9686416 to
7571f2e
Compare
|
New changes are detected. LGTM label has been removed. |
7571f2e to
b6dfa3b
Compare
Detect when the infrastructure manifest could not be updated within the Bootstrap ignition and report that.
When userProvisionedDNS is enabled, we edit the bootstrap Ignition after it is created to inser Load Balancer IP information. It is possible for a user to start their cluste install with their own bootstrap ignition that could be base64 encoded. So, decode the bootstrap ignition before updating it.
b6dfa3b to
92fff40
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-gcp-custom-dns |
|
@patrickdillon, the bootstrap Ignition file being modified with clusterapi code is obtained from the bootstrap Ign asset. If the user provided their own updated bootstrap ignition file as would be the case after the promtail injection step in CI, would it present itself as the bootstrap ign asset? @jinyunma thanks for trying the version of the fix that just reports error when it can't read the Infrastructure manifest in the bootstrap ignition. If regular installs work with promtail enabled, then maybe we should support it with custom-dns too. My 2nd commit just tries to base64 decode the entire ignition file befor JSON unmarshaling it. Based on this comment it appears that the bootstrap ign is also gunzipped. Could you please confirm? (If so, my 2nd commit is insufficient.) Also, it appears that a user provided Bootstrap ignition to create a cluster only happens with UPI where the user is required to provision other infrastructure too. Is this within the scope of custom-dns IPI? |
|
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
I downloaded bootstrap.ign file from installer created storage account in running CI cluster, and checked that the data is gunzipped and base64 compressed, also sent the file to you via slack DM. |
Detect when the infrastructure manifest could not be updated within the Bootstrap ignition and report that.