feat: [NPM] Restructure code and add deploy manifests #1203

nitishm · 2022-01-21T22:08:31Z

Moved files from top level npm/ folder to their own respective folders
for controller and daemon components
Generated new deployment manifests for each components and moved into
the respective folders under npm/deploy

Signed-off-by: Nitish Malhotra nitishm@microsoft.com

Reason for Change:

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

- Moved files from top level npm/ folder to their own respective folders for controller and daemon components - Generated new deployment manifests for each components and moved into the respective folders under npm/deploy Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

nitishm · 2022-01-21T22:09:07Z

Checking this in for tracking. Will port to kustomize in follow up commits.

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

vakalapa · 2022-01-25T06:03:49Z

npm/deploy/npm/azure-npm.yaml

@@ -99,18 +99,12 @@ spec:
          volumeMounts:
          - name: xtables-lock
            mountPath: /run/xtables.lock
-          - name: log


We will need to update this new file location for all tests

@matmerr mentioned this is obsolete? Do the tests still use this?

impacts these:

azure-container-networking/.github/workflows/cyclonus-netpol-test.yaml

Line 53 in 9d8e61e

kubectl apply -f ./npm/azure-npm.yaml

azure-container-networking/.pipelines/npm/npm-conformance-tests.yaml

Line 167 in 9d8e61e

./kubectl --kubeconfig=./kubeconfig apply -f https://raw.githubusercontent.com/Azure/azure-container-networking/master/npm/azure-npm.yaml

huntergregory

This is awesome. We should consider better names for services. We are stuck with "npm-metrics-cluster-service" for vanilla npm and npm daemon for Prometheus backwards compatibility. Perhaps we should reserve the term "cluster-metrics" for this service only

huntergregory · 2022-01-27T21:52:12Z

npm/deploy/kustomize/base/configmap.yaml

@@ -0,0 +1,24 @@
+---
+apiVersion: v1
+kind: ConfigMap


perhaps a better location: npm/profiles/

or we could move profiles/ into deploy/ but there are dependencies on the profiles path

Let's move profiles/ to deploy/profiles/ to keep everything together. Can we move all configmap yamls to this folder?

After moving the profiles/ folder, we'll need to update these (similar to the ones for moving azure-npm.yaml):

azure-container-networking/.github/workflows/cyclonus-netpol-test.yaml

Line 55 in 9d8e61e

kubectl apply -f ./npm/profiles/${{ matrix.profile }}

azure-container-networking/.pipelines/npm/npm-conformance-tests.yaml

Line 173 in 9d8e61e

./kubectl --kubeconfig=./kubeconfig apply -f https://raw.githubusercontent.com/Azure/azure-container-networking/master/npm/profiles/$(PROFILE).yaml

Upstream the common practice is to add deployment manifests to the deploy folder. You keep your helm charts, kustomize, vanilla k8s yamls and any related manifests that get picked up by these resources.
The Configmaps are common to both overlays hence they are in the base folder. If you still think profiles should go there I am happy to do that :)

makes sense. base/ seems good (as long as the current profiles are in the same folder, for testing sake)

Lets not increase the scope of this PR, for now, keep it simple with the filename changes and we can do a separate effort/PR to standardize this process , wdygt ?

huntergregory · 2022-01-27T21:54:40Z

npm/deploy/kustomize/overlays/controller/deployment.yaml

@@ -0,0 +1,67 @@
+


nit: remove empty line

huntergregory · 2022-01-27T21:57:36Z

npm/deploy/manifests/common/npm-configmap.yaml

@@ -0,0 +1,24 @@
+---
+apiVersion: v1
+kind: ConfigMap


where is this supposed to be used?

similarly for npm-serviceaccount.yaml and rbac.yaml in the common/ folder

Common across all deployments (aka overlays in kustomize)

huntergregory · 2022-01-27T22:02:06Z

npm/deploy/npm/azure-npm.yaml

@@ -99,18 +99,12 @@ spec:
          volumeMounts:
          - name: xtables-lock
            mountPath: /run/xtables.lock
