Logging by wrkode · Pull Request #8 · wrkode/namespace-watcher

wrkode · 2023-03-24T19:26:41Z

This PR addresses the issues: #1, #2, #3, #4, #5, #6, #7

…riables

…paces to be excluded

wrkode · 2023-03-25T09:00:36Z

@flavio please review

flavio · 2023-03-27T06:23:21Z

+
+	cpuLimitMin, err := resource.ParseQuantity(limits.CpuLimitMin)
+	if err != nil {
+		logrus.Fatalf("error parsing CPU_LIMIT_MIN: ", err)


It should return the error, given this function returns a error type. The logrus.Fatalf would panic

flavio · 2023-03-27T06:24:45Z

+			if err != nil {
+				return err
+			}
+			logrus.Warn("Created LimitRange for namespace: ", namespaceName)


This is a Info message IMHO

flavio · 2023-03-27T06:25:56Z

+		if err != nil {
+			return err
+		}
+		logrus.Warn("Updated LimitRange for namespace: ", namespaceName)


Also this one should be an Info message

flavio · 2023-03-27T06:27:33Z

+func lookupEnvOrEmpty(key string) string {
+	value, _ := os.LookupEnv(key)
+
+	return value
+}


This is not needed, just use os.Getenv which does exactly the same thing

I'm not sure I understand.

You can remove this function and just invoke os.Getenv instead of lookupEnvOrEmpty. The STD-lib function does exactly the very same thing as this code you wrote

flavio · 2023-03-27T06:33:29Z

+	CpuLimitMax              string
+	MemLimitMax              string
+	EphemeralStorageLimitMax string
+	CpuLimitMin              string
+	MemLimitMin              string
+	EphemeralStorageLimitMin string


use resource.Quantity instead

flavio · 2023-03-27T06:34:04Z

+	//Evaluate if environment variables are not set or set to ""
+	setLimits.CpuLimitMin = lookupEnvOrEmpty("CPU_LIMIT_MIN")
+	if setLimits.CpuLimitMin == "" || len(strings.TrimSpace(setLimits.CpuLimitMin)) == 0 {
+		logrus.Fatal("Environment variable CPU_LIMIT_MIN is not set or empty: ", setLimits.CpuLimitMin)


Some considerations about these conditional checks:

Do not use Fatal, it causes the program to exit immediately

Given you have a default in place, it's not even worth to mention that to the user. If you really want to, use the Trace or Debug level

I don't think you have to do all this "massaging" of the user input

There's a lot of repetition across these if statements. Maybe you can create a function like parseValueOrDefault(envKey string, defaultValue resource.Quantity) (resource.Quantity, error)

I'm not putting a default in place.
I thought of Fatal because the watcher should fail/stop if the values can't be used?

I see, I got confused. I though you had a default in place. It's up to you to decide what to do when one of these settings is missing.

Fine with me if you want to exit with an error, hence Fatal 😄

On a second thought. It might be a good idea to fall back to a set of defaults for if the use set limits are empty or wrong set user input.
I will add this into a feature for a new PR

flavio · 2023-03-27T06:34:54Z

 	if err != nil {
-		fmt.Println("Failed to get Kubernetes config:", err)
+		logrus.Fatal("Failed to get Kubernetes config: ", err)
 		os.Exit(1)


this line is not needed because Fatal causes the exit too

flavio · 2023-03-27T06:35:02Z

 	if err != nil {
-		fmt.Println("Failed to create Kubernetes client:", err)
+		logrus.Fatal("Failed to create Kubernetes client: ", err)
 		os.Exit(1)


flavio · 2023-03-27T06:35:08Z

 	if err != nil {
-		fmt.Println("Failed to watch namespaces:", err)
+		logrus.Fatal("Failed to watch namespaces: ", err)
 		os.Exit(1)


flavio · 2023-03-27T06:36:05Z

+			err := createOrUpdateLimitRange(clientset, namespaceName, setLimits)
 			if err != nil {
-				fmt.Printf("Failed to create LimitRange for namespace %s: %v\n", namespaceName, err)
+				logrus.Warn("Failed to create LimitRange for namespace: ", namespaceName, " ", err)


This is an Error

…PR#1

wrkode · 2023-03-29T11:14:55Z

@flavio please review the latest PR changes.

flavio · 2023-03-29T12:26:45Z

+	q, err := resource.ParseQuantity(value)
+	if err != nil {
+		logrus.Error("Error parsing ", key, ": ", err)
+		return resource.Quantity{}


In this case I would definitely return with an error. The user has to be made aware of that, the program has to exit

flavio · 2023-03-29T12:28:17Z

+	excludedNamespaces := map[string]struct{}{
+		"default":         {},
+		"cattle":          {},
+		"kube-system":     {},
+		"kube-public":     {},
+		"istio-system":    {},
+		"kube-node-lease": {},
+		"kube-local":      {},
+	}


Please, use a Set as a data structure: https://pkg.go.dev/github.com/deckarep/golang-set/v2#section-readme

flavio · 2023-03-29T12:29:24Z

+	if strings.Contains(namespaceName, "cattle") {
+		return true
+	}


Instead of doing that, read the list of namespaces that the user provided and then add to the cattle and other names to it.

Update: you can even drop that, you're already adding cattle and other namespaces to the excluded list

I have addressed this, but for some reason I must explicitly check for cattle word in the namespace.

wrkode · 2023-03-29T18:20:46Z

@flavio ready for new review

flavio · 2023-04-03T12:41:09Z

+			isExcluded := strings.Contains(namespaceName, "cattle") || excludedNamespaces.Contains(namespaceName)
+
+			if isExcluded {


The cattle namespace is already added to the list of excluded namespaces (see line 150).

This code can be simplified in this way:

Suggested change

isExcluded := strings.Contains(namespaceName, "cattle") || excludedNamespaces.Contains(namespaceName)

if isExcluded {

if excludedNamespaces.Contains(namespaceName) {

I will address in rc1

flavio · 2023-04-03T12:42:18Z

+			isExcluded := strings.Contains(namespaceName, "cattle") || excludedNamespaces.Contains(namespaceName)
+
+			if isExcluded {
+				logrus.Info("Checking if namespace should be skipped: ", namespaceName, " - Excluded: ", isExcluded)


This log message should be placed outside of the if statement, right now this is shown only when the check has already been done. Moreover, this is more of a debug message, you should not pollute the production logs with this message.

I will address in rc1

flavio · 2023-04-03T12:43:54Z

 - apiGroups: [""]
  resources: ["namespaces", "limitranges"]
-  verbs: ["get", "watch", "list", "create", "delete"]
+  verbs: ["get", "watch", "list", "create", "update", "delete"]


I think you should split that into 2 groups: one for namespace. Here you get only watch I think. The other one for limitranges, this one requires more permissions (I think get, list, create and delete)

I will address in rc1

…e double checking for cattle namespaces

wrkode added 6 commits March 14, 2023 09:54

added exclusion for namespace kube-node-lease

8a45895

improved logging. using logrus

70e3ebf

improved handling of limits - read in main function

b549907

added better parsing of user input with ParseQuantity

57b3ca4

update deployment manifest example

1835cfb

improved error handling and logging

1124bf5

wrkode self-assigned this Mar 24, 2023

wrkode marked this pull request as draft March 24, 2023 20:08

wrkode added 4 commits March 24, 2023 21:30

added capability to update existing LimitRange if exists

e47643d

added update verb to ClusterRole

0113c44

switched to os.LookupEnv and added checks for unset or empty limit va…

7416edf

…riables

improved namespace exclusion code and added dynamic addition of names…

68ece02

…paces to be excluded

wrkode marked this pull request as ready for review March 24, 2023 22:17

flavio reviewed Mar 27, 2023

View reviewed changes

wrkode marked this pull request as draft March 28, 2023 16:48

wrkode added 2 commits March 29, 2023 13:05

addressed extensive use of logrus.Fatal and comments from @flavio on …

99be1f7

…PR#1

update deployment to 1-aplha13

8f99c5f

wrkode marked this pull request as ready for review March 29, 2023 11:14

flavio reviewed Mar 29, 2023

View reviewed changes

changed escludedNamespaces into a set

aba7eae

flavio reviewed Apr 3, 2023

View reviewed changes

cut first beta - add namespace local to excluded namespaces and remov…

1024727

…e double checking for cattle namespaces

wrkode merged commit 140c2cd into main Apr 3, 2023

wrkode deleted the logging branch April 3, 2023 17:34

		isExcluded := strings.Contains(namespaceName, "cattle") \|\| excludedNamespaces.Contains(namespaceName)

		if isExcluded {

Conversation

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

Uh oh!

Uh oh!

wrkode commented Mar 25, 2023

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!

wrkode Mar 29, 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!

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!

wrkode commented Mar 29, 2023

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!

wrkode commented Mar 29, 2023

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wrkode commented Mar 24, 2023 •

edited

Loading

wrkode Mar 29, 2023 •

edited

Loading