Skip to content

fix initialize from kube pods flow #1194

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 31, 2022
Merged

fix initialize from kube pods flow #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c82cdc
fix initialize from kube pods flow
rbtr Jan 12, 2022
673a4ee
move list pods inside closure
rbtr Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion cns/NetworkContainerContract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ package cns

import (
"encoding/json"
"errors"
"fmt"
"net"
"strconv"
"strings"

"github.com/Azure/azure-container-networking/cns/types"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
)

// Container Network Service DNC Contract
Expand Down Expand Up @@ -254,6 +255,19 @@ func NewPodInfoFromIPConfigRequest(req IPConfigRequest) (PodInfo, error) {
return p, nil
}

func KubePodsToPodInfoByIP(pods []corev1.Pod) (map[string]PodInfo, error) {
podInfoByIP := map[string]PodInfo{}
for i := range pods {
if !pods[i].Spec.HostNetwork {
if _, ok := podInfoByIP[pods[i].Status.PodIP]; ok {
return nil, errors.Wrap(ErrDuplicateIP, pods[i].Status.PodIP)
}
podInfoByIP[pods[i].Status.PodIP] = NewPodInfo("", "", pods[i].Name, pods[i].Namespace)
}
}
return podInfoByIP, nil
}

// MultiTenancyInfo contains encap type and id.
type MultiTenancyInfo struct {
EncapType string
Expand Down
53 changes: 33 additions & 20 deletions cns/service/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,12 +572,7 @@ func main() {
cns.GlobalPodInfoScheme = cns.InterfaceIDPodInfoScheme
}
}
if cnsconfig.InitializeFromCNI {
logger.Printf("Initializing from CNI")
} else {
logger.Printf("Initializing from Kubernetes")
}
logger.Printf("Set GlobalPodInfoScheme %v", cns.GlobalPodInfoScheme)
logger.Printf("Set GlobalPodInfoScheme %v (InitializeFromCNI=%t)", cns.GlobalPodInfoScheme, cnsconfig.InitializeFromCNI)

err = InitializeCRDState(rootCtx, httpRestService, cnsconfig)
if err != nil {
Expand Down Expand Up @@ -826,7 +821,7 @@ type ncStateReconciler interface {

// TODO(rbtr) where should this live??
// InitCNS initializes cns by passing pods and a createnetworkcontainerrequest
func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncStateReconciler) error {
func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncStateReconciler, podInfoByIPProvider cns.PodInfoByIPProvider) error {
// Get nnc using direct client
nnc, err := cli.Get(ctx)
if err != nil {
Expand Down Expand Up @@ -856,13 +851,7 @@ func initCNS(ctx context.Context, cli nodeNetworkConfigGetter, ncReconciler ncSt
if err != nil {
return errors.Wrap(err, "failed to convert NNC status to network container request")
}

// rebuild CNS state from CNI
logger.Printf("initializing CNS from CNI")
podInfoByIPProvider, err := cnireconciler.NewCNIPodInfoProvider()
if err != nil {
return errors.Wrap(err, "failed to create CNI PodInfoProvider")
}
// rebuild CNS state
podInfoByIP, err := podInfoByIPProvider.PodInfoByIP()
if err != nil {
return errors.Wrap(err, "provider failed to provide PodInfoByIP")
Expand Down Expand Up @@ -921,11 +910,40 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn
}
}()

clientset, err := kubernetes.NewForConfig(kubeConfig)
Copy link
Member

@neaggarwMS neaggarwMS Jan 14, 2022

Choose a reason for hiding this comment

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

I still suggest to move this logic inside initCNS. It is more contained there as part of CNS initialization workflow.

Copy link
Member

@neaggarwMS neaggarwMS Jan 14, 2022

Choose a reason for hiding this comment

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

podInfoByProvider is kind of an internal state which we are passing to InitCNS func.

Copy link
Collaborator Author

@rbtr rbtr Jan 19, 2022

Choose a reason for hiding this comment

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

moving the clientset/the rest of this initialization into the initCNS breaks IOC - we should provide that func all of the objects it needs as inputs. passing it ready-to-use implementations of the interfaces it needs is the better pattern

if err != nil {
return errors.Wrap(err, "failed to build clientset")
}

var podInfoByIPProvider cns.PodInfoByIPProvider
if cnsconfig.InitializeFromCNI {
logger.Printf("Initializing from CNI")
podInfoByIPProvider, err = cnireconciler.NewCNIPodInfoProvider()
if err != nil {
return errors.Wrap(err, "failed to create CNI PodInfoProvider")
}
} else {
logger.Printf("Initializing from Kubernetes")
podInfoByIPProvider = cns.PodInfoByIPProviderFunc(func() (map[string]cns.PodInfo, error) {
pods, err := clientset.CoreV1().Pods("").List(ctx, metav1.ListOptions{ //nolint:govet // ignore err shadow
Copy link
Member

@matmerr matmerr Jan 22, 2022

Choose a reason for hiding this comment

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

I think I'm okay with the contents of this pr, but I do want to mention that hostnetworking pods on windows do require a hacky kubeconfig to be constructed externally and loaded via file as opposed to using an inclusterconfig, if running in hostnw on windows becomes a thing

Copy link
Collaborator Author

@rbtr rbtr Jan 22, 2022

Choose a reason for hiding this comment

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

good to know, we would need to update how we source the kubeconfigs in a couple of places for that

FieldSelector: "spec.nodeName=" + nodeName,
})
if err != nil {
return nil, errors.Wrap(err, "failed to list Pods for PodInfoProvider")
}
podInfo, err := cns.KubePodsToPodInfoByIP(pods.Items)
if err != nil {
return nil, errors.Wrap(err, "failed to convert Pods to PodInfoByIP")
}
return podInfo, nil
})
}

// apiserver nnc might not be registered or api server might be down and crashloop backof puts us outside of 5-10 minutes we have for
// aks addons to come up so retry a bit more aggresively here.
// will retry 10 times maxing out at a minute taking about 8 minutes before it gives up.
err = retry.Do(func() error {
err = initCNS(ctx, scopedcli, httpRestServiceImplementation)
err = initCNS(ctx, scopedcli, httpRestServiceImplementation, podInfoByIPProvider)
if err != nil {
logger.Errorf("[Azure CNS] Failed to init cns with err: %v", err)
}
Expand Down Expand Up @@ -957,11 +975,6 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn
return errors.Wrap(err, "failed to create manager")
}

clientset, err := kubernetes.NewForConfig(kubeConfig)
if err != nil {
return errors.Wrap(err, "failed to build clientset")
}

// get our Node so that we can xref it against the NodeNetworkConfig's to make sure that the
// NNC is not stale and represents the Node we're running on.
node, err := clientset.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})
Expand Down