-          - name: log


impacts these:

azure-container-networking/.github/workflows/cyclonus-netpol-test.yaml

Line 53 in 9d8e61e

kubectl apply -f ./npm/azure-npm.yaml

azure-container-networking/.pipelines/npm/npm-conformance-tests.yaml

Line 167 in 9d8e61e

./kubectl --kubeconfig=./kubeconfig apply -f https://raw.githubusercontent.com/Azure/azure-container-networking/master/npm/azure-npm.yaml

huntergregory · 2022-01-27T22:03:12Z

npm/deploy/npm/azure-npm.yaml

          - name: protocols
            mountPath: /etc/protocols
          - name: azure-npm-config
            mountPath: /etc/azure-npm
      hostNetwork: true
      volumes:
-      - name: log


wondering why we are removing /var/log?

@matmerr said we are using this in production anymore. I made an assumption it wasn't being used at all. I will add it back in :)

Mat said we are or aren't?

We are NOT logging to a file anymore so we can safely remove this.

huntergregory · 2022-01-27T22:09:45Z

npm/deploy/kustomize/overlays/daemon/service.yaml

+apiVersion: v1
+kind: Service
+metadata:
+  name: npm-deamon-metrics-cluster-service


per our Prometheus discussion yesterday, we'll need to serve up the same "cluster-metrics" service that we have currently

azure-container-networking/npm/azure-npm.yaml

Line 130 in 6a63ac0

name: npm-metrics-cluster-service

If we add daemon-specific metrics to npm-metrics-cluster-service, then we'll need to update a scrape config owned by Container Insights team

This is a good point. Can we discuss this on chat?

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

…into nitishm/feat/kubernetes-yamls-decomposed-mode Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

huntergregory · 2022-01-31T19:39:20Z

Discussed offline: we can use one service for the daemon (whether for vanilla NPM or the fan-out mode), which will have 3+ endpoints. The current ones will be for customers:

/cluster-metrics
/node-metrics

and new endpoints can be for internal use

Signed-off-by: Ubuntu <nitishm@acn-dev.a3ahweixzwte5k2rkdoyg14y4b.gx.internal.cloudapp.net>

Use the remote port for the client to connect to the k8s service port Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

Signed-off-by: Nitish Malhotra <nitishm@Ubuntu-21.10>

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

matmerr · 2022-02-02T23:16:16Z

Tiltfile

@@ -0,0 +1,10 @@
+allow_k8s_contexts('acn-dev-azure-cni')
+default_registry('ttl.sh/nitishm-12390')


if this is an ephemeral image can we put it on acnpublic instead?

I couldnt find how to provide the credentials (plus we don't want the credentials checked in). Need to look into this

matmerr · 2022-02-02T23:28:53Z

npm/controller/server.go

 	"k8s.io/klog"
 )

+var aiMetadata string //nolint // aiMetadata is set in Makefile


will require further verification of how this is used, but this is set via the Makefile on build in the dockerfile. Not sure of implications with introducing it into subpackages, and/or if aiMetadata is even relevant in this scope if used in the parent npm/

matmerr · 2022-02-02T23:35:30Z

npm/config/config.go

-		Address: "0.0.0.0",
-		Port:    defaultGrpcPort,
+		Address:    "0.0.0.0",
+		Port:       defaultGrpcPort,


maybe ServerPort to be extra explicit alongside the RemotePort?

ServicePort maybe?

matmerr · 2022-02-02T23:38:21Z

npm/deploy/manifests/common/npm-configmap.yaml

