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

Service port for performance analyzer #346

Conversation

Phoelsch
Copy link
Contributor

@Phoelsch Phoelsch commented Nov 2, 2022

Signed-off-by: Philipp Hölscher [email protected]

Description

The change adds an additional port to the statefulset and service to expose the performance analyzer.

Issues Resolved

251 - [Enhancement][opensearch]add port 9600 (Performance Analyzer)

Check List

  • [X ] Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • [X ] Helm chart version bumped
  • [X ] Helm chart CHANGELOG.md updated to reflect change

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Phoelsch Phoelsch requested review from a team, TheAlgo and DandyDeveloper as code owners November 2, 2022 11:38
@TheAlgo
Copy link
Member

TheAlgo commented Nov 3, 2022

@Phoelsch Thanks for this PR. Can you please check the contributing guidelines and refactor this PR? Thanks

@Phoelsch
Copy link
Contributor Author

Phoelsch commented Nov 3, 2022

@TheAlgo I checked the guidlines even before creating the pull request. Can please give me a hint what`s missing or wrong?

@TheAlgo
Copy link
Member

TheAlgo commented Nov 3, 2022

@Phoelsch Phoelsch force-pushed the feature/performance_analyser_service branch 2 times, most recently from 6edcad9 to c7439c8 Compare November 3, 2022 20:48
@Phoelsch
Copy link
Contributor Author

Phoelsch commented Nov 3, 2022

@TheAlgo Thanks, commit is now signed.

charts/opensearch/CHANGELOG.md Show resolved Hide resolved
charts/opensearch/values.yaml Show resolved Hide resolved
@Phoelsch Phoelsch force-pushed the feature/performance_analyser_service branch from ecf9611 to 656df3e Compare November 7, 2022 12:47
Copy link
Member

@TheAlgo TheAlgo left a comment

Choose a reason for hiding this comment

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

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

@TheAlgo TheAlgo self-requested a review November 8, 2022 17:41
@Phoelsch
Copy link
Contributor Author

Phoelsch commented Nov 9, 2022

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

The port is set to 9600 as default with values.yaml. If its not explicitly overwritten by the user it will still be 9600. I dont see any problem with that.

@peterzhuamazon
Copy link
Member

I am good with this change @Phoelsch thanks for the contribution.
@prudhvigodithi please take a look on this.

Thanks.

@DandyDeveloper
Copy link
Collaborator

DandyDeveloper commented Nov 13, 2022

I actually wonder if we need to explicitly label this as performanceAnalyzer rather than metric.

Many people uniformally recognize metrics ports as prometheus compliant metrics. Aka, they can be pulled in via prometheus.

Additional, many people may be using the prometheus metric plugin to pull metrics and have probably already added a port called metrics.

Could we change this to be more explicitly labeled as performance analyzer?

@peterzhuamazon
Copy link
Member

We actually plan to remove the coupled dependency between OS and PA process:

@DandyDeveloper
Copy link
Collaborator

@peterzhuamazon Do we have the facilities in the chart to enable and include this for users by removing this?

Aka, can they independently define the plugins, ports etc. Necessary to at least not inadvertedly remove the functionality from our chart entirely?

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Nov 14, 2022

@peterzhuamazon Do we have the facilities in the chart to enable and include this for users by removing this?

Aka, can they independently define the plugins, ports etc. Necessary to at least not inadvertedly remove the functionality from our chart entirely?

Hi @DandyDeveloper

The current change planned in that PR is that if user does not change aanything then behavior is mostly same.
The PA and OS will both start, but this time killing PA process wont bring down OS process anymore.
If use define env var DISABLE_PERFORMANCE_ANALYZER_AGENT_CLI=true then PA will not start during the creation of the container.

User can modify the pa config to change port from 9600 to something else if needed, before starting container.
Thanks.

@TheAlgo
Copy link
Member

TheAlgo commented Nov 19, 2022

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

The port is set to 9600 as default with values.yaml. If its not explicitly overwritten by the user it will still be 9600. I dont see any problem with that.

@Phoelsch But if someone is upgrading with previous values.yaml , will it not break?

@Phoelsch
Copy link
Contributor Author

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

The port is set to 9600 as default with values.yaml. If its not explicitly overwritten by the user it will still be 9600. I dont see any problem with that.

@Phoelsch But if someone is upgrading with previous values.yaml , will it not break?

@TheAlgo If I upgrade an existing deployment to this version the belonging values.yaml with the new default value will be used. How could it happen that the previous values.yaml will be used?

@TheAlgo
Copy link
Member

TheAlgo commented Dec 9, 2022

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

The port is set to 9600 as default with values.yaml. If its not explicitly overwritten by the user it will still be 9600. I dont see any problem with that.

@Phoelsch But if someone is upgrading with previous values.yaml , will it not break?

@TheAlgo If I upgrade an existing deployment to this version the belonging values.yaml with the new default value will be used. How could it happen that the previous values.yaml will be used?

@Phoelsch Someone can still use the old values.yaml and issue an upgrade?

@TheAlgo
Copy link
Member

TheAlgo commented Dec 19, 2022

One small question @Phoelsch if in a chart one does not provide the value for the port? Will it throw error? Will this break the chart?

The port is set to 9600 as default with values.yaml. If its not explicitly overwritten by the user it will still be 9600. I dont see any problem with that.

@Phoelsch But if someone is upgrading with previous values.yaml , will it not break?

@TheAlgo If I upgrade an existing deployment to this version the belonging values.yaml with the new default value will be used. How could it happen that the previous values.yaml will be used?

@Phoelsch Someone can still use the old values.yaml and issue an upgrade?

@Phoelsch Opinions?

@TheAlgo
Copy link
Member

TheAlgo commented Jan 20, 2023

@Phoelsch Can you check the comments wen free so that we can have a closure on this PR?

@prudhvigodithi
Copy link
Member

Hey @Phoelsch this a nice PR to get merged, can you please fix the conflicts so that we can proceed to merge?
Thank you

@Phoelsch
Copy link
Contributor Author

Phoelsch commented Mar 8, 2023

@TheAlgo I'm sorry for the delay. Still I don't get you`re problem und could not reproduce that. We could use a default value within the statefulset template, but that would be awful. The Helm Docs itself says that the values.yaml should be used as I did.

@TheAlgo
Copy link
Member

TheAlgo commented Mar 9, 2023

@TheAlgo I'm sorry for the delay. Still I don't get you`re problem und could not reproduce that. We could use a default value within the statefulset template, but that would be awful. The Helm Docs itself says that the values.yaml should be used as I did.

Yes that might not be the right way. Lets have a minor version upgrade for this. What do you think? Tagging @prudhvigodithi as well

Copy link
Member

@TheAlgo TheAlgo left a comment

Choose a reason for hiding this comment

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

Also lets resolve the conflicts as well! Thank you for this.

@TheAlgo
Copy link
Member

TheAlgo commented Mar 18, 2023

@Phoelsch Any updates on this PR?

@Phoelsch
Copy link
Contributor Author

@Phoelsch Any updates on this PR?

@TheAlgo resolved

Signed-off-by: Peter Zhu <[email protected]>
@prudhvigodithi prudhvigodithi merged commit 69daf97 into opensearch-project:main Mar 22, 2023
@prudhvigodithi prudhvigodithi added the backport 1.x Backport to 1.x branch after merging to main label Mar 22, 2023
@prudhvigodithi
Copy link
Member

Hey @Phoelsch this is a good change for 1.x version as well, can you please backport to 1.x branch?
Thank you

peterzhuamazon added a commit to peterzhuamazon/helm-charts that referenced this pull request Mar 23, 2023
* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
@peterzhuamazon peterzhuamazon mentioned this pull request Mar 23, 2023
3 tasks
peterzhuamazon added a commit that referenced this pull request Mar 23, 2023
* Service port for performance analyzer (#346)

* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>

* Add hostPort support for http- and transport-ports (#336)

Signed-off-by: Christian Kuhn <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: Christian Kuhn <[email protected]>
Co-authored-by: Philipp Hölscher <[email protected]>
Co-authored-by: Christian Kuhn <[email protected]>
prathaptce pushed a commit to prathaptce/helm-charts that referenced this pull request Mar 24, 2023
* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
prathaptce pushed a commit to prathaptce/helm-charts that referenced this pull request Mar 24, 2023
* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>
prathaptce pushed a commit to prathaptce/helm-charts that referenced this pull request Mar 24, 2023
* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>
bbarani added a commit that referenced this pull request Apr 24, 2023
* Update appVersion for 2.4.0 release (#350)

Signed-off-by: Zelin Hao <[email protected]>

Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Add .whitesource configuration file (#353)

Co-authored-by: mend-for-github.aaakk.us.kg[bot] <50673670+mend-for-github.aaakk.us.kg[bot]@users.noreply.github.com>
Signed-off-by: prathaptce <[email protected]>

* Resolve Kind Cluster not able to be built in PR checks (#356)

Signed-off-by: Peter Zhu <[email protected]>

Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Fix the kindest/node docker images versions (#357)

* Resolve Kind Cluster not able to be built in PR checks

Signed-off-by: Peter Zhu <[email protected]>

* Fix the kindest/node versions on docker images

Signed-off-by: Peter Zhu <[email protected]>

Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Resolve Kind Cluster not able to be built in PR checks (#358)

Signed-off-by: Peter Zhu <[email protected]>

Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* allow adding plugins and change defaultmode for opensearch dashboards (#342)

* allow adding plugins and change defaultmode for opensearch dashboards yaml file

Signed-off-by: Lu Yu <[email protected]>

* bump version and update changelog

Signed-off-by: Lu Yu <[email protected]>

* add new line

Signed-off-by: Lu Yu <[email protected]>

* bump version for os

Signed-off-by: Lu Yu <[email protected]>

* resolve conflict in changelog

Signed-off-by: Lu Yu <[email protected]>

* trigger build

Signed-off-by: Lu Yu <[email protected]>

Signed-off-by: Lu Yu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Fix path in securityConfig section on OpenSearch (values.yaml) (#344)

* fix securityConfig.path

Signed-off-by: Ruslan Gainanov <[email protected]>

* add link to issue

Signed-off-by: Ruslan Gainanov <[email protected]>

Signed-off-by: Ruslan Gainanov <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Update appVersion to 2.4.1 (#363)

* Update appVersion to 2.4.1

Signed-off-by: Sayali Gaikawad <[email protected]>

* Update appVersion to 2.4.1

Signed-off-by: Sayali Gaikawad <[email protected]>

* Fix changelog

Signed-off-by: Sayali Gaikawad <[email protected]>

* Fix changelog

Signed-off-by: Sayali Gaikawad <[email protected]>

* Fix version

Signed-off-by: Sayali Gaikawad <[email protected]>

Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Fix version for OpenSearch

Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Fix version for OpenSearch-Dasboards

Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Add hostPort support for http- and transport-ports (#336)

Signed-off-by: Christian Kuhn <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Updated MAINTAINERS.md to match recommended opensearch-project format. (#367)

Signed-off-by: dblock <[email protected]>

Signed-off-by: dblock <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Bump OS and OSD version to 2.5.0 (#373)

Signed-off-by: Rishabh Singh <[email protected]>

Signed-off-by: Rishabh Singh <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Created untriaged issue workflow. (#382)

Signed-off-by: dblock <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Bump OpenSearch and Dashboards to 2.6.0 (#393)

Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Updating the CODEOWNERS file (#399)

Signed-off-by: bbarani <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Add lifecycle support in opensearch container (#376)

* Add lifecycle support in opensearch container

Signed-off-by: josephteddick <[email protected]>

* lifecycle support PR cleanup

Signed-off-by: josephteddick <[email protected]>

---------

Signed-off-by: josephteddick <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* feat: Add user-defined labels option to ingress (#390)

Signed-off-by: Jason Witkowski <[email protected]>
Co-authored-by: Jason Witkowski <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Update deployment.yaml

Signed-off-by: prathaptce <[email protected]>

* Update values.yaml

Signed-off-by: prathaptce <[email protected]>

* Update values.yaml

Signed-off-by: prathaptce <[email protected]>

* Update statefulset.yaml

Signed-off-by: prathaptce <[email protected]>

* Updating chart version and changelog.md

Signed-off-by: Prathap Mahalingam (Nokia) <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Add lifecycle support in opensearch container (#376)

* Add lifecycle support in opensearch container

Signed-off-by: josephteddick <[email protected]>

* lifecycle support PR cleanup

Signed-off-by: josephteddick <[email protected]>

---------

Signed-off-by: josephteddick <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Add github-merit-badger.yml (#408)

Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Service port for performance analyzer (#346)

* Performance analyzer port mapping

Signed-off-by: Philipp Hölscher <[email protected]>

* Performance analyzer port on ci-values

Signed-off-by: Philipp Hölscher <[email protected]>

* Update changelog

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Philipp Hölscher <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Signed-off-by: prathaptce <[email protected]>

* Incorporated the review comments

Signed-off-by: prathaptce <[email protected]>

* Update values.yaml

Signed-off-by: prathaptce <[email protected]>

* Incorporated the review comments

Signed-off-by: prathaptce <[email protected]>

* Correcting the merge conflicts

Signed-off-by: prathaptce <[email protected]>

* Updated the README.md for OpenSearch & Dashboard

Signed-off-by: prathaptce <[email protected]>

* Updated the changelog message

Signed-off-by: prathaptce <[email protected]>

* Updating the version number

Signed-off-by: prathaptce <[email protected]>

---------

Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: prathaptce <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: Lu Yu <[email protected]>
Signed-off-by: Ruslan Gainanov <[email protected]>
Signed-off-by: Sayali Gaikawad <[email protected]>
Signed-off-by: Christian Kuhn <[email protected]>
Signed-off-by: dblock <[email protected]>
Signed-off-by: Rishabh Singh <[email protected]>
Signed-off-by: bbarani <[email protected]>
Signed-off-by: josephteddick <[email protected]>
Signed-off-by: Jason Witkowski <[email protected]>
Signed-off-by: Prathap Mahalingam (Nokia) <[email protected]>
Signed-off-by: Prudhvi Godithi <[email protected]>
Signed-off-by: Philipp Hölscher <[email protected]>
Co-authored-by: Zelin Hao <[email protected]>
Co-authored-by: mend-for-github.aaakk.us.kg[bot] <50673670+mend-for-github.aaakk.us.kg[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <[email protected]>
Co-authored-by: Lu Yu <[email protected]>
Co-authored-by: Ruslan Gainanov <[email protected]>
Co-authored-by: Sayali Gaikawad <[email protected]>
Co-authored-by: Sayali Gaikawad <[email protected]>
Co-authored-by: Christian Kuhn <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Co-authored-by: Rishabh Singh <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Co-authored-by: Barani <[email protected]>
Co-authored-by: Joseph Teddick <[email protected]>
Co-authored-by: Jason Witkowski <[email protected]>
Co-authored-by: Jason Witkowski <[email protected]>
Co-authored-by: Prudhvi Godithi <[email protected]>
Co-authored-by: Philipp Hölscher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.x Backport to 1.x branch after merging to main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants