Add ownerRefernce to external check creation #378

NissesSenap · 2020-03-22T14:47:16Z

This doesn't work. Need input.
This is my initial stab at #366

NissesSenap · 2020-03-22T15:03:08Z

Can someone point me in the right direction?
My go skills is still low and so i get lost within channels and interfaces.

I wanted to send the the UID together with name etc. that is needed for the ownerReference meta data to the checks created but from the kuberhealthy I can't manage to find how to get the data from the CRD and then push it towards the objects straight away.

Instead I tried to solve it by pulling the data from the CRD as seen in my PR.
First of all it doesn't work due to missing info (could do some strange for loops to make a educated guess) and it would give unneeded access to the SA and when i write this comment I realize updating the final object from itself sounds like a dumb idea... :)

Any way if someone can point me in the correct direction I think i should be able to solve it.

integrii · 2020-03-26T21:51:53Z

Hey @NissesSenap - Thank you for hacking on this! Unfortunately, I think the current approach may not be quite right.

I think the right place to do this is around here. Basically we want to go into the p variable here and set the right Owner metadata information. According to the docs, the metadata should be at p.ownerReferences (or something similar) and should be of type OwnerReference.

Ultimately, I think that when a pod is programmatically created (anywhere) we should include this owner information. It's really good stuff and I am happy you brought it up. Making this one change would implement this owner information for all checker pods created by Kuberhealthy... Which is awesome.

The next step would be to go through each checker pod and ensure that anytime it creates a pod or daemonset or whatever, it always specifies the right OwnerReference object that points back to the checker pod.

Added crdUID to Checker struct to save the UID created with the CRD.

NissesSenap · 2020-03-27T09:44:09Z

Man I don't know how many hours i was looking for something like createPod() but ofc i should have looked in the pkg and not in the cmd/kuberhealthy...
I did a force push so i removed the old code from my branch so i didn't have to recreated the PR.

I'm not sure I found the correct path this time ether but it's definitely closer.
Now i can see a ownerRefernece in the created objects but they point back to there own controller which makes sense but i guess i hoped it would append to the list and be able to still refer back to the kuberhealthy deployment.

Or I still haven't been able to wrap my head around it ^^
Anyway need to do some other things now but please have a look and I will try to get some more time for this soon.

integrii · 2020-03-31T01:10:22Z

Thanks @NissesSenap ! I will take a look at this one tomorrow... I didn't get enough time to investigate your PR today.

integrii · 2020-04-02T00:33:31Z

Have you by any chance built/tested this one yet on one of your own clusters? Our tests are almost non-existent due to how hard it is to spin up, tear down, and instrument a cluster for testing.

If you have tested it, what version is that cluster?

NissesSenap · 2020-04-06T09:13:25Z

Yes I have tried it but it dosen't do the trick the ownerRef gets overwritten by the deployment, I will need to take a look at it again, but currently i need to prioritize other things.
If you or someone else have time to look at it please go for it.

I run Kubernetes Version: v1.16.2 (Openshift version 4.3.0)

joshulyne · 2020-04-06T18:55:28Z

pkg/checks/external/main.go

