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

feat: dummy linux interface and existing IPs #1765

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

milanlenco
Copy link
Collaborator

@milanlenco milanlenco commented Dec 1, 2020

This PR adds two new features in the context of the linux interface plugin:

  1. Support for Dummy interfaces: they are effectively additional loopbacks. Currently we have "LOOPBACK" type, but it acts exactly as EXISTING with host_name="lo". In other words it attaches to the always-present "lo" interface. Since the same can be accomplished using EXISTING type, I decided to mark "LOOPBACK" as deprecated (but left the implementation intact) and explained in the comment what alternatives are recommended to use.
    Edit: later I realized that LOOPBACK is not actually just a subset of EXISTING. Unlike EXISTING it allows to attach to "lo" interface in any namespace. EXISTING is currently supported only for the default namespace because the interface watcher does not watch other namespaces. LOOPBACK is not affected by this limitation because it actually does not depend on anything and assumes that "lo" interface always exists in every namespace. So until EXISTING with non-default namespace is supported, LOOPBACK must remain non-deprecated. I will remove the deprecated flag and update the comment shortly...[DONE]

  2. Support for "existing" IPs. Currently we allow to attach to an externally created interface and let the agent to configure IP addresses and other parameters, i.e. everything except the link. This is possible using the EXISTING interface type. Later, for different use-cases we added link_only attribute, which is effectively the opposite -- if link_only=true then agent creates the interface but IP addresses are managed externally. Combining EXISTING with link_only=true had the effect of agent waiting for an interface but never touching it. So it had no effect if you set ip_addresses field or not -- IP addresses were not even derived in this case and therefore were not represented in the KV graph. But routes and other L3 features depend on interface having an IP address (any or a specific one), therefore it was not possible to attach them to interface that is completely (link+IPs) managed externally. What this PR does, is that if type=EXISTING, link_only=true and some IP addresses are defined, then each IP will be derived in KV graph, but Create/Delete of those derived KVs will be NOOP and will depend on those IP addresses to appear on the interface (by an external action). So in other words it is possible to reference existing IPs just like we can currently reference existing interfaces.

Signed-off-by: Milan Lenco [email protected]

Copy link
Collaborator

@fgschwan fgschwan left a comment

Choose a reason for hiding this comment

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

Just one note. Dummy interfaces are not loopback interfaces. You might get tempted to interchange them (and it also might work in most cases), but there are subtle semantic differences that can bite you later. I would prefer to keep them separated. The linux keeps them separated (maybe for compatibility reasons, but maybe also for different assumptions that are hidden), so lets keep it separate too.

@milanlenco milanlenco added the 🚧 WIP do not merge! work in progress! label Dec 3, 2020
@milanlenco
Copy link
Collaborator Author

I will add some further improvements into this PR and simplify some code, so please do not merge yet.

@fgschwan
Copy link
Collaborator

fgschwan commented Dec 3, 2020

I will add some further improvements into this PR and simplify some code, so please do not merge yet.

@milanlenco You can insert "WIP" into the PR title and the WIP checker will block merging. This is done for example in #1658. After you are done, just remove it and it can be merged again.

@milanlenco milanlenco changed the title feat: dummy linux interface and existing IPs WIP: feat: dummy linux interface and existing IPs Dec 3, 2020
@milanlenco milanlenco changed the title WIP: feat: dummy linux interface and existing IPs feat: dummy linux interface and existing IPs Dec 3, 2020
@milanlenco milanlenco removed the 🚧 WIP do not merge! work in progress! label Dec 3, 2020
@milanlenco
Copy link
Collaborator Author

Actually I reconsidered and I think it is better if this is merged as is because the follow up commit is large enough and deserves its own PR.
So if there are no objections please merge...

@ondrej-fabry
Copy link
Member

ondrej-fabry commented Dec 4, 2020

@milanlenco If you did not yet, please verify that e2e/integration tests are passing for all VPP versions, because we no longer have Travis running for PRs (they changed their bussiness model, limitting max number of minutes per month..).

Run these commands to test all versions:

make e2e-tests integration-tests VPP_VERSION=2009
make e2e-tests integration-tests VPP_VERSION=2005
make e2e-tests integration-tests VPP_VERSION=2001

@milanlenco
Copy link
Collaborator Author

Verified that PR does not break any e2e tests for any of the supported VPP versions.

Please note, however, that TestInterfaceTapTunnel is and has been already failing (for 20.09). Looking at the test there seems to be lots of commented out code, so I'm not sure what is the state of this feature. It appears that dump doesn't work properly and the Tap link section is not retrieved which triggers re-create and the delete fails with failed to remove interface vpp-taptun, index 1: invalid tap version (0) (it is because 0 was incorrectly retrieved as the TAP version).

@ondrej-fabry ondrej-fabry merged commit e5edff9 into ligato:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants