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

[WIP] Log missing mapped ports instead of failing #3637

Closed
wants to merge 1 commit into from

Conversation

schmichael
Copy link
Member

SDNs, overlays, etc often won't need to map ports.

SDNs, overlays, etc often won't need to map ports.
@schmichael schmichael force-pushed the f-rkt-port-map-nonfatal branch from c54f1f5 to 7dbf0c4 Compare December 9, 2017 00:23
@schmichael schmichael changed the title Log missing mapped ports instead of failing [WIP] Log missing mapped ports instead of failing Dec 9, 2017
@schmichael
Copy link
Member Author

Actually, let's hold off on this PR for a minute.

@ashald would you mind weighing in on this? I'm having trouble understanding why it's necessary, but maybe I'm misunderstanding your comment on #3615 (comment)

Currently for non-host networking in rkt we make sure that any ports the task reserves in a resource { network { ... } } stanza has a port map in the rkt config. I'm not sure why you would define a port in a resource stanza and not map it. That seems like a user error that's good to catch.

If you're using a network plugin that doesn't require a port map, you shouldn't need to define the ports in the resources stanza either correct?

If so I think this PR should be closed as it would remove a useful check for an easy misconfiguration.

@ashald
Copy link

ashald commented Dec 9, 2017

I'm not sure why you would define a port in a resource stanza and not map it. That seems like a user error that's good to catch.

That is often the case (that's why I proposed logging a warning about that) but not always. When non-host CNI network is used, it's quite possible to have a configuration which essentially will give the container an IP address on the same network as the host (ipvlan/macvlan). In this way there is not need for a port mapping and rather I'm not sure it's even possible as, as far as I understand, the touring happens on lower levels (l2/l3) so iptables won't see traffic at all. Still, even when you do that you want to be able to announce services in Consul. And that's the root cause of some problem that you meet here - you cannot announce service in Consul with Nomad without defining a port in a network stanza.

You indeed right that there is actually a side effect of this - as in when container gets some port on its unique "public" IP address we actually don't want to consider that port to be "claimed" on the scheduler side. So we probably want to account ports with the relation to the network used. This complicates things a bit on one hand but on the other I believe it's better to have this implemented in a way how this PR does it rather than how it was done before. Yes, indeed, we actually "over-provisioning" ports and we can optimize on that but at least now we can use some of the network types that was impossible to use before.

Does this help?

@schmichael
Copy link
Member Author

@ashald Does #3619 address this by allowing you to specify literal port numbers in service.port and check.port when address_mode=driver is used? The goal there is that Nomad (both the client and scheduler) don't need to have any knowledge of the ports being used when using an SDN/overlay/etc?

@ashald
Copy link

ashald commented Dec 11, 2017

Yeah, that sounds good.

@schmichael
Copy link
Member Author

Thanks @ashald! Closing this and cutting 0.7.1rc!

@schmichael schmichael closed this Dec 11, 2017
@schmichael schmichael deleted the f-rkt-port-map-nonfatal branch December 11, 2017 20:56
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants