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

Add flag for ingesting pod UID from KSM #1327

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Conversation

kaelanspatel
Copy link
Contributor

What does this PR change?

Adds a flag .Values.kubecostProductConfigs.ingestPodUID which toggles appending pod UID to pod names in the costmodel. See associated CM PR for more info.

Does this PR rely on any other PRs?

How does this PR impact users? (This is the kind of thing that goes in release notes!)

See CM PR

Links to Issues or ZD tickets this PR addresses or fixes

How was this PR tested?

See CM PR

Have you made an update to documentation?

@kaelanspatel kaelanspatel merged commit dd5d012 into develop Apr 28, 2022
@kaelanspatel kaelanspatel deleted the kaelan-ingest-pod-uid branch April 28, 2022 20:10
@@ -787,3 +787,4 @@ awsstore:
# secretname: productkeysecret # create a secret out of a file named productkey.json of format { "key": "kc-b1325234" }
# mountPath: "/some/custom/path/productkey.json" # (use instead of secretname) declare the path at which the product key file is mounted (eg. by a secrets provisioner). The file must be of format { "key": "kc-b1325234" }
# cloudIntegrationSecret: "cloud-integration"
# ingestPodUID: false # Enables using UIDs to uniquely ID pods. This requires either Kubecost's replicated KSM metrics, or KSM v2.1.0+. This may impact performance, and changes the default cost-model allocation behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

As a user, it's not clear why I would use this... I'm also torn on us provided very detailed notes for very infrequently flags. Did you think about potentially referencing an external docs page?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment above I realized got stuck in a pending state... sorry about that. @kaelanspatel do we have more detail on this change?

cc @AdamStack18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dwbrown2, didn't see this last week.

I definitely thought about documenting this in more depth, but there's a very specific edge case where this is the solution. I talked it over with some people and decided against just inserting detailed notes (like you say). Ofc I'm not particularly happy with this vague comment you see here.

It seems like we have a lot of "trivia" about the app which we just expect people to ask about because it doesn't fit anywhere in the technical docs (e.g. What does this flag do? How is kubecost calculating this edge case? Is X an important warning or can I ignore it?)

Maybe we could have a set of clarification pages that we can add additional documentation to if we need to (like helm-chart-variables.md, edge-cases.md)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you make great points... what do you think about this proposal:

Most flags have a short descriptions, e.g. 1-liner, directly in values.
Then, those that require more detail (most?) have a "Learn more" or "Read more" link.
Those links point to either a helm-chart-vars page or individual pages.

@Adam-Stack-PM
Copy link

Internal Note: Review with future technical writer

cc @AdamStack18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants