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: results are labeled #264

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

prometherion
Copy link
Contributor

@prometherion prometherion commented Nov 15, 2023

Closes #263

πŸ“‘ Description

The following PR labels the Result objects with some additional labels, such as:

  • k8sgpts.k8sgpt.ai/name, referring to the k8sgpt resource's Name
  • k8sgpts.k8sgpt.ai/namespace, referring to the k8sgpt resource's namespace
  • if specified, k8sgpts.k8sgpt.ai/backend, referring to the k8sgpt used AI driver

βœ… 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

N.R.

@prometherion prometherion requested review from a team as code owners November 15, 2023 16:22
@prometherion
Copy link
Contributor Author

Tested locally, and this is an example output.

apiVersion: core.k8sgpt.ai/v1alpha1
kind: Result
metadata:
  creationTimestamp: "2023-11-15T16:34:09Z"
  generation: 2
  labels:
    k8sgpts.k8sgpt.ai/backend: openai
    k8sgpts.k8sgpt.ai/name: k8sgpt-sample
    k8sgpts.k8sgpt.ai/namespace: k8sgpt-operator-system
  name: defaultnginx
  namespace: k8sgpt-operator-system
  resourceVersion: "301482"
  uid: ce9f370f-38e6-4c65-833b-355bda35783b
spec:
  backend: openai
  details: "Error: The deployment \"default/nginx\" only has 1 replica available,
    but there are 2 replicas available.\n\nSolution: \n1. Determine the current state
    of the deployment by running the command: \"kubectl get deployments\"\n2. Scale
    up the deployment by running the command: \"kubectl scale deployment default/nginx
    --replicas=2\"\n3. Verify the deployment has been scaled up by running the command:
    \"kubectl get deployments\"\n4. Check the status of the pods by running the command:
    \"kubectl get pods\"\n5. If the pods are still not available, troubleshoot the
    issue further by checking the logs of the pods using the command: \"kubectl logs
    <pod_name>\"\n6. Repeat steps 4 and 5 until the desired state is achieved."
  error:
  - sensitive:
    - masked: W1Q3UEhYWw==
      unmasked: default
    - masked: YlspM0o=
      unmasked: nginx
    text: Deployment default/nginx has 1 replicas but 2 are available
  kind: Deployment
  name: default/nginx
  parentObject: ""
status:
  lifecycle: historical

@arbreezy
Copy link
Member

Hey @prometherion many thanks for the PR and your interest in the project, a few comments

  • In case you deploy two K8sGPT resources, the operator will always use the latter and will only create one k8sgpt deployment. I don't see much value from adding a label of the CR name unless you have multiple K8sGPT operators. Eager to get your thoughts on this nevertheless maybe you have a use case I haven't though about..
  • We have the backend's value in Result's spec but I think filtering with a label, is nice in case you change AI backend
  • Result's name includes the namespace and the analysed resource names but maybe it will make sense having these as labels, although from your code you are setting labels like,
  • K8sGPT's namespace which will be always the same with the namespace Results are getting created, so I believe there is no value adding this again as a label
  • Results should be constructed inside MapResults func, while keeping as flat as possible the CreateOrUpdate logic
  • Adding more labels should honour the backstage labels support, which now is overriding the annotation we are setting. You can test it by enabling backstage in your CR,
extraOptions:
    backstage:
      enabled: true

@prometherion
Copy link
Contributor Author

prometherion commented Nov 16, 2023

  • Eager to get your thoughts on this nevertheless maybe you have a use case I haven't though about.

Well, I was planning to introduce changes gradually, but since you're asking, I'll go for it.

I'm working on a platform that is managing Kubernetes clusters at scale thanks to Cluster API.
The idea is to offer several sets of tools, and k8sgpt is among them to assist in troubleshooting.

Before jumping in, just a small appendix on the jargon used by Cluster API: the management cluster is the one collecting all the CAPI manifests, and the workload clusters are the deployed ones on different providers, made of their own Control Planes, and worker nodes.

As you pointed out correctly, the current Operator will create a Deployment and watch for the events from the current cluster: so it means that having multiple instances of the k8sgpt resource is not helpful, since the will be always a single k8sgpt-deployment Deployment.

I was planning to introduce a new set of changes where the k8sgpt could track changes in remote clusters, thanks to the CAPI-provided kubeconfig, and the whole label selection plays a huge role in the context of the features I'm designing. I'll try to get it through of it, by providing a sort of user story.

The k8sgpt Namespace resource is useful to overcome some limitations of the Kubernetes field selector which is just supporting equality and not In operators.
Let's say I have 3 identical clusters but in different environments such as dev, staging, and production.
These environments are mapped into different Namespaces to simplify RBAC and other policies.

The user would like to see if the reported issue is affecting all the clusters, or just a small subset of them, so using a selection such as k8sgpts.k8sgpt.ai/namespace in (dev, staging, production) would help in getting a comprehensive list of the issues of all my owned clusters, without adding any additional map-reduce filtering.

I was planning to discuss about it, as well as a proof of concept already running in our test environment to showcase how the troubleshooting is performed without installing the k8sgpt-deployment: as I reported in the issue, unfortunately, the slack workspace invitation is not working for me, and that's the reason why I'm also trying to reach out to you on LinkedIn and, as well, by explaining my plans for the mid-term although in a GH issue which is out-of-topic πŸ™ƒ

@AlexsJones
Copy link
Member

Thanks for contributing @prometherion and sharing your ideas. I dont see anything here that worries me.
Do you want to try this new slack link and let me know if it works for you https://join.slack.com/t/k8sgpt/shared_invite/zt-276pa9uyq-pxAUr4TCVHubFxEvLZuT1Q ?

pkg/resources/results.go Outdated Show resolved Hide resolved
@prometherion prometherion force-pushed the issues/263 branch 3 times, most recently from f137b5b to 68a0d1f Compare November 16, 2023 14:18
Signed-off-by: Dario Tranchitella <dario@tranchitella.eu>
Copy link
Member

@arbreezy arbreezy left a comment

Choose a reason for hiding this comment

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

LGTM

@arbreezy arbreezy merged commit d3f7b9d into k8sgpt-ai:main Nov 16, 2023
6 checks passed
@prometherion prometherion deleted the issues/263 branch November 16, 2023 15:02
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]: Labeled results.core.k8sgpt.ai resources
3 participants