-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
Signed-off-by: Johannes Kleinlercher <johannes@kleinlercher.at>
[ x ] Tested this does not break existing k8s clusters ( non openshift ) |
Value: "/k8sgpt-config/", | ||
}, | ||
{ | ||
Name: "XDG_CACHE_HOME", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
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>
I can test it today on my openshift cluster |
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:
config file:
Also result objects in the k8sgpt namespace are created as expected |
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.
|
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. |
@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 |
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 ? |
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
βΉ Additional Information