NetworkConfig CRD defintions #570

thatmattlong · 2020-05-28T17:11:31Z

What this PR does / why we need it:
This PR adds definitions for the NetworkConfig CRD that will be used to pass goal state to CNS

Special notes for your reviewer:
We need to confirm that only bufferSize is passed to CNS via the status, and no other options regarding the pool of allocated IP addresses. In other words, is CNS responsible for setting its own thresholds based on the buffer size, or should we also pass it a threshold?

codecov · 2020-05-28T17:18:34Z

Codecov Report

Merging #570 into master will increase coverage by 3.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
+ Coverage   49.22%   52.54%   +3.31%     
==========================================
  Files          28       23       -5     
  Lines        3437     2845     -592     
==========================================
- Hits         1692     1495     -197     
+ Misses       1455     1077     -378     
+ Partials      290      273      -17

networkconfig/api/v1/networkconfig_types.go

paulgmiller

Adding as a refernce for what I'm expecting.

networkconfig/api/v1/networkconfig_types.go

ramiro-gamarra

Some food for thought.

networkconfig/api/v1/networkconfig_types.go

ramiro-gamarra · 2020-05-29T15:58:02Z

networkconfig/api/v1/networkconfig_types.go

+// Important: Run "make" to regenerate code after modifying this file
+
+// AvailableIPMapping groups an IP address and Name. This is a struct and not a Map for future extensibility
+type AvailableIPMapping struct {


Perhaps this doesn't need to be qualified as "available" or a "mapping"? It could just be an IP struct. The availability is signaled by its inclusion in AvailableIPs below. Thinking a bit more about it, what about DelegatedIP or something like that? I feel like mentioning its delegated might help explain why it has a UUID.

I don't want to call it an IP because this fundamentally represents an IP allocation, since an IP address could be allocated to this node, deallocated, then reallocated under a different "IP allocation". Similarly, deletions happen via the IP allocation's UUID, not IP. How about IPAllocation instead of AvailableIPMapping?

networkconfig/api/v1alpha/networkconfig_types.go

paulgmiller · 2020-05-29T23:26:48Z

nodenetworkconfig/api/v1alpha/nodenetworkconfig.go

+
+// NodeNetworkConfigStatus defines the observed state of NetworkConfig
+type NodeNetworkConfigStatus struct {
+	AssignedIPCount         int64              `json:"assignedIPCount,omitempty"`


the assigned up count here and in network container seem unnecessary can just len() IpAssignments. It's a slice not a stream :)

@neaggarwMS wanted these for easier debugging/ops. If you don't object @paulgmiller, I'll leave them in. If you do, we can discuss with Neha.

This is added for debugging purpose specially during devops. Lets discuss this in today's syncup

paulgmiller · 2020-05-29T23:27:30Z

nodenetworkconfig/api/v1alpha/nodenetworkconfig.go

+	BatchSize               int64              `json:"batchSize,omitempty"`
+	ReleaseThresholdPercent int64              `json:"releaseThresholdPercent,omitempty"`
+	RequestThresholdPercent int64              `json:"requestThresholdPercent,omitempty"`
+	NetworkContainers       []NetworkContainer `json:"networkContainers,omitempty"`


Can we give an example if why we'd have multiple network containers on a single node? i

When would a single node have different different primary ips and different subnets.

Right now we are planning one NC per node and thus one primary IP and one subnet. However, this imposes a limit on the number of IPs, so we want to support multiple NCs in the future. It will be easier to add multi-NC support when we can support it on the other tooling if the CRD already supports it.

paulgmiller · 2020-05-29T23:31:00Z

nodenetworkconfig/api/v1alpha/nodenetworkconfig.go

+	IPAssignments   []IPAssignment `json:"iPAssignments,omitempty"`
+	AssignedIPCount int64          `json:"assignedIPCount,omitempty"`
+	DefaultGateway  string         `json:"defaultGateway,omitempty"`
+	Netmask         string         `json:"netmask,omitempty"`


if this is a mask is the subnet an id? more comments explaining the relation ship between these might be nice. or group them into a struct subnet { id , mask }

An NC can have only one subnet. I'll make this subnetId.

matmerr · 2020-06-01T18:07:33Z

/azp run

azure-pipelines · 2020-06-01T18:07:43Z

Azure Pipelines successfully started running 1 pipeline(s).

thatmattlong · 2020-06-01T18:08:58Z

/azp run

azure-pipelines · 2020-06-01T18:09:08Z

Azure Pipelines successfully started running 1 pipeline(s).

neaggarwMS · 2020-06-01T19:11:30Z

nodenetworkconfig/api/v1alpha/nodenetworkconfig.go

+
+// NodeNetworkConfigStatus defines the observed state of NetworkConfig
+type NodeNetworkConfigStatus struct {
+	AssignedIPCount         int64              `json:"assignedIPCount,omitempty"`


This is added for debugging purpose specially during devops. Lets discuss this in today's syncup

neaggarwMS · 2020-06-01T19:12:56Z

nodenetworkconfig/api/v1alpha/nodenetworkconfig.go

+// NodeNetworkConfigStatus defines the observed state of NetworkConfig
+type NodeNetworkConfigStatus struct {
+	AssignedIPCount         int64              `json:"assignedIPCount,omitempty"`
+	BatchSize               int64              `json:"batchSize,omitempty"`


Can we move these batchSize, Release and Request threshold in some other struct for code readiblity? Also, In case we need to extend it, it wont change the main struct

thatmattlong · 2020-06-02T18:00:26Z

/azp run

azure-pipelines · 2020-06-02T18:00:37Z

Azure Pipelines successfully started running 1 pipeline(s).

This pull request adds a new binary, `requestcontroller` to watch the Nodes of a Kubernetes cluster so that it can create a [NodeNetworkConfig](Azure#570) CRD object for each Node on Node creation, and delete it on Node deletion. **To Do:** - [X] Update Makefile to build - [X] Add tests - [X] Discuss the use of [finalizers](https://book.kubebuilder.io/reference/using-finalizers.html). This PR uses them currently, but need to discuss with AKS team. - [ ] Add task to pipeline to build/test PR URL: https://msazure.visualstudio.com/DefaultCollection/One/_git/Networking-Aquarius/pullrequest/2980584 Related work items: #7207918, #7208644

NetworkConfig CRD defintions

0b52a89

thatmattlong requested a review from pjohnst5 May 28, 2020 17:12

paulgmiller reviewed May 28, 2020

View reviewed changes

networkconfig/api/v1/networkconfig_types.go Outdated Show resolved Hide resolved

paulgmiller reviewed May 28, 2020

View reviewed changes

networkconfig/api/v1/networkconfig_types.go Outdated Show resolved Hide resolved

paulgmiller reviewed May 28, 2020

View reviewed changes

networkconfig/api/v1/networkconfig_types.go Outdated Show resolved Hide resolved

paulgmiller suggested changes May 28, 2020

View reviewed changes

networkconfig/api/v1/networkconfig_types.go Outdated Show resolved Hide resolved

paulgmiller reviewed May 28, 2020

View reviewed changes

networkconfig/api/v1/networkconfig_types.go Outdated Show resolved Hide resolved

ramiro-gamarra reviewed May 29, 2020

View reviewed changes

v1 to v1alpha, release/request thresholds, naming

5f89f93

neaggarwMS requested changes May 29, 2020

View reviewed changes

networkconfig/api/v1alpha/networkconfig_types.go Outdated Show resolved Hide resolved

networkconfig/api/v1alpha/networkconfig_types.go Outdated Show resolved Hide resolved

networkconfig/api/v1alpha/networkconfig_types.go Outdated Show resolved Hide resolved

networkconfig to nodnetworkconfig, renaming vars

af4803e

thatmattlong requested review from neaggarwMS, paulgmiller and ramiro-gamarra May 29, 2020 20:21

neaggarwMS previously approved these changes May 29, 2020

View reviewed changes

paulgmiller reviewed May 29, 2020

View reviewed changes

subnet to subnetID, added comment for netmask

588e7e5

thatmattlong dismissed neaggarwMS’s stale review via 588e7e5 June 1, 2020 16:34

thatmattlong requested a review from neaggarwMS June 1, 2020 20:00

removed counts

ec58366

neaggarwMS approved these changes Jun 2, 2020

View reviewed changes

thatmattlong merged commit 9e15490 into Azure:master Jun 2, 2020

thatmattlong deleted the networkconfig branch June 2, 2020 22:14

NetworkConfig CRD defintions #570

NetworkConfig CRD defintions #570

Uh oh!

Conversation

thatmattlong commented May 28, 2020

Uh oh!

codecov bot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulgmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulgmiller May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matmerr commented Jun 1, 2020

Uh oh!

azure-pipelines bot commented Jun 1, 2020

Uh oh!

thatmattlong commented Jun 1, 2020

Uh oh!

azure-pipelines bot commented Jun 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatmattlong commented Jun 2, 2020

Uh oh!

azure-pipelines bot commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented May 28, 2020 •

edited

Loading

paulgmiller May 29, 2020 •

edited

Loading