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.

Sign up for GitHub

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

Jump to bottom

add label to service, name to port, to make it selectable via Service Monitors #2767

Merged
merged 2 commits into from
May 16, 2023
Merged

add label to service, name to port, to make it selectable via Service Monitors #2767

merged 2 commits into from
May 16, 2023

Conversation

noyoshi
Copy link
Contributor

@noyoshi noyoshi commented Apr 6, 2023

Hi! I am trying to monitor my installation of volcano with ServiceMonitors. It looks like since the service is not labeled, we are not able to select it via the ServiceMonitor.

I also added a named port, since I believe that is required.

Here are the docs around the ServiceMonitor.

https://docs.openshift.com/container-platform/4.10/rest_api/monitoring_apis/servicemonitor-monitoring-coreos-com-v1.html

Thanks!

@volcano-sh-bot
Copy link
Contributor

Welcome @noyoshi! It looks like this is your first PR to volcano-sh/volcano 🎉

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 6, 2023
@wangyang0616
Copy link
Member

Welcome to the Volcano community.

You can execute the make update-development-yaml command to synchronize the update of installer/helm/chart/volcano/templates/scheduler.yaml to the installer/volcano-development.yaml file, which can ensure helm It is consistent with the deployment information of yaml, thanks.

@hwdef
Copy link
Member

hwdef commented Apr 7, 2023

can you use podmonitor?

@noyoshi
Copy link
Contributor Author

noyoshi commented Apr 7, 2023

@hwdef I wasn't able to use a pod monitor since the pod doesn't have ports defined.

@hwdef
Copy link
Member

hwdef commented Apr 9, 2023

@hwdef I wasn't able to use a pod monitor since the pod doesn't have ports defined.

Please follow the tips above to modify your pr and fix DCO CI

@noyoshi
Copy link
Contributor Author

noyoshi commented Apr 10, 2023

@hwdef done!
@wangyang0616 done!

Thanks for the help :)

@wangyang0616
Copy link
Member

/ok-to-test

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 11, 2023
@wangyang0616
Copy link
Member

Please help to trigger CI in the background, thanks.
cc @william-wang

@wangyang0616
Copy link
Member

/priority important-soon

@volcano-sh-bot volcano-sh-bot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 18, 2023
hwdef
hwdef reviewed Apr 22, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

please trigger the CI to run

@noyoshi
Copy link
Contributor Author

noyoshi commented Apr 24, 2023

Is triggering the CI something that I should be doing :) Thanks!

@wangyang0616
Copy link
Member

Is triggering the CI something that I should be doing :) Thanks!

The first PR in the community needs the maintainer to help trigger the operation of CI.

Can you help trigger CI, Thanks! @william-wang

hwdef
hwdef reviewed Apr 25, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. do-not-merge/contains-merge-commits and removed lgtm Indicates that a PR is ready to be merged. labels Apr 25, 2023
@noyoshi
Copy link
Contributor Author

noyoshi commented Apr 26, 2023

@hwdef I updated off master, so I think you might need to add your approval again to re-trigger CI. Thanks!

@noyoshi
add label to service, name to port, to make it selectable via Service…
cb009b7 
…Monitors

Signed-off-by: Noah Yoshida <noahcy117@gmail.com>
@noyoshi
run make
6657eb6 
Signed-off-by: Noah Yoshida <noahcy117@gmail.com>
@noyoshi
Copy link
Contributor Author

noyoshi commented Apr 26, 2023

Ah I see, removed the merge commit

@hwdef
Copy link
Member

hwdef commented Apr 27, 2023

@william-wang please retrigger the CI.

@noyoshi
Copy link
Contributor Author

noyoshi commented May 8, 2023

Would love to get this in if possible :) Thanks! @william-wang

@hwdef
Copy link
Member

hwdef commented May 9, 2023

/assign @Thor-wl @shinytang6

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2023
@noyoshi
Copy link
Contributor Author

noyoshi commented May 15, 2023

Do we still need someone to trigger ci?

@hwdef
Copy link
Member

hwdef commented May 16, 2023

Do we still need someone to trigger ci?

yes, @william-wang please trigger the CI.

william-wang
william-wang approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2023
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shinytang6, william-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [shinytang6,william-wang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Yikun
Copy link
Member

Yikun commented May 16, 2023

/lgtm

@volcano-sh-bot volcano-sh-bot merged commit a4a8838 into volcano-sh:master May 16, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwdef hwdef hwdef left review comments

@william-wang william-wang william-wang approved these changes

@shinytang6 shinytang6 shinytang6 approved these changes

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu

@Yikun Yikun Awaiting requested review from Yikun

Assignees

@Yikun Yikun

@william-wang william-wang

@hwdef hwdef

@shinytang6 shinytang6

@Thor-wl Thor-wl

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@noyoshi @volcano-sh-bot @wangyang0616 @hwdef @Yikun @william-wang @shinytang6 @Thor-wl