Skip to content

Bug 2013726: [on-prem] Update NetworkManager-resolv-prepender.yaml#2790

Closed
Rupesh-git-eng wants to merge 2 commits intoopenshift:masterfrom
Rupesh-git-eng:patch-1
Closed

Bug 2013726: [on-prem] Update NetworkManager-resolv-prepender.yaml#2790
Rupesh-git-eng wants to merge 2 commits intoopenshift:masterfrom
Rupesh-git-eng:patch-1

Conversation

@Rupesh-git-eng
Copy link
Copy Markdown

@Rupesh-git-eng Rupesh-git-eng commented Oct 5, 2021

Fixies # Don't add white namespace while generating /etc/resolv.conf on nodes if variables are empty.
Empty NetworkManager variables cause generating whitespace in /etc/resolv.conf between search and search domains.
This patch checks if the variables are empty. If empty, don't add it to Variable $DOMAINS

i.e

$ cat /etc/resolv.conf

Generated by KNI resolv prepender NM dispatcher script

search abc.com ## three whitespace added here between search and abc.com
nameserver xx.xx.xx.xx
nameserver xx.xx.xx.xx
nameserver xx.xx.xx.xx

- What I did
Validate if variables are set to empty.

- How to verify it
Don't pass search domain via dhcp service.

- Description for the changelog
Validate the NetworkManager variables before using them in the dispatcher script.

Validate the NetworkManager variables before using them in the dispatcher script.

Empty variables cause of generating whitespace in /etc/resolv.conf between search and search domains.
@openshift-ci openshift-ci Bot requested review from jstuever and mdbooth October 5, 2021 10:23
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 5, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Rupesh-git-eng
To complete the pull request process, please assign sinnykumari after the PR has been reviewed.
You can assign the PR to them by writing /assign @sinnykumari in a comment when ready.

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

Details 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

@kikisdeliveryservice kikisdeliveryservice changed the title Update NetworkManager-resolv-prepender.yaml [on-prem] Update NetworkManager-resolv-prepender.yaml Oct 5, 2021
@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

@Rupesh-git-eng just double checking: is this related to a bug?

@mandre
Copy link
Copy Markdown
Member

mandre commented Oct 12, 2021

It's not clear to me what we're trying to achieve here. Is it a cosmetic change?

@Rupesh-git-eng
Copy link
Copy Markdown
Author

@Rupesh-git-eng just double checking: is this related to a bug?

Yes.

@Rupesh-git-eng
Copy link
Copy Markdown
Author

It's not clear to me what we're trying to achieve here. Is it a cosmetic change?

I would say both...
Cosmetic change - We are seeing a white namespace in /etc/resolv.conf
Bug - We are not using proper variables while DHCP Is used and the search list is provided by DHCP option 119.

Unfortunately, I don't have a test environment to test this and I was looking for a way to test it.
I can assist in testing, if required, or do the entire testing myself, if I get the image release or hackway

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

@Rupesh-git-eng just double checking: is this related to a bug?

Yes.

@Rupesh-git-eng can you link to the bug ?

@Rupesh-git-eng
Copy link
Copy Markdown
Author

@Rupesh-git-eng
Copy link
Copy Markdown
Author

@Rupesh-git-eng just double checking: is this related to a bug?

Yes.

@Rupesh-git-eng can you link to the bug ?

Attached above.

@sinnykumari sinnykumari changed the title [on-prem] Update NetworkManager-resolv-prepender.yaml Bug 2013726: [on-prem] Update NetworkManager-resolv-prepender.yaml Oct 14, 2021
@sinnykumari
Copy link
Copy Markdown
Contributor

/bugzilla refresh

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 14, 2021

@sinnykumari: Bugzilla bug 2013726 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • qe_staff
  • redhat
Details

In response to this:

/bugzilla refresh

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.

@sinnykumari
Copy link
Copy Markdown
Contributor

/bugzilla refresh

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 14, 2021

@sinnykumari: Bugzilla bug 2013726 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • qe_staff
  • redhat
Details

In response to this:

/bugzilla refresh

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.

@sinnykumari
Copy link
Copy Markdown
Contributor

@sinnykumari: Bugzilla bug 2013726 is in a bug group that is not in the allowed groups for this repo. Allowed groups for this repo are:

* qe_staff

* redhat

Not sure, why this is complaining.

@jstuever
Copy link
Copy Markdown
Contributor

jstuever commented Nov 9, 2021

/uncc

@openshift-ci openshift-ci Bot removed the request for review from jstuever November 9, 2021 00:14
@mandre
Copy link
Copy Markdown
Member

mandre commented Nov 9, 2021

@sinnykumari: Bugzilla bug 2013726 is in a bug group that is not in the allowed groups for this repo. Allowed groups for this repo are:

* qe_staff

* redhat

Not sure, why this is complaining.

It complains because the bug is private. This looks like a dup of #2823, @cybertron do you confirm?

@cybertron
Copy link
Copy Markdown
Member

It's not exactly a dupe, but #2823 will also fix this. I would prefer to just merge that because it also fixes another bug.

I'm also not entirely sure about all of the changes in this patch. My understanding when we added IP4 and IP6_DOMAINS was that they were supposed to include DHCP-provided nameservers (see bbbb722#diff-255f8a4599166f31961853ea8626f969ca4231c55aacbc20a5bb3ceb640f911d). One side benefit of #2823 is that we just use what NetworkManager provides instead of having to come up with a complete list ourselves. That should result in less surprising behavior and eliminate questions like this.

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

adding a hold until the above is resolved.

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 17, 2021

@Rupesh-git-eng: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws 8bb5620 link false /test okd-e2e-aws
ci/prow/e2e-aws-single-node 8bb5620 link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel8 8bb5620 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-aws-workers-rhel7 8bb5620 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-metal-ipi 8bb5620 link false /test e2e-metal-ipi
ci/prow/e2e-aws-disruptive 8bb5620 link false /test e2e-aws-disruptive
ci/prow/e2e-gcp-op 8bb5620 link true /test e2e-gcp-op
ci/prow/e2e-agnostic-upgrade 8bb5620 link true /test e2e-agnostic-upgrade
ci/prow/e2e-aws 8bb5620 link true /test e2e-aws

Full PR test history. Your PR dashboard.

Details

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.

@cybertron
Copy link
Copy Markdown
Member

Note that #2823 has merged so the bug being addressed by this should be fixed. Even if we do need an additional patch, this one won't work because the other PR changed how the domain list is built.

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

Note that #2823 has merged so the bug being addressed by this should be fixed. Even if we do need an additional patch, this one won't work because the other PR changed how the domain list is built.

Thanks for the update and details, @cybertron ! Given the above, I'll close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants