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

Fix webhook typo, add podLabels, add podEnv to flyte-core Helm chart #4756

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

ddl-ebrown
Copy link
Contributor

@ddl-ebrown ddl-ebrown commented Jan 22, 2024

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

Hopefully this isn't too controversial a change. To deploy in our environments, we've standardized on the use of podAnnotations, podLabels and podEnv helm values. Globally, this allows for setting values across all deployment, statefulset and daemonset resources.

What changes were proposed in this pull request?

Flyte already has podAnnotations, but not podLabels or podEnv. This PR adds both to flyte-core.

Of not, flyteadmin has a env: [] value set already, but it's the only one. I didn't want to break anything so left it as-is. Ideally I think that would become podEnv: {} and would render in the same way as the other charts.

How was this patch tested?

make helm was used locally to generate charts / docs. I set values explicitly in the values.yaml to verify correct rendering

i.e.

podEnv:
  - name: TEST
    value: value

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

welcome bot commented Jan 22, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: ddl-ebrown <[email protected]>
@ddl-ebrown ddl-ebrown force-pushed the add-pod-config branch 4 times, most recently from 36bf642 to 392e02b Compare January 22, 2024 22:03
@ddl-ebrown ddl-ebrown marked this pull request as ready for review January 22, 2024 22:08
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Jan 22, 2024
@ddl-ebrown
Copy link
Contributor Author

I tested setting the podLabels / podEnv, ran make helm locally and generated some templates / made sure they were correct

@ddl-ebrown ddl-ebrown changed the title Fix webhook typo, add podLabels, add podEnv Fix webhook typo, add podLabels, add podEnv to flyte-core Helm chart Jan 22, 2024
 - podLabels is a standard pod value often needed to introduce
   user-customizable labels to pods

Signed-off-by: ddl-ebrown <[email protected]>
 - podEnv is a standard Helm addition used to add additional environment
   variables to pods

 - NOTE: flyteadmin already defined a `env: []`, so no podEnv was added
   there was it would be a breaking change

Signed-off-by: ddl-ebrown <[email protected]>
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5300e1b) 58.17% compared to head (174dd91) 58.18%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4756      +/-   ##
==========================================
+ Coverage   58.17%   58.18%   +0.01%     
==========================================
  Files         626      626              
  Lines       53833    53833              
==========================================
+ Hits        31315    31322       +7     
+ Misses      20010    20003       -7     
  Partials     2508     2508              
Flag Coverage Δ
unittests 58.18% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ddl-ebrown
Copy link
Contributor Author

@davidmirror-ops any chance you would be able to have a look at this one? Thanks!

Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

Thanks @ddl-ebrown!
This is very helpful not only for consistent config but eventually even for observability use cases I think. I'm also curious about the actual use case that led your org to standardize on these keys.

BTW, please consider signing up for the weekly community raffle, this contribution would give you points!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 26, 2024
@davidmirror-ops davidmirror-ops merged commit fb9ffd5 into flyteorg:master Jan 26, 2024
47 checks passed
@ddl-ebrown ddl-ebrown deleted the add-pod-config branch January 27, 2024 00:49
@ddl-ebrown
Copy link
Contributor Author

Cool thanks @davidmirror-ops!

We use annotations specifically for things like Istio and Prometheus. Clearly, they're useful for many different operators.

We primarily use labels for configuring network policy. So pods marked with foo-client are allowed to talk to pods using a pretty basic selector matching mechanism.

podEnv can be injected at deployment time using values from the Helmfile config. I'm actually not sure what we're using those for, but it is a standard that we adopted.

ddl-ebrown added a commit to ddl-ebrown/flyte that referenced this pull request Jan 31, 2024
 - In flyteorg#4756 / fb9ffd5, flyte-core got
   consistent podEnv values established in values.yaml. However, these
   values were not properly injected into *all* the containers being
   used in various deployments.

   Fix that so that they are used in all deployments

Signed-off-by: ddl-ebrown <[email protected]>
pingsutw pushed a commit that referenced this pull request Feb 6, 2024
* Flyte-core add missing podEnv values

 - In #4756 / fb9ffd5, flyte-core got
   consistent podEnv values established in values.yaml. However, these
   values were not properly injected into *all* the containers being
   used in various deployments.

   Fix that so that they are used in all deployments

Signed-off-by: ddl-ebrown <[email protected]>

* Flyte-core chart prevent empty env:

 - Some linters consider empty env: as invalid k8s YAML, because env is
   typically an [] when no values are set

   Prevent rendering the console env block without values

Signed-off-by: ddl-ebrown <[email protected]>

---------

Signed-off-by: ddl-ebrown <[email protected]>
ddl-ebrown added a commit to ddl-ebrown/flyte that referenced this pull request Jun 20, 2024
 - Previously, the webhook was sharing some pod level settings in the
   core chart with flytepropeller like:

   * podAnnotations
   * podEnv
   * podLabels
   * nodeSelector

   Since the webhook runs a separate pod, it should have separate
   settings

 NOTE: no attempt is made to honor carrying over any previous settings
 from flytepropeller values to webhook values, but given these were
 recently introduced / fixed in January as part of
 flyteorg#4756, I think they're not used
 very much

Signed-off-by: ddl-ebrown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants