Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support arbitrary uid for openshift environments #126

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

jkleinlercher
Copy link
Contributor

Closes #125

πŸ“‘ Description

similar to k8sgpt-ai/k8sgpt#454 k8sgpt-operator should create a deployment where XDG_ variables point to an emptyDir volume so any uid can run this container.

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

Signed-off-by: Johannes Kleinlercher <johannes@kleinlercher.at>
@jkleinlercher jkleinlercher marked this pull request as ready for review May 26, 2023 21:55
@jkleinlercher jkleinlercher requested review from a team as code owners May 26, 2023 21:55
@AlexsJones
Copy link
Member

[ x ] Tested this does not break existing k8s clusters ( non openshift )
[ ] Test with OpenShift

Value: "/k8sgpt-config/",
},
{
Name: "XDG_CACHE_HOME",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to have a different path for cache, e.g /k8sgpt-config/.cache and use subPath in the ConfigMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arbreezy what do you think about my last namings? I use k8sgpt-data for the volume mountpath and .config and .cache sub directories

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@arbreezy
Copy link
Member

I can't run minishift in my local env and have no access to an Openshift env at the moment to test the PR.

Signed-off-by: Johannes Kleinlercher <johannes@kleinlercher.at>
@jkleinlercher
Copy link
Contributor Author

I can't run minishift in my local env and have no access to an Openshift env at the moment to test the PR.

I can test it today on my openshift cluster

@jkleinlercher
Copy link
Contributor Author

jkleinlercher commented Jun 1, 2023

I built the image based on https://github.com/jkleinlercher/k8sgpt-operator/tree/feat/arbitrary-uid locally and deployed it on my openshift test cluster. I added a busybox sidecar image to the k8sgpt-deployment to check if the expected files are created in the emptyDir volume paths. As you can also see the uid of the files are the arbitery uids openshift assigns to the pod.

cache files:

~ $ ls -l /k8sgpt-data/.cache/k8sgpt/
total 160
-rw-------    1 10009800 10009800       452 Jun  1 07:10 010edbef1e54bd9f22a90ff8cdbd30265dccd7215454e7bb822da8d3fda2f985
-rw-------    1 10009800 10009800       612 Jun  1 07:08 036233045d065366d6a8ac063961ac1b7f89619d872af827931ddc1ee0b08306

config file:

~ $ ls -l /k8sgpt-data/.config/k8sgpt/
total 4
-rw-r--r--    1 10009800 10009800       176 Jun  1 07:02 k8sgpt.yaml

Also result objects in the k8sgpt namespace are created as expected

@jkleinlercher
Copy link
Contributor Author

I tested again and now I get the following panic. Don't know if it is related to our code changes or something else currently in the main branch.

2023-06-01T09:42:34Z    INFO    Starting workers        {"controller": "k8sgpt", "controllerGroup": "core.k8sgpt.ai", "controllerKind": "K8sGPT", "worker count": 1}
Creating new client for 10.129.2.12:8080
Connection established between 10.129.2.12:8080 and localhost with time out of 1 seconds.
Remote Address : 10.129.2.12:8080
Local Address : 10.130.2.84:53334
2023-06-01T09:42:46Z    INFO    Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference        {"controller": "k8sgpt", "controllerGroup": "core.k8sgpt.ai", "controllerKind": "K8sGPT", "K8sGPT": {"name":"k8sgpt-sample","namespace":"sx-k8sgpt"}, "namespace": "sx-k8sgpt", "name": "k8sgpt-sample", "reconcileID": "6110b16a-bd41-4483-bcf5-dbf63d09d18c"}
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x174c11a]

goroutine 110 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
        /go/pkg/mod/sigs.k8s.io/controller-runtime@v0.15.0/pkg/internal/controller/controller.go:115 +0x1fa
panic({0x190aec0, 0x2b189c0})
        /usr/local/go/src/runtime/panic.go:884 +0x213

@AlexsJones
Copy link
Member

Hard to know without the full stack trace, any more in the logs?

@jkleinlercher
Copy link
Contributor Author

Hard to know without the full stack trace, any more in the logs?

Unfortunately not, but I can say that it has nothing to do with the changes of this PR. It must be something already in main branch, because this error only occurs after the commit of merging main to this branch.

@arbreezy
Copy link
Member

arbreezy commented Jun 1, 2023

@jkleinlercher , @AlexsJones found the issue, it 's caused from my backstage PR, thankfully we didn't cut a release yet

this conditional will produce the error if you don't specify in a vanilla run the extra options.. we need to check first if the extraOptions is nil.
https://github.com/k8sgpt-ai/k8sgpt-operator/blob/main/controllers/k8sgpt_controller.go#L224

@jkleinlercher
Copy link
Contributor Author

@jkleinlercher , @AlexsJones found the issue, it 's caused from my backstage PR, thankfully we didn't cut a release yet

this conditional will produce the error if you don't specify in a vanilla run the extra options.. we need to check first if the extraOptions is nil. https://github.com/k8sgpt-ai/k8sgpt-operator/blob/main/controllers/k8sgpt_controller.go#L224

Okay since the panic error is clarified and ask myself how to proceed with this PR. As documented in #126 (comment) I think my tests on openshift are fine. As long as I didn't introduced other problems on other K8s clusters (which you were able to test, I think?) I would be very thankful if you can merge this PR? How can I help with doing this @AlexsJones and @arbreezy ?

@AlexsJones AlexsJones merged commit 10484eb into k8sgpt-ai:main Jun 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: support arbitrary uid for openshift environments
3 participants