+            "EnableV2NPM":             false,
+            "PlaceAzureChainFirst":    false
+        },
+        "Transport": {


can we add this to examples/windows/* manifests too

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

…://github.com/nitishm/azure-container-networking into nitishm/feat/kubernetes-yamls-decomposed-mode

…rking

…://github.com/nitishm/azure-container-networking into nitishm/feat/kubernetes-yamls-decomposed-mode

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

vakalapa · 2022-02-04T19:21:37Z

/azp run

azure-pipelines · 2022-02-04T19:21:53Z

Azure Pipelines successfully started running 2 pipeline(s).

huntergregory

Will need to revisit service names when we finalize the fan-out model

nitishm marked this pull request as draft January 21, 2022 22:08

Add kustomize manifests

9bd79df

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

nitishm marked this pull request as ready for review January 24, 2022 21:17

nitishm requested review from huntergregory, matmerr and vakalapa and removed request for matmerr and vakalapa January 24, 2022 21:17

nitishm force-pushed the nitishm/feat/kubernetes-yamls-decomposed-mode branch 4 times, most recently from e289220 to 479d729 Compare January 24, 2022 21:41

Address lint errors

5360c30

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

nitishm force-pushed the nitishm/feat/kubernetes-yamls-decomposed-mode branch from 479d729 to 5360c30 Compare January 24, 2022 21:44

vakalapa reviewed Jan 25, 2022

View reviewed changes

huntergregory added the npm Related to NPM. label Jan 27, 2022

huntergregory suggested changes Jan 27, 2022

View reviewed changes

nitishm added 2 commits January 28, 2022 22:09

Deployment manifests

d10798d

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

Merge branch 'master' of github.com:Azure/azure-container-networking …

f900e31

…into nitishm/feat/kubernetes-yamls-decomposed-mode Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

Fix failing tests and other minor changes

0c1af28

Signed-off-by: Ubuntu <nitishm@acn-dev.a3ahweixzwte5k2rkdoyg14y4b.gx.internal.cloudapp.net>

nitishm requested review from huntergregory and vakalapa February 1, 2022 20:32

Ubuntu and others added 3 commits February 1, 2022 21:59

Add RemotePort field to grpc config

04eae56

Use the remote port for the client to connect to the k8s service port Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

Fix lint issue

674b63b

Signed-off-by: Nitish Malhotra <nitishm@Ubuntu-21.10>

Readd the azure-npm.yaml file to npm root for cyclonus

74db27b

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

matmerr reviewed Feb 2, 2022

View reviewed changes

nitishm added 5 commits February 3, 2022 11:22

Readd the azure-npm.yaml file to npm root for cyclonus

ee958c8

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

Merge branch 'nitishm/feat/kubernetes-yamls-decomposed-mode' of https…

b07fae9

…://github.com/nitishm/azure-container-networking into nitishm/feat/kubernetes-yamls-decomposed-mode

Merge branch 'master' of git://github.com/Azure/azure-container-netwo…

017a32a

…rking

Merge branch 'nitishm/feat/kubernetes-yamls-decomposed-mode' of https…

7cf7ea3

…://github.com/nitishm/azure-container-networking into nitishm/feat/kubernetes-yamls-decomposed-mode

Add tm config to windows example manifest

db39877

Signed-off-by: Nitish Malhotra <nitishm@microsoft.com>

vakalapa approved these changes Feb 4, 2022

View reviewed changes

huntergregory approved these changes Feb 4, 2022

View reviewed changes

nitishm merged commit d3aeda7 into Azure:master Feb 5, 2022

		@@ -0,0 +1,10 @@
		allow_k8s_contexts('acn-dev-azure-cni')
		default_registry('ttl.sh/nitishm-12390')

feat: [NPM] Restructure code and add deploy manifests #1203

feat: [NPM] Restructure code and add deploy manifests #1203

Uh oh!

Conversation

nitishm commented Jan 21, 2022

Uh oh!

nitishm commented Jan 21, 2022

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!

huntergregory left a comment

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!

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!

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!

huntergregory Jan 27, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

huntergregory commented Jan 31, 2022

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!

vakalapa commented Feb 4, 2022

Uh oh!

azure-pipelines bot commented Feb 4, 2022

Uh oh!

huntergregory left a comment • 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!

Reviewers

Assignees

Labels

huntergregory Jan 27, 2022 •

edited

Loading

huntergregory left a comment •

edited

Loading