Delegated Azure IPAM #1432
Delegated Azure IPAM #1432
Conversation
nddq
added
work-in-progress
cns
|
@wedaly fyi |
azure-ipam/cnsutils.go
Outdated
| @@ -0,0 +1,108 @@ | |||
| package main | |||
There was a problem hiding this comment.
can directory be named "delegated-ipam" to avoid confusion with v1 ipam?
There was a problem hiding this comment.
would swift-ipam works? I think the name got thrown around in meetings :)
There was a problem hiding this comment.
lets keep it delegated-ipam.. if we want to merge v1ipam tomorrow, then it wouldn't make sense to have swift-ipam
There was a problem hiding this comment.
I'll suggest again that if we're going to converge to a single IPAM, it should just be azure-ipam.
There was a problem hiding this comment.
we already have ipam directory in our repo..having azure-ipam directory would be confusing/conflicting.. i dont think we gonna remove ipam directory in near future... also delegated-ipam makes sense to me since this ipam delegating to cns to allocate/release ip and not involved in ip address management.
wedaly
left a comment
wedaly
left a comment
There was a problem hiding this comment.
all my comments have been addressed, thanks! please check whether you need / should remove the go.mod in azure-ipam
| @@ -0,0 +1,82 @@ | |||
| module github.com/Azure/azure-container-networking/azure-ipam | |||
There was a problem hiding this comment.
Could you try running a go mod tidy from within the azure-ipam directory? When I tried it, I got this error:
go: finding module for package github.com/containernetworking/cni/pkg/types/current
github.com/Azure/azure-container-networking/azure-ipam imports
github.com/Azure/azure-container-networking/cns/client imports
github.com/Azure/azure-container-networking/cns/restserver imports
github.com/Azure/azure-container-networking/cns/ipamclient imports
github.com/Azure/azure-container-networking/ipam tested by
github.com/Azure/azure-container-networking/ipam.test imports
github.com/containernetworking/cni/pkg/types/current: module github.com/containernetworking/cni@latest found (v1.1.1), but does not contain package github.com/containernetworking/cni/pkg/types/current
Actually, do we even need azure-ipam to be a go module? It seems like there's already a go.mod at the repository root.
There was a problem hiding this comment.
I want it to be a separate module; I've been trying to move the functionally distinct binaries in this monorepo to modules for a while so that they can be released and versioned separately (it's not super clear from reading the mod proposal, but you can tag path/v#.#.# and go understands that as the submodule version).
As for your build error - multimodule workspaces are new in go1.18 and with nested modules require that you have a go.work in the root module. (there is a make workspace target that should set this up but I wouldn't rely on it yet)
azure-ipam/ipam_test.go
Outdated
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| fmt.Printf("test : %v\n", tt.name) |
There was a problem hiding this comment.
nit: the Go test framework should print the (sub)test name for you, I think? (Same for the other tests in this file)
There was a problem hiding this comment.
@nddq tests shouldn't print directly to stdout, they should just pass or fail with message - testify's assert and require functions have an optional message parameter at the end. Parallel tests all printing to stdout just make a mess 🥲
4 participants
This is the draft PR for the delegated Azure IPAM