dualstack Overlay ACN pipeline #2032

paulyufan2 · 2023-06-24T01:25:13Z

Reason for Change:

This PR is the datapath for ACN Linux test and dualstack Linux node properties check and cns state file check.
To run them locally, use following commands on dualstack overlay setup:

Linux Test Commands:
go test -timeout 30m -tags load -run ^TestLoad$ -tags=load
go test -timeout 30m -tags load -cni cniv2 -run ^TestValidateState$ -tags=load
go test -timeout 30m -tags load -run ^TestDualStackProperties$ -tags=load
go test -count=1 test/integration/datapath/datapath_linux_test.go -timeout 1m -tags connection -run ^TestDatapathLinux$ -tags=connection,integration -v

Windows Test Commands
go test -timeout 30m -tags load -run ^TestLoad$ -tags=load -os=windows
go test -timeout 30m -tags load -run ^TestDualStackProperties$ -tags=load -os=windows -cni cniv2
go test -timeout 30m -tags load -cni cniv2 -run ^TestValidateState$ -tags=load -os=windows
go test -count=1 test/integration/datapath/datapath_windows_test.go -timeout 3m -tags connection -run ^TestDatapathWin$ -tags=connection -v

Logs:
https://msazure.visualstudio.com/One/_build/results?buildId=76033872&view=results

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

pjohnst5 · 2023-06-26T17:59:06Z

test/integration/datapath/datapath_linux_test.go

+	-nodepoolSelector="yournodepoolname"
+
+To run the test use one of the following commands:
+go test -count=1 test/integration/datapath/datapath_linux_test.go -timeout 3m -tags connection -run ^TestDatapathLinux$ -tags=connection,integration


I think you want to add -mod=mod also to the test parameters, otherwise, golang makes you do go mod vendor which changes a bunch of files in the vendor directory

If you add the -mod=mod argurment however, no go mod vendor is needed

Looking at make test-integration, it uses -mod=readonly, which ensures that it doesnt read the vendor directory. Would that be more beneficial here?

there should be no vendor directory

test/integration/datapath/datapath_linux_test.go

pjohnst5 · 2023-06-26T18:54:13Z

test/integration/datapath/datapath_linux_test.go