@@ -119,6 +123,7 @@ func New(client *kubernetes.Clientset, checkConfig *khcheckcrd.KuberhealthyCheck
 		OriginalPodSpec:          checkConfig.Spec.PodSpec,
 		PodSpec:                  checkConfig.Spec.PodSpec,
 		KubeClient:               client,
+		crdUID:                   checkConfig.GetObjectMeta().GetUID(),


seems like instead of the khcheck crdUID, you want to reference the kuberhealthy deployment UID? im not sure we have something in the code that gets the kuberhealthy deployment UID... @integrii ?

may need to add a function somewhere that finds the kuberhealthy deployment and gets the UID from it?

I would think that the external checker pods that get spun up would have an owner reference that points to the kuberhealthy pod that spawned them (not the deployment or khcheck). This way, if the kuberhealthy instance that is orchestrating the check disappears for some reason, the checker pods it was orchestrating (and had stateful data for) are also removed.

I think this means we can probably drop all the crdUID property stuff and just simply put in the hostname of the kuberhealthy instance that spawns the pod in the owner reference.

So I will finally take a third stab at this and I will do as you say just point back at the kuberhealthy pod as the owner. I hopefully have something to push later today.

…fDeployment

Don't try to point to the CRD:s, instead point the owenerRefernece to the operator. Don't take in to account that the operator pod can be removed.

NissesSenap · 2020-04-13T15:49:45Z

pkg/checks/external/main.go

+		p.OwnerReferences = []metav1.OwnerReference{
+			{
+				APIVersion: "v1",
+				Kind:       "Pod",


I tried to find a way to not hardcode the APIVersion and Kind but I was unable to do so.
I'm sure there is a nice way through the k8s core api, if you know it please point it out.

Tried with apiv1.Pod{}.APIVersion but this just returns an empty string

NissesSenap · 2020-04-13T15:52:49Z

I cleaned the CRD stuff as pointed out and put it towards the kuberhealthy master pod.
I seen it work in my own setup, if you want to give it a try you can use: quay.io/nissessenap/kuberhealthy:2.1.3

NissesSenap · 2020-04-14T07:13:48Z

Was thinking about remaking my function a bit and make it global so the other cmd could use it to reference to them self thus getting a ownerReference on the DS/deployment etc that they create.

But the none of the checks currently just imports comcast/kuberhealthy/v2/pkg/checks/external
so it dosen't make sense. Not that the same function is hard to write but boring to have the exact same code in all the checks.
Could add it in kubeClient but it dosen't feel super natural.
Could create a util or something like that?
All we need is a KubeClient and namespace and those two things is something the checks always have.
Do you have a good idea on where to put a globally scope function like this?

joshulyne · 2020-04-14T17:59:21Z

Hmm I've been looking through that too, where would be a good place for this. So I know all checks import the external/checkclient package since it provides functions for external checks to report back to Kuberhealthy. I'm thinking that could be a spot, but its definitely not perfect. I would say there for now, unless others have an issue with that. @jonnydawg? @integrii is on paternal leave 🎉 rn so he may be slower to respond :P

Both @jonnydawg and I can try importing that function in the daemonset and deployment check once you have it implemented so we can start adding ownerReferences to those checks!

joshulyne · 2020-04-14T18:00:48Z

pkg/checks/external/main.go

+	}
+	return kHealthyPod, nil
+}
+


wondering if this could all be combined to a single function addOwnerReference where it takes in the kube client and namespace (like you mentioned with making this more of a global function) and returns an ownerReference.

Got an issue with circular imports so i created a separate package that i called utils and made it too two functions where ownerRef calls on the get pod one. Please have a look at the latest commit

This to make it reachable from the clients.

external/client needs the external packages. For now I have the exact same code in both files. This way it still works and the external checkers can still get ownerRef in a easy way.

This way we only need 1 function to get ownerRef

NissesSenap · 2020-04-15T08:14:48Z

Due to circular imports I created a new package that i called uitls, maybe not the best name.

It won't be any issue for you to call this package from the different checks.
If you don't like this solution we can ofc have the same code base in two places.

Hopefully you think this looks okay. I can also have a mock fest and try to write some test for my very small package.

pkg/checks/external/checkclient/main.go

joshulyne · 2020-04-15T18:27:24Z

pkg/checks/external/main.go

+	} else {
+		// Sets OwnerRefernces on all checker pods
+		p.OwnerReferences = ownerRef
+	}


Hmm after @jonnydawg and I look through this a bit, I believe we want to return the error instead of just logging it

My thought was that it isn't worth not creating the pod just because you can't find the ownerRef so that's why I did a log instead of a return.

But I have updated the code to return the error and removed the else.

joshulyne · 2020-04-15T18:30:49Z

Overall, I think this looks good! I like how its in a separate util package. I think as long as we bubble up the error we should be good to go! @jonnydawg and I should be able to import this package to the ds and deployment checks 👍 thanks so much!

go.mod

Removing some old constants and fixing go.mod

pkg/checks/external/util/main.go

joshulyne · 2020-04-17T21:06:32Z

Also, tested this locally, and it worked well! When I delete the kuberhealthy instance responsible for all the external checks, all the completed external checks were deleted -- super helpful for cleanup purposes :)

integrii self-requested a review March 26, 2020 21:38

Add ownerRefernce part of external check creation

ab39fa9

Added crdUID to Checker struct to save the UID created with the CRD.

NissesSenap force-pushed the ownerRefDeployment branch from dd56582 to ab39fa9 Compare March 27, 2020 09:32

NissesSenap changed the title ~~Add ownerRefernce to daemonset-check~~ Add ownerRefernce to external check creation Mar 27, 2020

joshulyne reviewed Apr 6, 2020

View reviewed changes

NissesSenap added 2 commits April 13, 2020 12:02

Merge branch 'master' of github.com:Comcast/kuberhealthy into ownerRe…

05ff6fd

…fDeployment

Add ownerRef to kuberhealthy operator to checks

7a16789

Don't try to point to the CRD:s, instead point the owenerRefernece to the operator. Don't take in to account that the operator pod can be removed.

NissesSenap commented Apr 13, 2020

View reviewed changes

joshulyne reviewed Apr 14, 2020

View reviewed changes

NissesSenap added 3 commits April 15, 2020 09:33

Move ownerRef to checkclient

48c8fb5

This to make it reachable from the clients.

Cirkular imports of external/client

d97abb3

external/client needs the external packages. For now I have the exact same code in both files. This way it still works and the external checkers can still get ownerRef in a easy way.

Migrate to util

0cd6a4e

This way we only need 1 function to get ownerRef

joshulyne reviewed Apr 15, 2020

View reviewed changes

pkg/checks/external/checkclient/main.go Outdated Show resolved Hide resolved

joshulyne reviewed Apr 15, 2020

View reviewed changes

jonnydawg requested changes Apr 15, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

NissesSenap added 2 commits April 16, 2020 09:13

Cleaning up the PR

6692255

Removing some old constants and fixing go.mod

Return error instead of log in createPod()

a57ea74

NissesSenap requested a review from jonnydawg April 16, 2020 07:23

joshulyne reviewed Apr 17, 2020

View reviewed changes

pkg/checks/external/util/main.go Outdated Show resolved Hide resolved

jonnydawg approved these changes Apr 17, 2020

View reviewed changes

Add comment & changing name to getKuberhealthyPod

9e0b117

joshulyne merged commit 6533108 into kuberhealthy:master Apr 20, 2020

joshulyne mentioned this pull request Apr 30, 2020

MOD:fix ownerRef bug && create ownerRef for khcheck-pod khstate to khcheck #438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ownerRefernce to external check creation #378

Add ownerRefernce to external check creation #378

NissesSenap commented Mar 22, 2020

NissesSenap commented Mar 22, 2020

integrii commented Mar 26, 2020

NissesSenap commented Mar 27, 2020

integrii commented Mar 31, 2020

integrii commented Apr 2, 2020 •

edited

NissesSenap commented Apr 6, 2020

joshulyne Apr 6, 2020

integrii Apr 6, 2020

NissesSenap Apr 13, 2020

NissesSenap Apr 13, 2020

NissesSenap Apr 14, 2020

NissesSenap commented Apr 13, 2020

NissesSenap commented Apr 14, 2020

joshulyne commented Apr 14, 2020

joshulyne Apr 14, 2020

NissesSenap Apr 15, 2020

NissesSenap commented Apr 15, 2020

joshulyne Apr 15, 2020

NissesSenap Apr 16, 2020

joshulyne commented Apr 15, 2020 •

edited

joshulyne commented Apr 17, 2020

Add ownerRefernce to external check creation #378

Add ownerRefernce to external check creation #378

Conversation

NissesSenap commented Mar 22, 2020

NissesSenap commented Mar 22, 2020

integrii commented Mar 26, 2020

NissesSenap commented Mar 27, 2020

integrii commented Mar 31, 2020

integrii commented Apr 2, 2020 • edited

NissesSenap commented Apr 6, 2020

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

NissesSenap commented Apr 13, 2020

NissesSenap commented Apr 14, 2020

joshulyne commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NissesSenap commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshulyne commented Apr 15, 2020 • edited

joshulyne commented Apr 17, 2020

integrii commented Apr 2, 2020 •

edited

joshulyne commented Apr 15, 2020 •

edited