refactor: [CI-CNI] Removing the predefined interfaces from validate #2047

vipul-21 · 2023-07-07T18:16:27Z

Removing the predefined interfaces from validate package

Reason for Change:

Issue Fixed:

Requirements:

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

Notes:

paulyufan2 · 2023-07-25T15:59:22Z

test/validate/client.go

+	return nil
+}
+
+func (v *validator) validate(stateFileIps stateFileIpsFunc, cmd []string, checkType, namespace, labelSelector string) error {


Can we rename it as validateIPs?

jpayne3506 · 2023-07-19T20:04:36Z

test/integration/load/load_test.go

+		t.Log("State file validated successfully")
+	} else if *osType == "linux" {
+		t.Log("Validating the linux state file and restart network scenario")
+		linuxClient := &validate.LinuxClient{}
+		linuxClient = linuxClient.CreateClient(ctx, clientset, config, namespace, *cniType, *restartCase)

-	//We are restarting the systmemd network and checking that the connectivity works after the restart. For more details: https://github.com/cilium/cilium/issues/18706
-	t.Log("Validating the restart network scenario")
-	err = validator.ValidateRestartNetwork()
-	if err != nil {
-		t.Fatal(err)
+		err = linuxClient.Validate()
+		if err != nil {
+			t.Fatal(err)
+		}
 	}
+
+	t.Log("State file validated successfully")


nit: Is 155 or 143 the design pattern?

If linux passes then it will report it once, but windows will report it twice. Would prefer to have the ubiquitous log message outside of the if/if else block.

@jpayne3506 You shouldn't log at all when things are successful. The status of the test will tell you the same information. testing.(*T).Log is really a bit of a misnomer, since it's really to accumulate line-stamped log messages in a buffer that will be displayed if testing.(*T).Fail or testing.(*T).FailNow are called. It's not really a "logger" in the traditional sense.

thatmattlong · 2023-07-27T16:32:22Z

test/integration/load/load_test.go

+	if *osType == "windows" {
+		t.Log("Validating the windows state file")
+		windowsClient := &validate.WindowsClient{}
+		windowsClient = windowsClient.CreateClient(ctx, clientset, config, namespace, *cniType, *restartCase)


no need to create an empty struct to then populate later, just use a constructing function. Change this

func (*WindowsClient) CreateClient(context.Context, ...)

to construct the WindowsClient itself:

func CreateWindowsClient(context.Context, ...) (*WindowsClient) { return &WindowsClient{ clientset: clientset, config: config, ... } }

Cool, updated it with the rebase from your PR.

thatmattlong · 2023-07-27T16:32:52Z

test/integration/load/load_test.go

+	} else if *osType == "linux" {
+		t.Log("Validating the linux state file and restart network scenario")
+		linuxClient := &validate.LinuxClient{}
+		linuxClient = linuxClient.CreateClient(ctx, clientset, config, namespace, *cniType, *restartCase)


same as above, this func should just build a new linuxClient, don't create an empty one then populate it

thatmattlong · 2023-07-27T16:34:10Z

test/integration/load/load_test.go

-	err = validator.ValidateRestartNetwork()
-	if err != nil {
-		t.Fatal(err)
+		err = linuxClient.Validate()


nit:

if err := linuxClient.Validate(); err != nil { t.Fatal(err) }

combining the if statement with err initialization keeps the err variable scoped only to the if block, so it's better to do this when possible to not leak scope.

thatmattlong · 2023-07-27T16:35:25Z

test/validate/client.go

 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 )

-type Validator struct {
+type validator struct {
 	ctx         context.Context


as part of this refactor, we should remove context.Context from this struct. A context should (almost) never get stored in a struct. See this PR where I started refactoring these tests to see how to remove it: #2082

Rebased with the #2082

vipul-21 · 2023-07-27T21:20:56Z

test/integration/load/load_test.go

 	default:
 		t.Fatalf("unknown os type %s", *osType)
 	}
-
-	err = validator.ValidateStateFile(ctx)


Since this is linux specific, I moved it linux validate file instead of load test.

thatmattlong · 2023-07-27T21:22:10Z

test/validate/linux_validate.go

@@ -64,6 +72,10 @@ type Address struct {
 	Addr string `json:"ipv4"`
 }

+type LinuxValidator struct {
+	validator linuxValidator


why have a LinuxValidator and a WindowsValidator that just hold validators? I'm guessing you are trying to implement some os-specific code. I would suggest to just remove this concept of having OS-specific validators, so just get rid of LinuxValidator and WindowsValidator, and just have one (exported) Validator. Then the os-specific branching can happen in the funcs where it needs to.

Yeah that made sense. Removed os specifci validator. Branching the funcs if needed based on os.

paulyufan2

lgtm

vipul-21 · 2023-08-01T18:07:54Z

/azp run Azure Container Networking PR

azure-pipelines · 2023-08-01T18:07:59Z

Command 'Azure' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

azure-pipelines · 2023-08-01T18:09:10Z

Azure Pipelines successfully started running 1 pipeline(s).

…ackage

vipul-21 changed the title ~~refactor: [CI-CNI] Removing the predefined interfaces from validate p…~~ refactor: [CI-CNI] Removing the predefined interfaces from validate Jul 7, 2023

vipul-21 added cni Related to CNI. ci Infra or tooling. labels Jul 7, 2023

vipul-21 requested a review from rbtr July 7, 2023 18:18

vipul-21 force-pushed the singhvipul/validate branch from 4ef0ff2 to b73a55e Compare July 7, 2023 18:38

vipul-21 marked this pull request as ready for review July 7, 2023 19:03

vipul-21 requested a review from a team as a code owner July 7, 2023 19:03

vipul-21 requested review from jpayne3506 and paulyufan2 July 7, 2023 19:04

vipul-21 enabled auto-merge (squash) July 7, 2023 19:04

vipul-21 mentioned this pull request Jul 11, 2023

dualstack Overlay ACN pipeline #2032

Closed

4 tasks

paulyufan2 reviewed Jul 25, 2023

View reviewed changes

jpayne3506 reviewed Jul 25, 2023

View reviewed changes

vipul-21 force-pushed the singhvipul/validate branch from b73a55e to 8d343a8 Compare July 27, 2023 01:17

vipul-21 requested review from jpayne3506 and paulyufan2 July 27, 2023 01:19

vipul-21 force-pushed the singhvipul/validate branch from 8d343a8 to 4120a80 Compare July 27, 2023 15:06

vipul-21 mentioned this pull request Jul 27, 2023

test: clean up cns state check tests #2082

Merged

4 tasks

thatmattlong reviewed Jul 27, 2023

View reviewed changes

vipul-21 disabled auto-merge July 27, 2023 19:43

vipul-21 force-pushed the singhvipul/validate branch from 4120a80 to d185f2a Compare July 27, 2023 21:11

vipul-21 requested a review from thatmattlong July 27, 2023 21:14

vipul-21 commented Jul 27, 2023

View reviewed changes

thatmattlong reviewed Jul 27, 2023

View reviewed changes

vipul-21 force-pushed the singhvipul/validate branch 3 times, most recently from 9e24ce7 to 74cdd87 Compare July 27, 2023 23:40

vipul-21 force-pushed the singhvipul/validate branch 2 times, most recently from c0e672e to 1ce7850 Compare July 28, 2023 18:03

vipul-21 enabled auto-merge (squash) July 28, 2023 18:03

paulyufan2 force-pushed the singhvipul/validate branch from 1ce7850 to 3853ebe Compare August 1, 2023 15:06

paulyufan2 previously approved these changes Aug 1, 2023

View reviewed changes

vipul-21 force-pushed the singhvipul/validate branch from 3853ebe to 249a02a Compare August 1, 2023 16:11

vipul-21 dismissed paulyufan2’s stale review via 249a02a August 1, 2023 18:12

refactor: [CI-CNI] Removing the predefined interfaces from validate p…

42f4e7a

…ackage

vipul-21 force-pushed the singhvipul/validate branch from 249a02a to 42f4e7a Compare August 1, 2023 18:13

paulyufan2 approved these changes Aug 1, 2023

View reviewed changes

vipul-21 merged commit 0e4fbdd into master Aug 1, 2023

vipul-21 deleted the singhvipul/validate branch August 1, 2023 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [CI-CNI] Removing the predefined interfaces from validate #2047

refactor: [CI-CNI] Removing the predefined interfaces from validate #2047

vipul-21 commented Jul 7, 2023 •

edited

Loading

paulyufan2 Jul 25, 2023

jpayne3506 Jul 19, 2023

timraymond Jul 25, 2023

thatmattlong Jul 27, 2023

vipul-21 Jul 27, 2023

thatmattlong Jul 27, 2023

thatmattlong Jul 27, 2023

thatmattlong Jul 27, 2023

vipul-21 Jul 27, 2023

vipul-21 Jul 27, 2023

thatmattlong Jul 27, 2023

vipul-21 Jul 27, 2023

paulyufan2 left a comment

vipul-21 commented Aug 1, 2023 •

edited

Loading

azure-pipelines bot commented Aug 1, 2023

azure-pipelines bot commented Aug 1, 2023

refactor: [CI-CNI] Removing the predefined interfaces from validate #2047

refactor: [CI-CNI] Removing the predefined interfaces from validate #2047

Conversation

vipul-21 commented Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulyufan2 left a comment

Choose a reason for hiding this comment

vipul-21 commented Aug 1, 2023 • edited Loading

azure-pipelines bot commented Aug 1, 2023

azure-pipelines bot commented Aug 1, 2023

vipul-21 commented Jul 7, 2023 •

edited

Loading

vipul-21 commented Aug 1, 2023 •

edited

Loading