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

multi-interface network support #8208

Merged
merged 6 commits into from
Jun 19, 2020
Merged

multi-interface network support #8208

merged 6 commits into from
Jun 19, 2020

Conversation

nickethier
Copy link
Member

No description provided.

@nickethier nickethier added this to the 0.12.0-beta1 milestone Jun 19, 2020
@nickethier nickethier marked this pull request as ready for review June 19, 2020 18:40
@nickethier nickethier requested review from schmichael and shoenig June 19, 2020 18:40
})
f.logger.Debug("detected CNI network", "name", name)
}

resp.NodeResources = &structs.NodeResources{
Networks: nodeNetworks,
Networks: nodeNetworks,
NodeNetworks: newNodeNetworks,
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe rename nodeNetworks to something else so we aren't cross-naming fields here


if len(alloc.AllocatedResources.Shared.Ports) == 0 && len(alloc.AllocatedResources.Shared.Networks) > 0 {
for _, network := range alloc.AllocatedResources.Shared.Networks {
for _, port := range append(network.DynamicPorts, network.ReservedPorts...) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is safe - append may or may not re-allocate the underlying array of the first slice which gets confusing if it's also being indexed elsewhere (at which point each slice pointer is pointing to different arrays even though they were supposed to always be the same).
Very unlikely to be a problem since I don't think we modify these after they're created, but I'd rather error on safety by creating a copy first

@@ -197,6 +198,8 @@ func TestNetworkFingerprint_basic(t *testing.T) {
t.Fatalf("err: %v", err)
}

spew.Dump(response)
os.Exit(0)
Copy link
Member

Choose a reason for hiding this comment

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

some leftover debugging?

@@ -10,14 +10,15 @@ import (
)

func TestNetworkIndex_Overcommitted(t *testing.T) {
t.Skip()
Copy link
Member

Choose a reason for hiding this comment

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

maybe a leave a deprecated note in the Skip

@shoenig
Copy link
Member

shoenig commented Jun 19, 2020

This looks great, @nickethier !

Seems like we might want to come up with a test matrix for the upgrade path, that we can at least go through manually before GA. It's hard for me to think about exactly what's going to happen between the old and new structs

Comment on lines +201 to +202
spew.Dump(response)
os.Exit(0)
Copy link
Member

Choose a reason for hiding this comment

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

Left in some debugging code

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

Let's get this merged and fix it up in follow ups. Public structs, fields, and funcs need comments too.

I think the system scheduler is the only significant bug I noticed.

@@ -70,15 +70,21 @@ const (
// The ip:port are always the host's.
AddrPrefix = "NOMAD_ADDR_"

HostAddrPrefix = "NOMAD_HOST_ADDR_"
Copy link
Member

Choose a reason for hiding this comment

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

All of these need comments.

@@ -36,6 +36,7 @@ require (
github.com/coreos/go-semver v0.3.0
github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f // indirect
github.com/cyphar/filepath-securejoin v0.2.3-0.20190205144030-7efe413b52e1 // indirect
github.com/davecgh/go-spew v1.1.1
Copy link
Member

Choose a reason for hiding this comment

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

Also due to debugging code above

Suggested change
github.com/davecgh/go-spew v1.1.1

@@ -226,6 +261,30 @@ func (idx *NetworkIndex) AddReservedPortRange(ports string) (collide bool) {
return
}

// AddReservedPortsForIP
Copy link
Member

Choose a reason for hiding this comment

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

Finish + comment other funcs

@@ -10,14 +10,15 @@ import (
)

func TestNetworkIndex_Overcommitted(t *testing.T) {
t.Skip()
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

@@ -500,6 +500,7 @@ func (s *GenericScheduler) computePlacements(destructive, place []placementResul
}
if option.AllocResources != nil {
resources.Shared.Networks = option.AllocResources.Networks
resources.Shared.Ports = option.AllocResources.Ports
Copy link
Member

Choose a reason for hiding this comment

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

Need to make this change in the system scheduler too

@github-actions
Copy link

github-actions bot commented Jan 2, 2023

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 Jan 2, 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.

3 participants