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

Support interop with Azure CNI IPAM plugin. #626

Merged
merged 6 commits into from Oct 31, 2018

Conversation

Projects
None yet
2 participants
@caseydavenport
Copy link
Member

caseydavenport commented Oct 24, 2018

Description

Add support for Azure IPAM, which requires a few things:

  • that we pass in the allocated subnet from the previous call.
  • on delete, also pass in any IP addresses associated with the container.

In order to do that, we need to store information about azure endpoints as well as networks so that we can provide that information to the IPAM plugin when we call it.

This PR implements that by writing configuration to disk in /var/run/calico/azure/<network>/

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Calico CNI support for the azure-vnet-ipam plugin

@caseydavenport caseydavenport force-pushed the caseydavenport:azure-ipam-support branch from aa100c0 to df1265e Oct 24, 2018

@caseydavenport caseydavenport requested a review from fasaxc Oct 24, 2018

@fasaxc
Copy link
Member

fasaxc left a comment

Just a few nits (and would be nice to have some tests!)

Would also be nice to break up the god function at some point too but that might make conflicts with my team's work if we do it right now.

k8s/k8s.go Outdated
@@ -253,6 +258,17 @@ func CmdAddK8s(ctx context.Context, args *skel.CmdArgs, conf types.NetConf, epID
ipAddrsNoIpam := annot["cni.projectcalico.org/ipAddrsNoIpam"]
ipAddrs := annot["cni.projectcalico.org/ipAddrs"]

if conf.IPAM.Type == "azure-vnet-ipam" {
// When using the Azure IPAM plugin, we need to pass it a subnet for all calls
// beyond the first one. This call updates updates the CNI configuration to meet

This comment has been minimized.

@fasaxc

fasaxc Oct 25, 2018

Member

Typo: updates x 2

k8s/k8s.go Outdated
@@ -477,6 +505,17 @@ func CmdDelK8s(ctx context.Context, c calicoclient.Interface, epIDs utils.WEPIde
}
}

if conf.IPAM.Type == "azure-vnet-ipam" {
// When using the Azure IPAM plugin, we need to pass it a subnet for all calls
// beyond the first one. This call updates updates the CNI configuration to meet

This comment has been minimized.

@fasaxc

fasaxc Oct 25, 2018

Member

Typo: updates x 2

Show resolved Hide resolved k8s/k8s.go
k8s/k8s.go Outdated
@@ -828,3 +867,30 @@ func getPodCidr(client *kubernetes.Clientset, conf types.NetConf, nodename strin
}
return node.Spec.PodCIDR, nil
}

// handleAzureIPAM mutates the given args to include the Azure IPAM subnet, if appropriate.
func handleAzureIPAM(logger *logrus.Entry, args *skel.CmdArgs, filename string) error {

This comment has been minimized.

@fasaxc

fasaxc Oct 25, 2018

Member

handle always seems like a cop out :-P. How about adjustAzureIPAMConfig or fixUpAzureIPAMConfig or something like that?

caseydavenport added some commits Oct 25, 2018

@caseydavenport caseydavenport force-pushed the caseydavenport:azure-ipam-support branch from b3f3c12 to 398af92 Oct 26, 2018

logger.Info("Cleaning up IP allocations for failed ADD")
if err := os.Setenv("CNI_COMMAND", "DEL"); err != nil {
// Failed to set CNI_COMMAND to DEL.
logger.Warning("Failed to set CNI_COMMAND=DEL")
} else {
if err := ipam.ExecDel(ipamType, stdinData); err != nil {
if err := DeleteIPAM(conf, args, logger); err != nil {

This comment has been minimized.

@caseydavenport

caseydavenport Oct 26, 2018

Member

I noticed that in the cleanup code, we weren't actually executing the plugin-specific logic that's needed. So, I've modified this to call through to the DeleteIPAM code instead of calling ExecDel directly. This way, we execute the plugin-specific logic every time we release an IP address (whether it's because its a DEL call, or a failed ADD).

@fasaxc
Copy link
Member

fasaxc left a comment

Looks like it should work and thanks for the incremental cleanups(!) but I'm a bit wary of all this file writing; a lot can go wrong with writing files and it's quite different to our tried-and-true approach of using a highly-consistent central datastore for our state.

Where does the Azure IPAM plugin store its state; if it stores on disk too then this approach would be a nice fit. OTOH, if it stores its state in k8s CRDs.... (I suspect the answer is neither?)

You probably should at least switch to something like this: https://github.com/natefinch/atomic, and in future, you'll need a file lock to prevent races when multiple CNI plugins try to add different subnets to the file.

if err != nil {
return err
}
if err := ioutil.WriteFile(ae.filename(), bytes, 0600); err != nil {

This comment has been minimized.

@fasaxc

fasaxc Oct 26, 2018

Member

Does WriteFile auto-create the dir?

This comment has been minimized.

@fasaxc

fasaxc Oct 26, 2018

Member

ah, looks like it gets created elsewhere

"github.com/sirupsen/logrus"
)

var networksDir string = "/var/run/calico/azure/networks/"

This comment has been minimized.

@fasaxc

fasaxc Oct 26, 2018

Member

Is this information needed over reboot? /var/run is cleared over reboot.

@fasaxc

This comment has been minimized.

Copy link
Member

fasaxc commented Oct 29, 2018

Another thought: if it is intentional that all the files get lost over reboot, maybe that's enough to address my concerns:

  • if the node dies; problem solved
  • otherwise, node can be rebooted to recreate all the state?
@caseydavenport

This comment has been minimized.

Copy link
Member

caseydavenport commented Oct 30, 2018

Where does the Azure IPAM plugin store its state; if it stores on disk too then this approach would be a nice fit. OTOH, if it stores its state in k8s CRDs.... (I suspect the answer is neither?)

Yup, the Azure IPAM plugin stores its state to disk as well in the /var/run directory, which is why I've done the same for this PR and thought it was a safe bet :)

From my own experimentation, it seems rebooting the node clears all IP allocations in the Azure IPAM plugin. The IPAM plugin keeps a local pool of IPs and manages them locally rather than relying on allocations being stored centrally.

@fasaxc

This comment has been minimized.

Copy link
Member

fasaxc commented Oct 30, 2018

OK, that sounds fine then.

@caseydavenport caseydavenport merged commit cda66d6 into projectcalico:master Oct 31, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@caseydavenport caseydavenport deleted the caseydavenport:azure-ipam-support branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment