Adds debug commands to CNS binary with debug API #650

matmerr · 2020-08-14T04:01:05Z

Reason for Change:

Issue Fixed:

Adds a debug API to get IP addresses, options include "Available", "Allocated", and "All".

Usage example:

$ k exec -it azure-cns-z74hd -- /usr/local/bin/azure-cns -cmd get -darg All
{10.0.0.39 Allocated}
{10.0.0.40 Allocated}
{10.0.0.41 Available}
{10.0.0.42 Allocated}
{10.0.0.43 Available}
{10.0.0.44 Available}
...

$ k exec -it azure-cns-z74hd -- /usr/local/bin/azure-cns -cmd get -darg Allocated
{10.0.0.39 Allocated}
{10.0.0.40 Allocated}
{10.0.0.42 Allocated}
{10.0.0.45 Allocated}
{10.0.0.46 Allocated}
{10.0.0.48 Allocated}
{10.0.0.50 Allocated}
...

$ k exec -it azure-cns-z74hd -- /usr/local/bin/azure-cns -cmd get -darg Available
{10.0.0.41 Available}
{10.0.0.43 Available}
{10.0.0.44 Available}
{10.0.0.47 Available}
{10.0.0.49 Available}
{10.0.0.54 Available}

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

codecov · 2020-08-14T04:41:35Z

Codecov Report

Merging #650 into master will decrease coverage by 0.18%.
The diff coverage is 18.01%.

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
- Coverage   38.57%   38.38%   -0.19%     
==========================================
  Files          79       80       +1     
  Lines       10471    10577     +106     
==========================================
+ Hits         4039     4060      +21     
- Misses       5941     6019      +78     
- Partials      491      498       +7

weng271190436 · 2020-08-14T18:36:12Z

cns/cnsclient/cli.go

+		addrs, _ := iface.Addrs()
+		for _, address := range addrs {
+			if ipnet, ok := address.(*net.IPNet); ok && !ipnet.IP.IsLoopback() && ipnet.IP.To4() != nil {
+				ip = ipnet.IP


Should you break once you found one? Or are you looking for the last one that satisfies these criteria

weng271190436 · 2020-08-14T18:47:24Z

cns/cnsclient/cli.go

+
+		printIPAddresses(addr)
+	default:
+		fmt.Printf("argument supplied for the get cmd, use the '%v' flag", acn.OptDebugCmdAlias)


are you trying to say "no/invalid argument supplied for the get cmd"?

yeah, the error message reads a bit off

weng271190436 · 2020-08-14T20:49:44Z

cns/restserver/ipam.go

+	logger.Request(service.Name, &req, err)
+	if err != nil {
+		returnMessage = err.Error()
+		return


If you return here, returnMessage and statusCode won't do anything because your defer func() is below?

weng271190436 · 2020-08-14T21:01:17Z

cns/service/main.go


+	if clientDebugCmd != "" {
+		cnsclient.HandleCNSClientCommands(clientDebugCmd, clientDebugArg)
+		os.Exit(0)


I think we should still return 1 when an error happened instead of always returning 0 even though this is just a debug tool

weng271190436 · 2020-08-14T21:01:48Z

cns/cnsclient/cli.go

+	}
+
+	cnsurl := "http://" + ip.String() + ":10090"
+	cnsClient, _ := InitCnsClient(cnsurl)


is the second return value error?

should log it and exit 1 if an error happened

ramiro-gamarra

A few comments

ramiro-gamarra · 2020-09-23T01:17:38Z

cns/cnsclient/cli.go

+			return false
+		}
+
+		return addrSlice[i].IPAddress < addrSlice[j].IPAddress


you only need this line, the rest of the block can be removed

ramiro-gamarra · 2020-09-23T01:24:37Z

cns/cnsclient/cli.go

+	releaseArg = "release"
+
+	eth0InterfaceName   = "eth0"
+	azure0InterfaceName = "azure0"


Some of these are unused, the rest can be consts

ramiro-gamarra · 2020-09-23T01:34:45Z

cns/cnsclient/cli.go

+	default:
+		return fmt.Errorf("No debug cmd supplied, options are: %v", getArg)
+	}
+	return nil


do you foresee commands other than get being added? maybe simpler to just

if !strings.EqualFold(cmd, getArg) { return fmt.Errorf("invalid cmd %q supplied, options are: %v", cmd, getArg) } return getCmd(cnsClient, arg)

or if more commands will be added, switching is fine, but I would still recommend to equalfold

ramiro-gamarra · 2020-09-23T01:35:23Z

cns/cnsclient/cli.go

+)
+
+var (
+	getArg = "get"


I think a better name for this would be getCmd

ramiro-gamarra · 2020-09-23T01:39:20Z

cns/cnsclient/cli.go

+	return nil
+}
+
+func getCmd(client *CNSClient, arg string) {


I think this func should return an error. It seems the ultimate caller of these debug commands is logging any errors anyway, so it will trim down some lines below.

ramiro-gamarra · 2020-09-23T01:46:39Z

cns/cnsclient/cli.go

+			fmt.Println(err)
+			return
+		}
+		printIPAddresses(addr)


you could DRY up this block a bit by just building the args in each case, such as

