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.

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

Make podRef to be unique #433

Closed
wants to merge 3 commits into from
Closed

Make podRef to be unique #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9e55dd
Support running ip reconciler with KUBECONFIG
pliurh Feb 29, 2024
7cb8a7c
Make podRef to be unique
pliurh Mar 4, 2024
8402e6b
User podRef as indentifier in IP allocation/deallocation
pliurh Mar 11, 2024
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
26 changes: 24 additions & 2 deletions cmd/controlloop/controlloop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"k8s.io/client-go/kubernetes/scheme"
typedcorev1 "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/tools/record"

nadclient "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/clientset/versioned"
Expand All @@ -38,6 +39,7 @@ const (
cronSchedulerCreationError
fileWatcherError
couldNotCreateConfigWatcherError
initialReconsileFailed
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: initialReconsileFailed -> initialReconcileFailed

)

const (
Expand Down Expand Up @@ -78,6 +80,16 @@ func main() {
}
defer watcher.Close()

// trigger one immediate reconcile before cron job start
go reconciler.ReconcileIPs(errorChan)
err = <-errorChan
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

if err = <-errorChan; err != nil {
	logging.Verbosef("initial reconciler failure: %s", err)
	os.Exit(initialReconcileFailed)
}
logging.Verbosef("initial reconciler success")

(see https://go.dev/play/p/ltsR09kXKW9)

logging.Verbosef("initial reconciler success")
} else {
logging.Verbosef("initial reconciler failure: %s", err)
os.Exit(initialReconsileFailed)
}

reconcilerConfigWatcher, err := reconciler.NewConfigWatcher(
reconcilerCronConfiguration,
s,
Expand Down Expand Up @@ -125,9 +137,19 @@ func handleSignals(stopChannel chan struct{}, signals ...os.Signal) {
}

func newPodController(stopChannel chan struct{}) (*controlloop.PodController, error) {
cfg, err := rest.InClusterConfig()
var cfg *rest.Config
var err error
cfg, err = rest.InClusterConfig()
Copy link
Contributor

@andreaskaris andreaskaris Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If this is run inside the cluster, but the KUBECONFIG env variable is set:

  • ip.go will use NewReconcileLooperWithKubeconfig due to the if condition
  • newPodController will use rest.InClusterConfig because in cluster rest.InClusterConfig will succeed

So your code can end up using the in cluster config for one, and the kubeconfig for the other

if err != nil {
return nil, fmt.Errorf("failed to implicitly generate the kubeconfig: %w", err)
logging.Debugf("failed to generate the kubeconfig from service account: %v", err)
kubeConfigFile := os.Getenv("KUBECONFIG")
Copy link
Contributor

Choose a reason for hiding this comment

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

both newPodController and ReconcileIPs are exclusively called from controlloop.go:main().
I know that the operation doesn't cost anything, but it rubs me the wrong way to do kubeConfigFile := os.Getenv("KUBECONFIG") in each of them, the correct place imo is in controlloop.go:main() and then pass kubeConfigFile as an argument to both newPodController and ReconcileIPs . Perhaps that allows even for more optimizations

if kubeConfigFile == "" {
return nil, fmt.Errorf("KUBECONFIG environment variable not set")
}
cfg, err = clientcmd.BuildConfigFromFlags("", kubeConfigFile)
if err != nil {
return nil, fmt.Errorf("failed to generate kubeconfig from file %s: %v", kubeConfigFile, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the improved logging on error

}

k8sClientSet, err := kubernetes.NewForConfig(cfg)
Expand Down
28 changes: 17 additions & 11 deletions cmd/whereabouts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ func AllocateAndReleaseAddressesTest(ipVersion string, ipamConf *whereaboutstype
ifname string = "eth0"
nspath string = "/some/where"
cniVersion = "0.3.1"
podName = "dummyPOD"
podNamespace = "dummyNS"
)

Expand All @@ -62,12 +61,13 @@ func AllocateAndReleaseAddressesTest(ipVersion string, ipamConf *whereaboutstype
fakek8sclient.NewSimpleClientset(),
0)
for i := 0; i < len(expectedAddresses); i++ {
ipamConf.PodName = fmt.Sprintf("pod-%d", i)
args := &skel.CmdArgs{
ContainerID: fmt.Sprintf("dummy-%d", i),
Netns: nspath,
IfName: ifname,
StdinData: cniConf,
Args: cniArgs(podNamespace, podName),
Args: cniArgs(podNamespace, ipamConf.PodName),
}
client := mutateK8sIPAM(args.ContainerID, ipamConf, wbClient)

Expand Down Expand Up @@ -956,6 +956,8 @@ var _ = Describe("Whereabouts operations", func() {
var ipArgs []*skel.CmdArgs
// allocate 8 IPs (192.168.1.5 - 192.168.1.12); the entirety of the pool defined above
for i := 0; i < 8; i++ {
podName := fmt.Sprintf("pod-%d", i)
ipamConf.PodName = podName
args := &skel.CmdArgs{
ContainerID: fmt.Sprintf("dummy-%d", i),
Netns: nspath,
Expand All @@ -982,7 +984,7 @@ var _ = Describe("Whereabouts operations", func() {
}))
ipArgs = append(ipArgs, args)
}

ipamConf.PodName = podName
// assigning more IPs should result in error due to the defined range_start - range_end
args := &skel.CmdArgs{
ContainerID: "dummy-failure",
Expand Down Expand Up @@ -1014,6 +1016,8 @@ var _ = Describe("Whereabouts operations", func() {
It("detects IPv4 addresses used in other ranges, to allow for overlapping IP address ranges", func() {
const ifname string = "eth0"
const nspath string = "/some/where"
const podName1 string = "pod-1"
const podName2 string = "pod-2"

// ----------------------------- range 1

Expand All @@ -1037,12 +1041,12 @@ var _ = Describe("Whereabouts operations", func() {
Netns: nspath,
IfName: ifname,
StdinData: []byte(conf),
Args: cniArgs(podNamespace, podName),
Args: cniArgs(podNamespace, podName1),
}

confPath := filepath.Join(tmpDir, "whereabouts.conf")
Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed())
ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath)
ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName1), confPath)
Expect(err).NotTo(HaveOccurred())
Expect(ipamConf.IPRanges).NotTo(BeEmpty())
wbClient := *kubernetes.NewKubernetesClient(
Expand Down Expand Up @@ -1090,12 +1094,12 @@ var _ = Describe("Whereabouts operations", func() {
Netns: nspath,
IfName: ifname,
StdinData: []byte(confsecond),
Args: cniArgs(podNamespace, podName),
Args: cniArgs(podNamespace, podName2),
}

secondConfPath := filepath.Join(tmpDir, "whereabouts.conf")
Expect(os.WriteFile(confPath, []byte(confsecond), 0755)).To(Succeed())
secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), secondConfPath)
secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName2), secondConfPath)
Expect(err).NotTo(HaveOccurred())

// Allocate the IP
Expand Down Expand Up @@ -1134,6 +1138,8 @@ var _ = Describe("Whereabouts operations", func() {
It("detects IPv6 addresses used in other ranges, to allow for overlapping IP address ranges", func() {
const ifname string = "eth0"
const nspath string = "/some/where"
const podName1 string = "pod-1"
const podName2 string = "pod-2"

// ----------------------------- range 1

Expand All @@ -1157,12 +1163,12 @@ var _ = Describe("Whereabouts operations", func() {
Netns: nspath,
IfName: ifname,
StdinData: []byte(conf),
Args: cniArgs(podNamespace, podName),
Args: cniArgs(podNamespace, podName1),
}

confPath := filepath.Join(tmpDir, "whereabouts.conf")
Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed())
ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath)
ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName1), confPath)
Expect(err).NotTo(HaveOccurred())
Expect(ipamConf.IPRanges).NotTo(BeEmpty())
wbClient := *kubernetes.NewKubernetesClient(
Expand Down Expand Up @@ -1210,12 +1216,12 @@ var _ = Describe("Whereabouts operations", func() {
Netns: nspath,
IfName: ifname,
StdinData: []byte(confsecond),
Args: cniArgs(podNamespace, podName),
Args: cniArgs(podNamespace, podName2),
}

secondConfPath := filepath.Join(tmpDir, "whereabouts.conf")
Expect(os.WriteFile(confPath, []byte(confsecond), 0755)).To(Succeed())
secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), secondConfPath)
secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName2), secondConfPath)
Expect(err).NotTo(HaveOccurred())

// Allocate the IP
Expand Down
18 changes: 10 additions & 8 deletions e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,9 @@ var _ = Describe("Whereabouts functionality", func() {

Context("deleting a pod from the statefulset", func() {
var (
containerID string
podRef string
containerID string
podRef string
decomposedPodRef []string
)

BeforeEach(func() {
Expand All @@ -443,9 +444,11 @@ var _ = Describe("Whereabouts functionality", func() {
containerID = ipPool.Spec.Allocations["1"].ContainerID
podRef = ipPool.Spec.Allocations["1"].PodRef

decomposedPodRef := strings.Split(podRef, "/")
decomposedPodRef = strings.Split(podRef, ":")
Expect(decomposedPodRef).To(HaveLen(2))
podName := decomposedPodRef[1]
podNameNamespace := strings.Split(decomposedPodRef[0], "/")
Expect(podNameNamespace).To(HaveLen(2))
podName := podNameNamespace[1]

rightNow := int64(0)
Expect(clientInfo.Client.CoreV1().Pods(namespace).Delete(
Expand Down Expand Up @@ -476,8 +479,7 @@ var _ = Describe("Whereabouts functionality", func() {
metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(ipPool.Spec.Allocations).NotTo(BeEmpty())

Expect(allocationForPodRef(podRef, *ipPool).ContainerID).NotTo(Equal(containerID))
Expect(allocationForPodName(decomposedPodRef[0], *ipPool).ContainerID).NotTo(Equal(containerID))
})
})
})
Expand Down Expand Up @@ -669,9 +671,9 @@ var _ = Describe("Whereabouts functionality", func() {
})
})

func allocationForPodRef(podRef string, ipPool v1alpha1.IPPool) *v1alpha1.IPAllocation {
func allocationForPodName(podName string, ipPool v1alpha1.IPPool) *v1alpha1.IPAllocation {
for _, allocation := range ipPool.Spec.Allocations {
if allocation.PodRef == podRef {
if strings.Contains(allocation.PodRef, podName) {
return &allocation
}
}
Expand Down
24 changes: 16 additions & 8 deletions pkg/allocate/allocate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ func AssignIP(ipamConf types.RangeConfiguration, reservelist []types.IPReservati
}

// DeallocateIP assigns an IP using a range and a reserve list.
func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]types.IPReservation, net.IP, error) {
func DeallocateIP(reservelist []types.IPReservation, podRef string) ([]types.IPReservation, net.IP, error) {

updatedreservelist, hadip, err := IterateForDeallocation(reservelist, containerID, getMatchingIPReservationIndex)
updatedreservelist, hadip, err := IterateForDeallocation(reservelist, podRef, getMatchingIPReservationIndex)
if err != nil {
return nil, nil, err
}
Expand All @@ -52,13 +52,13 @@ func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]type
// IterateForDeallocation iterates overs currently reserved IPs and the deallocates given the container id.
func IterateForDeallocation(
reservelist []types.IPReservation,
containerID string,
podRef string,
matchingFunction func(reservation []types.IPReservation, id string) int) ([]types.IPReservation, net.IP, error) {

foundidx := matchingFunction(reservelist, containerID)
foundidx := matchingFunction(reservelist, podRef)
// Check if it's a valid index
if foundidx < 0 {
return reservelist, nil, fmt.Errorf("did not find reserved IP for container %v", containerID)
return reservelist, nil, fmt.Errorf("did not find reserved IP for container %v", podRef)
}

returnip := reservelist[foundidx].IP
Expand All @@ -70,7 +70,7 @@ func IterateForDeallocation(
func getMatchingIPReservationIndex(reservelist []types.IPReservation, id string) int {
foundidx := -1
for idx, v := range reservelist {
if v.ContainerID == id {
if v.PodRef == id {
foundidx = idx
break
}
Expand Down Expand Up @@ -102,6 +102,15 @@ func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, r
// Build reserved map.
reserved := make(map[string]bool)
for _, r := range reserveList {
if r.PodRef == podRef {
// If this IP has been reserved for this pod, return it.
if r.ContainerID != containerID {
// update the containerID if it has changed
r.ContainerID = containerID
}
logging.Debugf("Returning reserved IP: |%v|", r.IP.String()+" "+podRef)
return r.IP, reserveList, nil
}
reserved[r.IP.String()] = true
}

Expand All @@ -118,7 +127,6 @@ func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, r
// Iterate over every IP address in the range, accounting for reserved IPs and exclude ranges. Make sure that ip is
// within ipnet, and make sure that ip is smaller than lastIP.
for ip := firstIP; ipnet.Contains(ip) && iphelpers.CompareIPs(ip, lastIP) <= 0; ip = iphelpers.IncIP(ip) {
// If already reserved, skip it.
if reserved[ip.String()] {
continue
}
Expand All @@ -129,7 +137,7 @@ func IterateForAssignment(ipnet net.IPNet, rangeStart net.IP, rangeEnd net.IP, r
continue
}
// Assign and reserve the IP and return.
logging.Debugf("Reserving IP: |%v|", ip.String()+" "+containerID)
logging.Debugf("Reserving IP: |%v|", ip.String()+" "+podRef)
reserveList = append(reserveList, types.IPReservation{IP: ip, ContainerID: containerID, PodRef: podRef})
return ip, reserveList, nil
}
Expand Down
Loading