+const (
+	LinuxDeployIPV4            = "../manifests/datapath/linux-deployment.yaml"
+	LinuxDeployIPv6            = "../manifests/datapath/linux-deployment-ipv6.yaml"
+	podLabelKey                = "app"


This podLabelKey field is re-defined here, so it is throwing an error using local debugging:

The reason is, the datapath_win_test.go file, is not named datapath_windows_test.go
Go knows which OS you're using, and if you name your files *_linux_* or *_windows_*, it will ignore them if that's not your OS
*_win_* however, is not recognized by Go for windows

So can you rename the datapath_win_test.go file to datapath_windows_test.go?

Today I learned! Does GO know based off of what GOOS is set as? If so, that would avoid the overwriting from occurring if the filename change happens.

Windows clusters can be run on a linux VM. Wouldn't that break this setup? If so, perhaps they shouldn't be under the same build tag.

Does GO know based off of what GOOS is set as

yes

*_windows.go is equivalent to adding a build tag //go:build windows in the file
https://pkg.go.dev/go/build

pjohnst5 · 2023-06-26T18:59:25Z

test/integration/manifests/goldpinger/daemonset-ipv6.yaml

+        app: goldpinger
+        type: goldpinger-host
+    spec:
+      tolerations:


This is trying to make goldpinger pods on my winodws node pool (don't want that, since it's for linux only, should target linux nodes only)
Can you make it so the goldpinger pods only get created on linux nodes, by using taints/tolerations?
https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/

Ah, I see you have this flag in your tests, which is good:
-nodepoolSelector="yournodepoolname"

Keep that, but also I think you should make sure, there is no way, that this goldpinger daemonset can land on windows VMs, since goldpinger does not support windows

jpayne3506 · 2023-06-27T00:12:09Z

test/integration/datapath/datapath_linux_test.go

+		}
+
+		t.Run("all linux pods can ping each other", func(t *testing.T) {
+			clusterCheckCtx, cancel := context.WithTimeout(ctx, 3*time.Minute)


Why do you get another context here? No other test has its context reassigned. Is this to leverage goldpinger?

yes, it's specifically used for goldpinger to get clusterState

jpayne3506 · 2023-06-27T00:44:17Z

test/validate/linux_validate.go

+
+		// get node allocated IPs and check whether it includes ipv4 and ipv6 address
+		// node status addresses object will return three objects; two of them are ip addresses object(one is ipv4 and one is ipv6)
+		if len(nodes.Items[index].Status.Addresses) < dualstackNodeStatusAddr {


I think you should explicitly check for the number of type InternalIP. There are multiple types of addresses that can be applied to a node and you are capturing all of them with this. https://github.com/kubernetes/api/blob/v0.27.3/core/v1/types.go#L5468-L5518

test/internal/k8sutils/utils_get.go

vipul-21 · 2023-06-26T17:01:42Z

test/integration/datapath/datapath_linux_test.go

+
+// return podLabelSelector and nodeLabelSelector
+func createLabelSelectors() (string, string) {
+	return fmt.Sprintf("%s=%s", podLabelKey, *podPrefix), fmt.Sprintf("%s=%s", nodepoolKey, *nodepoolSelector)


@jpayne3506 @paulyufan2 i guess this function can be extracted as I see John is also using the same thing in his windows tests.

vipul-21 · 2023-06-26T17:25:30Z

test/integration/datapath/datapath_linux_test.go

+		require.NoError(t, err, "could not get k8s node list: %v", err)
+	}
+
+	createPodFlag := !(apierrors.IsAlreadyExists(err))


Why is createPodFlag is using alreadyexist err of list the nodes ? I don't think nodelist will give an error of type already exists

i think we should be checking for namespace creation(create if not exists) as the podnamespace is a flag

vipul-21 · 2023-06-30T00:01:51Z

hack/aks/Makefile

+
+dualstack-windows-up: ## Brings up windows nodes on dualstack overlay cluster
+	$(AZCLI) aks nodepool add -g $(GROUP) -n npwin \
+	    --cluster-name  $(CLUSTER) \


nit: indentation is not aligned with the other attributes.

vipul-21 · 2023-06-30T00:11:54Z

test/integration/datapath/datapath_linux_test.go

+		if *testProfile == LinuxDeployIPV4 {
+			daemonset, err = k8sutils.MustParseDaemonSet(gpDaemonset)
+			if err != nil {
+				t.Fatal(err)
+			}
+		} else {
+			daemonset, err = k8sutils.MustParseDaemonSet(gpDaemonsetIPv6)
+			if err != nil {
+				t.Fatal(err)
+			}
+		}


nit: This can be replaced with(easier to scale if we need to add more test profiles.)

var gpDaemonset= map[string]string{ "LinuxDeployIPV4": "filename_linux", "DualStack" : "filename" } ... daemonset, err = k8sutils.MustParseDaemonSet(gpDaemonset[*testProfile]) if err != nil { t.Fatal(err) } ...

vipul-21 · 2023-06-30T00:25:45Z

test/integration/datapath/datapath_linux_test.go

+			podsClient := clientset.CoreV1().Pods(*podNamespace)
+
+			checkPodIPsFn := func() error {
+				podList, err := podsClient.List(ctx, metav1.ListOptions{LabelSelector: "app=goldpinger"})
+				if err != nil {
+					return err
+				}
+
+				if len(podList.Items) == 0 {
+					return errors.New("no pods scheduled")
+				}
+
+				for _, pod := range podList.Items {
+					if pod.Status.Phase == apiv1.PodPending {
+						return errors.New("some pods still pending")
+					}
+				}
+
+				for _, pod := range podList.Items {
+					if pod.Status.PodIP == "" {
+						return errors.New("a pod has not been allocated an IP")
+					}
+				}
+
+				return nil
+			}
+			err := defaultRetrier.Do(ctx, checkPodIPsFn)


The function WaitForPodsRunning does the same thing, we can use it here.

vipul-21 · 2023-06-30T00:29:48Z

test/integration/manifests/load/privileged-daemonset.yaml

            runAsUser: 0
          volumeMounts:
-            - mountPath: /var/run/azure-cns
+            - mountPath: /var/run/azure-network


This might break the existing cilium overlay load test pipeline fi privileged pods are used to get the json file. ( I don't think they do, but good to check)

test/internal/k8sutils/utils_get.go

test/validate/client.go

vipul-21 · 2023-06-30T00:38:56Z

test/validate/linux_validate.go

+			for _, ip := range networks.Endpoints {
+				pod := ip.PODName
+				ipv4 := ip.IPAddresses[0].IP
+				azureCnsPodIps[ipv4] = pod


I think this would pass in both cases, but want to make sure the variable names though. In case of dual stack the first IP address is IPv4, i thought it would be IPv6 ?

vipul-21 · 2023-06-30T00:40:21Z

test/validate/linux_validate.go

+		// check node status
+		nodeConditions := nodes.Items[index].Status.Conditions
+		if nodeConditions[len(nodeConditions)-1].Type != corev1.NodeReady {
+			return errors.Wrapf(err, "node %s status is not ready", nodeName)


Is this because of error ? Or we expect the node to be ready after a certain time. ?
If it is going to be ready after a certain time, should we add a retry ?

ashvindeodhar · 2023-07-10T19:50:41Z

.pipelines/singletenancy/dualstack-overlay/dualstackoverlay-e2e-step-template.yaml

+    name: "DualStack_Overlay_Linux_tests"
+    displayName: "DualStack Overlay Linux Tests"
+
+  - task: AzureCLI@2


I would suggest move the windows part to a separate PR. Let's keep the first PR for adding linux coverage for dualstack

ashvindeodhar · 2023-07-10T19:53:59Z

.pipelines/singletenancy/dualstack-overlay/dualstackoverlay-e2e-job-template.yaml

@@ -0,0 +1,32 @@
+parameters:


IMO, existing
.pipeline/singletenancy/overlay/* should be moved to cilium folder. Cilium folder can have overlay folder if needed. @tamilmani1989 @vipul-21
We should create
.pipeline/singletenancy/azcnioverlay/v4/.yaml
.pipeline/singletenancy/azcnioverlay/dualstack/.yaml

The idea was to keep the singletenancy folder as it is. Maybe add a parent to it which tells us that this is PR pipeline code and under that we can have different folder structure like:
pr_pipeline(or something better)/singletenancy/azcnioverlay/cilium
pr_pipeline(or something better)/singletenancy/azcnioverlay/v4/.yaml
pr_pipeline(or something better)/multitenany/azcnioverlay/dualstack/.yaml

Thoughts ?

hack/aks/Makefile

ashvindeodhar · 2023-07-10T19:55:34Z

hack/aks/Makefile

+		--yes
+	@$(MAKE) set-kubeconf
+
+dualstack-windows-up: ## Brings up windows nodes on dualstack overlay cluster


as mentioned above, let's move windows out of this PR

these make targets are misleading, they do not bring up a cluster, or (as far as I can tell) do anything dualstack-specific. it adds a windows nodepool. it should just be called add windows nodepool.

ashvindeodhar · 2023-07-10T19:56:28Z

hack/aks/README.md

+  swift-up              Bring up a SWIFT AzCNI cluster
+  windows-cniv1-up      Bring up a Windows AzCNIv1 cluster
+  dualstack-overlay-up  Brings up an dualstack overlay cluster
+  dualstack-windows-up  Brings up windows nodes on dualstack overlay cluster


dualstack-overlay-byocni-up: is missing

ashvindeodhar · 2023-07-10T20:06:14Z

test/integration/manifests/datapath/linux-deployment.yaml

@@ -0,0 +1,86 @@
+apiVersion: apps/v1


can we not reuse test\integration\manifests\goldpinger\deployment.yaml ?

The original goldpinger deployment yaml does not have:

- name: HOSTS_TO_RESOLVE value: "1.1.1.1 8.8.8.8 www.bing.com"

That we need to use to test host resolution

ashvindeodhar · 2023-07-10T20:06:45Z

test/integration/manifests/datapath/linux-deployment-ipv6.yaml

@@ -0,0 +1,88 @@
+apiVersion: apps/v1


can this be added to goldpinger folder to be consistent with existing structure?

rbtr · 2023-07-21T17:39:58Z

Makefile

 	kind create cluster --config ./test/kind/kind.yaml

+# install azure Linux CNS and CNI dropgz images
+install-azure-images:


if we're going to create standalone tooling for installing our components, let's fully commit to it and write some small kustomize payloads for them instead of hacking it with the integration test like this.

rbtr · 2023-07-21T17:42:51Z

hack/aks/Makefile

+		--yes
+	@$(MAKE) set-kubeconf
+
+dualstack-windows-up: ## Brings up windows nodes on dualstack overlay cluster


these make targets are misleading, they do not bring up a cluster, or (as far as I can tell) do anything dualstack-specific. it adds a windows nodepool. it should just be called add windows nodepool.

paulyufan2 · 2023-08-10T19:36:58Z

separate this PR to two PRs. Close this PR

paulyufan2 added 3 commits June 23, 2023 21:19

add Linux test cases for connectivity

7ac3b4d

datapath linux acn pipeline

54e90e3

acn datapath linux

044f9e9

paulyufan2 requested a review from a team as a code owner June 24, 2023 01:25

paulyufan2 requested a review from camrynl June 24, 2023 01:25

paulyufan2 added 5 commits June 23, 2023 21:50

add linux acn control plane test cases

1e75bda

add validateDualStack test case

08eb048

fix issues

4cc4e7c

fix linter issues

61ccd32

fix linter issues

ed5b10b

paulyufan2 added cni Related to CNI. ci Infra or tooling. labels Jun 24, 2023

fix minor issues

df934ec

pjohnst5 requested changes Jun 26, 2023

View reviewed changes

jpayne3506 reviewed Jun 27, 2023

View reviewed changes

paulyufan2 and others added 7 commits June 27, 2023 14:02

change file name

f5f96bb

fix conflicts

280493b

fix conflicts

8d3ce61

fix conflicts

f76a6ab

Merge branch 'master' into datapath_Linux_acn

d7fb934

integrate #1980 to one PR

da5879a

fix conflicts

0ca9a4b

paulyufan2 changed the title ~~Datapath linux ACN pipeline~~ dualstack and v4 Overlay Linux ACN pipeline Jun 29, 2023

paulyufan2 changed the title ~~dualstack and v4 Overlay Linux ACN pipeline~~ dualstack and v4 Overlay ACN pipeline Jun 29, 2023

vipul-21 requested changes Jun 30, 2023

View reviewed changes

paulyufan2 added 2 commits June 30, 2023 11:28

add windows properties check

70f355d

add windows dualstack hns test cases

5f55aab

paulyufan2 changed the title ~~dualstack and v4 Overlay ACN pipeline~~ dualstack Overlay ACN pipeline Jul 1, 2023

paulyufan2 added 2 commits July 5, 2023 11:18

dualstack overlay ipv6 windows connectivity tests

e0c112b

fix a minor issue

7994f7c

paulyufan2 added 6 commits July 6, 2023 16:41

add AzureCLI for windows nodepool

2723a46

remove ssh key check

46c945c

change the Linux test cases sequence

19bca75

add byo-cni

93b3e77

remove ns-load ns

bb134ed

remove ns-load ns

e75c993

paulyufan2 force-pushed the datapath_Linux_acn branch from a4a2d8d to e75c993 Compare July 7, 2023 17:39

paulyufan2 and others added 2 commits July 7, 2023 13:47

Merge branch 'master' into datapath_Linux_acn

ab611bb

fix deployment files

943926b

jpayne3506 mentioned this pull request Jul 7, 2023

ci:[CNI] Windows datapath using k8se2e template #2041

Merged

4 tasks

ashvindeodhar requested changes Jul 10, 2023

View reviewed changes

paulyufan2 and others added 15 commits July 10, 2023 17:31

windows ipv6 invoke-webrequest test case

3e5eb23

fix issues

bec1fad

Merge branch 'master' into datapath_Linux_acn

56fc1b4

fix ipv6 command

975a46b

fix windows ipv6 command

e7a7d7f

test v6 curl

bf1be0e

install linux cns and cni

878ed1c

add aks-custom-headers

2ec9b19

add aks-custom-headers

6e5b695

add install azure-vnet

18bc3a5

install kube

d942050

disable cleaning up dualstack overlay cluster

a32e69d

fix dualstack overlay linux dropgz conflist file

5b8ae6d

fix dropgz image

892012c

add sudo to run test cases

0fb5594

rbtr reviewed Jul 21, 2023

View reviewed changes

rbtr mentioned this pull request Aug 3, 2023

ci: dualstack overlay linux test cases #2098

Merged

4 tasks

paulyufan2 closed this Aug 10, 2023

dualstack Overlay ACN pipeline #2032

dualstack Overlay ACN pipeline #2032

Uh oh!

Conversation

paulyufan2 commented Jun 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

rbtr Jul 21, 2023 • 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!

paulyufan2 commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

paulyufan2 commented Jun 24, 2023 •

edited

Loading

rbtr Jul 21, 2023 •

edited

Loading