var states []string switch { case strings.EqualFold(args, cns.Available): // this is just to make it simpler on the client, where Available or available will work states = append(states, cns.Available) // ... } addr, err := client.GetIPAddressesMatchingStates(states...) if err != nil { return err } printIPAddresses(addr)

ramiro-gamarra · 2020-09-23T01:54:30Z

cns/cnsclient/cnsclient.go

+
+// GetIPAddressesWithStates takes a variadic number of string parameters, to get all IP Addresses matching a number of states
+// usage GetIPAddressesWithStates(cns.Available, cns.Allocated)
+func (cnsClient *CNSClient) GetIPAddressesMatchingStates(StateFilter ...string) ([]cns.IPAddressState, error) {


One thing about variadic funcs is that it's possible for clients to pass no args. I'm curious if the behavior then should be to just no-op.

ramiro-gamarra · 2020-09-23T01:56:05Z

cns/cnsclient/cnsclient.go

+		body bytes.Buffer
+	)
+
+	httpc := &http.Client{}


you can just use http.Post below (uses the default client) if we don't need any custom http config.

ramiro-gamarra · 2020-09-23T02:10:30Z

cns/restserver/ipam.go

+	for _, podstate := range req.IPConfigStateFilter {
+		resp.IPAddresses = append(resp.IPAddresses, filterIPConfigsMatchingState(service.PodIPConfigState, podstate, func(ipconfig cns.IPConfigurationStatus, state string) bool {
+			return ipconfig.State == podstate
+		})...)


No need to append. You can directly assign the result of filtering to resp.IPAddresses. You can also extract the anonymous function out to make it a bit easier to read.

ramiro-gamarra · 2020-09-23T02:19:49Z

cns/restserver/ipam.go

+}
+
+// filter the ipconfigs in CNS matching a state (Available, Allocated, etc.) and return in a slice
+func filterIPConfigsMatchingState(toBeAdded map[string]cns.IPConfigurationStatus, state string, f func(cns.IPConfigurationStatus, string) bool) []cns.IPAddressState {


The name seems odd given you are passing a func with which you could do the opposite :) and it also transforms the input. There are a few times when I miss generics and working with collections in a functional manner is one of them. Maybe a predicate function is not needed if we always want to filter based on a match? Would clean up the above too.

matmerr · 2020-10-07T17:05:48Z

/azp run

azure-pipelines · 2020-10-07T17:05:55Z

No pipelines are associated with this pull request.

matmerr · 2020-10-14T17:50:27Z

/azp run

azure-pipelines · 2020-10-14T17:50:42Z

Azure Pipelines successfully started running 1 pipeline(s).

ramiro-gamarra

* feat: add cns debug api and cli * update tests * fixed nits * feedback * remove merge artifacts * rebase artifacts * fix missing variable rename * add support for pending release * feedback * connect to localhost

matmerr force-pushed the cnsdebugcli branch 2 times, most recently from 7f88710 to 569d53e Compare August 14, 2020 04:34

weng271190436 reviewed Aug 14, 2020

View reviewed changes

matmerr force-pushed the cnsdebugcli branch from d02df45 to ef38406 Compare August 17, 2020 15:49

matmerr force-pushed the cnsdebugcli branch from ef38406 to f158c05 Compare August 27, 2020 20:14

weng271190436 previously approved these changes Aug 28, 2020

View reviewed changes

matmerr dismissed weng271190436’s stale review via d8d94de September 1, 2020 23:08

matmerr force-pushed the cnsdebugcli branch 2 times, most recently from d8d94de to 3c22c7d Compare September 1, 2020 23:13

matmerr force-pushed the cnsdebugcli branch from 14ddbe1 to cc70fd1 Compare September 17, 2020 14:35

ramiro-gamarra reviewed Sep 23, 2020

View reviewed changes

matmerr force-pushed the cnsdebugcli branch from 26ed600 to 4f94a8a Compare September 28, 2020 19:53

matmerr force-pushed the cnsdebugcli branch from 4f94a8a to 3afbd82 Compare October 20, 2020 20:17

matmerr added 10 commits October 29, 2020 16:10

feat: add cns debug api and cli

00dcae1

update tests

1afeb35

fixed nits

fe55a4a

feedback

494e789

remove merge artifacts

8535a04

rebase artifacts

f2bf85c

fix missing variable rename

0a77cf0

add support for pending release

eedbda2

feedback

3dbf841

connect to localhost

b506d0e

matmerr force-pushed the cnsdebugcli branch from 3afbd82 to b506d0e Compare October 30, 2020 00:16

ramiro-gamarra approved these changes Oct 30, 2020

View reviewed changes

matmerr merged commit c2b8cae into Azure:master Nov 2, 2020

matmerr deleted the cnsdebugcli branch November 2, 2020 17:53

Adds debug commands to CNS binary with debug API #650

Adds debug commands to CNS binary with debug API #650

Uh oh!

Conversation

matmerr commented Aug 14, 2020

Uh oh!

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

Uh oh!

Codecov Report

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!

ramiro-gamarra 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!

matmerr commented Oct 7, 2020

Uh oh!

azure-pipelines bot commented Oct 7, 2020

Uh oh!

matmerr commented Oct 14, 2020

Uh oh!

azure-pipelines bot commented Oct 14, 2020

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 14, 2020 •

edited

Loading