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

chore: add managed by label to namespace #604

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

bthari
Copy link
Contributor

@bthari bthari commented Sep 6, 2024

Description

When creating a namespace object in Kubernetes, add label “{prefix}/managed: true”. This will help when we want to modify resources created by CaraML services in a shared cluster, e.g. configuring log for the managed namespace only

By default the namespace prefix would caraml.dev/, and it can be configured in the config yaml

Modifications

  • As the creation of label for Kubernetes’ object in Merlin previously is bind to metadata struct, while namespace doesn’t have metada, in this changes the functionality of labelling (labeller) is moved to it’s own package
  • Add one more prefix in InitKubernetesLabeller initiation
  • Add the {prefix}/managed: true when creating namespace

Tests

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes


Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.70%. Comparing base (fa928c3) to head (f9140f5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
- Coverage   60.73%   60.70%   -0.03%     
==========================================
  Files         274      275       +1     
  Lines       22150    22171      +21     
==========================================
+ Hits        13452    13460       +8     
- Misses       7835     7847      +12     
- Partials      863      864       +1     
Flag Coverage Δ
api-test 58.68% <ø> (-0.03%) ⬇️
sdk-test-3.10 75.51% <ø> (ø)
sdk-test-3.8 75.49% <ø> (ø)
sdk-test-3.9 75.49% <ø> (ø)

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.

@bthari bthari added the enhancement New feature or request label Sep 6, 2024
@bthari bthari self-assigned this Sep 6, 2024
@bthari bthari marked this pull request as ready for review September 6, 2024 09:55
Copy link
Contributor

@tiopramayudi tiopramayudi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @bthari

@bthari bthari merged commit a9184ed into main Sep 12, 2024
34 of 36 checks passed
@bthari bthari deleted the add-label-to-namespace branch September 12, 2024 03:50
Copy link
Contributor

@deadlycoconuts deadlycoconuts left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 Just left a couple of comments but everything looks good! Thanks for the refactoring!

api/cluster/labeller/labeller.go Show resolved Hide resolved
Comment on lines -108 to +70
if _, usingReservedKeys := reservedKeys[prefix+label.Key]; usingReservedKeys {
if labeller.IsReservedKey(labeller.GetPrefix() + label.Key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hang on if we add the prefix to the label key, e.g. gojek.com/ + team, and look up its result in the reservedKeys map, we won't be able to find gojek.com/team in the map right? Since the map contains the label key values only i.e. team. Was this even working before? 😦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this will only work if the prefix is "" (but it can't be ""). Was this working before? I honestly don't think so 😅 but Turing also work the same way, so I just keep the functionality as is

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm alright I'll create a card for this in the backlog so that we can fix this in the future. Thanks for clarifying!

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

Successfully merging this pull request may close these issues.

3 participants