[POA-2928] Fixes discovered during testing #80

mudit-postman · 2025-02-14T11:08:23Z

No description provided.

- Add logs before each process start - Move default interval time to daemonset module:

- Handle case if any state change is allowed in changePodTrafficMonitorState() - if condition in addPodArgsToMap() is opposite also not this func will return error

…rt apidump process in next healthcheck run

Irrelevant

- telemetry api erroring our due to nil resp var - already deleted pod is not getting removed during pod health check

…bs/postman-insights-agent into mudit/poa-2609-testing-changes

mgritter

Approved, but I am uncertain of the resourceVersion changes.

mgritter · 2025-02-19T18:21:33Z

cmd/internal/kube/daemonset/daemonset.go

-		for {
-			select {
-			case received := <-sig:
-				printer.Stderr.Infof("Received %v, stopping daemonset...\n", received.String())
-				break DoneWaitingForSignal
-			}
+		for received := range sig {
+			printer.Stderr.Infof("Received %v, stopping daemonset...\n", received.String())
+			break DoneWaitingForSignal


Not sure what's going on here, but if you don't have the nested for/select then you don't need the named break

mgritter · 2025-02-19T18:48:16Z

cmd/internal/kube/daemonset/pods_healthcheck_worker.go

+	}
+
+	if podArgs.PodTrafficMonitorState == PodDetected || podArgs.PodTrafficMonitorState == PodInitialized {
+		printer.Debugf("Apidump process not started for pod %s during it's initialization, starting now\n", podArgs.PodName)


Suggested change

printer.Debugf("Apidump process not started for pod %s during it's initialization, starting now\n", podArgs.PodName)

printer.Debugf("Apidump process not started for pod %s during its initialization, starting now\n", podArgs.PodName)

mgritter · 2025-02-19T18:55:35Z

integrations/kube_apis/kube_apis.go

 	watcher, err := kc.Clientset.CoreV1().Events("").Watch(context.Background(), metaV1.ListOptions{
-		Watch:         true,
-		FieldSelector: "involvedObject.kind=Pod",
+		Watch:           true,
+		FieldSelector:   "involvedObject.kind=Pod",
+		ResourceVersion: pod.ResourceVersion,


From the Kubernetes docs it sounds like the intended use is to perform a List and then watch with the resourceVersion returned from that List. So you did that with the pod itself; but isn't the more natural thing to use the resourceVersion associated with the list operation we do over all pods, on startup? Then we wouldn't have to worry about the semantics being different in some unexpected way.

I am uncomfortable with the assumption that this pod's resourceVersion represents a point in time that serves as an anchor. I'm not convinced that is supported by the documentation, but I admit I don't understand how the Pod and list versions differ.

Should we be using a ListMeta field? Can we even get that via the ClientSet?

v1.meta/ListMeta - The metadata.resourceVersion of a resource collection (the response to a list) identifies the resource version at which the collection was constructed.

Another things that came up is that it doesn't look like we have any facilities for restart. Is that all handled internally by the Watch method?

If a client watch is disconnected then that client can start a new watch from the last returned resourceVersion; the client could also perform a fresh get / list request and begin again.

I am uncomfortable with the assumption that this pod's resourceVersion represents a point in time that serves as an anchor.

I guess the variable name pod is confusing (will change it to pods), I am using the resourceVersion returned from the listObject, the func I am using is .List() not .Get()

Another things that came up is that it doesn't look like we have any facilities for restart. Is that all handled internally by the Watch method?

They have some retry mechanism internally (ref), but not sure how reliable it is

Okay we have this for retry, can add as a follow-up

We are using retryWatcher now with this commit 1eb9960

mgritter · 2025-02-19T18:57:11Z

integrations/tests/kube_cri_apis/main.go

+				printer.Infof("Event data: %s\n", string(jsonData))
+			}
+		default:
+			printer.Infof("Unhandled event type: %v\n", event.Type)


This seems like it could be very high-rate, should it be Debug? Or throttled in some other way? Or eliminate common cases that we know occur?

This is test file, so Infof should be okay.

Child PRs: #67, #69, #71, #72, #73, #74, #75, #76, #77, #78, #79, #80, #82

mudit-postman added 11 commits February 14, 2025 15:36

Chores:

49306e9

- Add logs before each process start - Move default interval time to daemonset module:

chore: change StateChangeMutex to sync.Mutex instead to pointer

38dcb25

chore: change PodTrafficMonitorState from int to string

317c5e1

chore: pass pointer of mutext parent and run go mod tidy

79d2b4e

chore: use for range instead of single switch case

4fe1937

chore: set revisionVersion to only fetch events after agent creation

2b63162

fix: parent run command starting in separate go routine

0335c52

Fixes:

b8f9005

- Handle case if any state change is allowed in changePodTrafficMonitorState() - if condition in addPodArgsToMap() is opposite also not this func will return error

chore: if pod is already running but agent is not listening to it sta…

aa932f6

…rt apidump process in next healthcheck run

fix: prepend '/host' to network namespace

d11593e

fix: pass learnClient in util.NewLearnSession()

7718c43

Boomtokn previously approved these changes Feb 17, 2025

View reviewed changes

mudit-postman added 4 commits February 17, 2025 11:27

chore: set default apidump args

2168fc4

fix:

3811e45

- telemetry api erroring our due to nil resp var - already deleted pod is not getting removed during pod health check

fix:

295b355

- telemetry api erroring our due to nil resp var - already deleted pod is not getting removed during pod health check

Merge branch 'mudit/poa-2609-testing-changes' of github.com:postmanla…

a9075e0

…bs/postman-insights-agent into mudit/poa-2609-testing-changes

mudit-postman requested a review from mgritter February 18, 2025 14:03

mudit-postman marked this pull request as ready for review February 18, 2025 14:03

mgritter approved these changes Feb 19, 2025

View reviewed changes

chore: address PR comments

8326f15

mudit-postman merged commit dcb0bab into mudit/poa-2609 Feb 19, 2025
2 checks passed

mudit-postman deleted the mudit/poa-2609-testing-changes branch February 19, 2025 20:23

mudit-postman added a commit that referenced this pull request Feb 19, 2025

[POA-2609] Add k8s and CRI sub-modules and function interfaces (#66)

6027135

Child PRs: #67, #69, #71, #72, #73, #74, #75, #76, #77, #78, #79, #80, #82

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POA-2928] Fixes discovered during testing #80

[POA-2928] Fixes discovered during testing #80

mudit-postman commented Feb 14, 2025

mgritter left a comment

mgritter Feb 19, 2025

mgritter Feb 19, 2025

mgritter Feb 19, 2025

mudit-postman Feb 19, 2025

mudit-postman Feb 19, 2025

mudit-postman Feb 19, 2025

mgritter Feb 19, 2025

mudit-postman Feb 19, 2025

	printer.Debugf("Apidump process not started for pod %s during it's initialization, starting now\n", podArgs.PodName)
	printer.Debugf("Apidump process not started for pod %s during its initialization, starting now\n", podArgs.PodName)

[POA-2928] Fixes discovered during testing #80

[POA-2928] Fixes discovered during testing #80

Conversation

mudit-postman commented Feb 14, 2025

mgritter left a 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