Skip to content
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

Show DNS Resolver in NNS #867

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Conversation

rhrazdil
Copy link
Collaborator

@rhrazdil rhrazdil commented Nov 4, 2021

Signed-off-by: Radim Hrazdil [email protected]

Is this a BUG FIX or a FEATURE ?:

Uncomment only one, leave it on its own line:

/kind bug
/kind enhancement

What this PR does / why we need it:

DNS Resolver is not included in NodeNetworkState, adding types to marshal/unmarshal
DNS data and include it in the NNS as well.

Special notes for your reviewer:

Release note:

Show dns-resolver in NodeNetworkState

Signed-off-by: Radim Hrazdil <[email protected]>
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/enhancement dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 4, 2021
Context("With DNS Resolver populated", func() {
BeforeEach(func() {
state = nmstate.NewState(`interfaces:
- name: eth1
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we use the firstSecondaryNic variable here instead of using eth1 directly? Because the e2e tests in OCP use another NIC name for the first secondary (from FIRST_SECONDARY_NIC env var)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, this is just a unit test and the names don't really matter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry. My fault

@qinqon
Copy link
Member

qinqon commented Nov 4, 2021

So this was introduced at #709, maybe we don't need that anymore.

@rhrazdil
Copy link
Collaborator Author

rhrazdil commented Nov 4, 2021

So this was introduced at #709, maybe we don't need that anymore.

#709 introduces structures for routes and interfaces, but not the dns-resolver.

@qinqon
Copy link
Member

qinqon commented Nov 4, 2021

So this was introduced at #709, maybe we don't need that anymore.

#709 introduces structures for routes and interfaces, but not the dns-resolver.

Looks like nmstate is going to move to yaml 1.2 at the rust implementation, so when we integrate with it we can revert the PR.

@qinqon
Copy link
Member

qinqon commented Nov 4, 2021

/retest

Something is fishy at CI maybe it's time to use podman.

creating container buildx_buildkit_kubernetes-nmstate-builder0
#1 22.62 error: failed to list workers: Unavailable: connection error: desc = "transport: error while dialing: dial unix /run/buildkit/buildkitd.sock: connect: no such file or directory"
#1 creating container buildx_buildkit_kubernetes-nmstate-builder0 22.2s done
#1 

@yboaron
Copy link
Collaborator

yboaron commented Nov 4, 2021

Thanks for adding this fix.

Checked it in my env - looks OK.

It is worth mentioning that (at least based on what I get in my env) , it seems that nns.dnsresolver equals to /var/run/NetworkManager/resolv.conf and not /etc/resolv.conf

@yboaron
Copy link
Collaborator

yboaron commented Nov 4, 2021

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@qinqon
Copy link
Member

qinqon commented Nov 4, 2021

Thanks for adding this fix.

Checked it in my env - looks OK.

It is worth mentioning that (at least based on what I get in my env) , it seems that nns.dnsresolver equals to /var/run/NetworkManager/resolv.conf and not /etc/resolv.conf

That depends on the NetworkManager configuration, you can configure if you want it to manage /etc/resolv.conf or not, for example at OCP we have NetworkManager not manager /etc/resolv.conf but at the same time we have the prepend dispatcher script.

From an kubernetes-nmstate perspectives safest is to just check NNS.

/lgtm
/approve

@kubevirt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@kubevirt-bot kubevirt-bot merged commit a81753d into nmstate:main Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/enhancement lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants