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

state: support state:ignore #1298

Merged
merged 1 commit into from Sep 17, 2020
Merged

state: support state:ignore #1298

merged 1 commit into from Sep 17, 2020

Conversation

cathay4t
Copy link
Member

Introducing InterfaceState.IGNORE for:
* The ignored interface will not be changed when applying desire state.
* The state verification will not be impacted by ignored interface.

Test cases added.

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/nmstate-nmstate-1298
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@codecov
Copy link

codecov bot commented Aug 25, 2020

Codecov Report

Merging #1298 into base will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             base    #1298      +/-   ##
==========================================
+ Coverage   57.91%   58.10%   +0.18%     
==========================================
  Files          67       67              
  Lines        5316     5332      +16     
==========================================
+ Hits         3079     3098      +19     
+ Misses       2237     2234       -3     
Impacted Files Coverage Δ
libnmstate/nm/profile.py 25.13% <ø> (ø)
libnmstate/ifaces/base_iface.py 96.84% <100.00%> (+0.02%) ⬆️
libnmstate/ifaces/ifaces.py 90.90% <100.00%> (+1.74%) ⬆️
libnmstate/schema.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83fd948...146b6e3. Read the comment docs.

@ffmancera
Copy link
Member

I am fine with the current approach but I feel like it could be confusing to use state to mark the interface as ignore. Because ignore it is not a state itself. Maybe it would be better to add a new parameter that could be mark:ignore or something like that, indeed that could be useful to mark the interfaces in the future, what do you think?

@ffmancera ffmancera added Wait_on_submitter The submitter needs to do something before this can move on and removed Wait_Review labels Sep 1, 2020
@EdDev
Copy link
Member

EdDev commented Sep 1, 2020

I am fine with the current approach but I feel like it could be confusing to use state to mark the interface as ignore. Because ignore it is not a state itself. Maybe it would be better to add a new parameter that could be mark:ignore or something like that, indeed that could be useful to mark the interfaces in the future, what do you think?

Adding another field is odd. I think that when applying, "ignore" means... "do not do UP, DOWN or DISABLE" which translates to not touching it. So it seems ok to me.

When reporting, it will never be IGNORE, right? This merits a comment in the commit, on the API and in documentation.

@cathay4t
Copy link
Member Author

cathay4t commented Sep 1, 2020

I am fine with the current approach but I feel like it could be confusing to use state to mark the interface as ignore. Because ignore it is not a state itself. Maybe it would be better to add a new parameter that could be mark:ignore or something like that, indeed that could be useful to mark the interfaces in the future, what do you think?

Adding another field is odd. I think that when applying, "ignore" means... "do not do UP, DOWN or DISABLE" which translates to not touching it. So it seems ok to me.

When reporting, it will never be IGNORE, right? This merits a comment in the commit, on the API and in documentation.

The ignore is only used when applying desire state.
The use case is RHV/oVirt is expecting nmstate/NM don't touch the TUN/TAP interface managed by libvirt.

Currently, nmstate don't touch them as they are unknown to us. But I think the correct way is mark them as ignored in desire state.

@cathay4t cathay4t added Work_Ongoing Not ready to be merged and removed Wait_on_submitter The submitter needs to do something before this can move on labels Sep 1, 2020
@cathay4t cathay4t added Wait_Review and removed Work_Ongoing Not ready to be merged labels Sep 1, 2020
@cathay4t cathay4t force-pushed the state_ignore branch 2 times, most recently from 2d813a7 to 433a102 Compare September 2, 2020 03:22
Copy link
Member

@ffmancera ffmancera left a comment

Choose a reason for hiding this comment

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

As everyone agrees on introducing state: ignore I will not block on this. Approved, thanks!

@cathay4t
Copy link
Member Author

cathay4t commented Sep 2, 2020

QE approval request sent. Please wait test feedback before merge it.

@cathay4t cathay4t force-pushed the state_ignore branch 3 times, most recently from 90377c6 to b9d4074 Compare September 8, 2020 06:33
@smyle10
Copy link
Collaborator

smyle10 commented Sep 8, 2020

Applied 'state: ignore' to main port and subordinate port respectively, it works well.

Introducing `InterfaceState.IGNORE` for:
    * The ignored interface will not be changed when applying desire state.
    * The state verification will not be impacted by ignored interface.

This state will be only used for `libnmstate.apply()`.

Test cases added.

Signed-off-by: Gris Ge <fge@redhat.com>
@cathay4t
Copy link
Member Author

Timeout on waiting comsumer QE. Merging.

@cathay4t cathay4t merged commit dc13851 into nmstate:base Sep 17, 2020
@cathay4t cathay4t deleted the state_ignore branch September 17, 2020 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants