Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/apm-server] Add support for Service and Deployment #11661

Merged
merged 6 commits into from
Apr 15, 2019
Merged

[stable/apm-server] Add support for Service and Deployment #11661

merged 6 commits into from
Apr 15, 2019

Conversation

ali-essam
Copy link
Contributor

@ali-essam ali-essam commented Feb 22, 2019 •
edited
Loading

What this PR does / why we need it:

  • Adds support for the option of deploying APM Server as Deployment in addition to Daemonset
  • Make updateStrategy configurable
  • Adds support for creating a service pointing to APM pods
  • Updates missing docs

Which issue this PR fixes

Checklist

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Feb 22, 2019
@ali-essam ali-essam changed the title [stable/apm-server] Add support for Deployment in addition to Daemonset [stable/apm-server] Add support for Service and Deployment Feb 22, 2019
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2019
@ali-essam
Copy link
Contributor Author

ali-essam commented Feb 26, 2019 •
edited
Loading

@mattfarina Updated. PTAL.

I was also wondering whether this last update should be considered a breaking change or not (to bump the major version)? Especially that spec.selector.matchLabels is immutable. So anyone who deployed a daemonset before might not be able to upgrade.

#7680

Update:

As per the review guidelines, yeah this is a breaking change. For the sake of chart consistency, I would go for bumping the version rather than having the deployment with the new labels and leaving the daemonset unchanged.

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2019
@mattfarina
Copy link
Contributor

cc @mumoshu

Want the chart maintainer to take a look.

@ali-essam
Copy link
Contributor Author

@mattfarina no updates from chart maintainer. How should we proceed?

@Crazybus Crazybus mentioned this pull request Mar 11, 2019
Closed
@mumoshu
Copy link
Collaborator

mumoshu commented Mar 11, 2019

Hey! Thanks for your efforts. And - Yeah, this should be a breaking change.

@mumoshu
Copy link
Collaborator

mumoshu commented Mar 11, 2019

/lgtm

@k8s-ci-robot
Copy link
Contributor

@mumoshu: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mumoshu
Copy link
Collaborator

mumoshu commented Mar 11, 2019

Ah, too bad I missed adding myself to the OWNERS file under this chart 😢

@mattfarina Would you mind approving this? Thx!

mumoshu added a commit to mumoshu/charts that referenced this pull request Mar 11, 2019
@mumoshu
[stable/apm-server] add @mumoshu to OWNERS
86b9233 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)
@mumoshu mumoshu mentioned this pull request Mar 11, 2019
Merged
@mumoshu
Copy link
Collaborator

mumoshu commented Mar 11, 2019

Submitted #12066 believing that's needed for me to maintain this chart.

@mattfarina Would you mind approving #12066 as well?

@ali-essam I appreciate it if you could send a PR like the above if you will be eager to maintain the chart together :)

@ali-essam
Copy link
Contributor Author

/assign @mattfarina

mumoshu added a commit to mumoshu/charts that referenced this pull request Mar 21, 2019
@mumoshu
[stable/apm-server] add @mumoshu to OWNERS
1a007fc 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
mumoshu added a commit to mumoshu/charts that referenced this pull request Mar 25, 2019
@mumoshu
[stable/apm-server] add @mumoshu to OWNERS
cc71627 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
mumoshu added a commit to mumoshu/charts that referenced this pull request Mar 25, 2019
@mumoshu
[stable/apm-server] add @mumoshu to OWNERS
f2658dd 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
@mdibaiee
Copy link
Contributor

mdibaiee commented Apr 7, 2019

Thank you @ali-essam for this.

I believe you have to bump the chart version again since 1.0.0 has already been published.

@mdibaiee
Copy link
Contributor

mdibaiee commented Apr 7, 2019

I tested this pull-request and it seems like requests to pods result in:

curl: (7) Failed to connect to 10.16.4.191 port 8200: Connection refused

My configuration:

apm-server:
  kind: Deployment
  replicaCount: 3

  service:
    enabled: true

The same happens when using kind: DaemonSet.

@mdibaiee
Copy link
Contributor

mdibaiee commented Apr 7, 2019

The issue is, the default apm-server.host: "localhost:8200" should instead be 0.0.0.0:8200.

I already have the fix and I also wrote Readiness and Liveness probes for the deployment. I will send a pull-request to @ali-essam 's branch.

@mdibaiee mdibaiee mentioned this pull request Apr 7, 2019
Merged
3 tasks
@helm-bot helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 7, 2019
@ali-essam ali-essam force-pushed the feature/apm-server-deployment branch from 21610c8 to aa08cf3 Compare April 7, 2019 11:14
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Apr 7, 2019
@ali-essam
Copy link
Contributor Author

I bumped the chart version to 1.1.0 and merged @mdibaiee changes.

@mattfarina can you please check

@paulczar
bump version for breaking change
0ebbec0 
Signed-off-by: Paul Czarkowski <[email protected]>
@paulczar
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ali-essam, mumoshu, paulczar

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 37ae135 into helm:master Apr 15, 2019
TsuyoshiUshio pushed a commit to TsuyoshiUshio/charts that referenced this pull request Apr 18, 2019
@ali-essam @TsuyoshiUshio
[stable/apm-server] Add support for Service and Deployment (helm#11661)
f3550f4 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
TsuyoshiUshio pushed a commit to TsuyoshiUshio/charts that referenced this pull request Apr 18, 2019
@mumoshu @TsuyoshiUshio
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
e1fbffc 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
hajnej pushed a commit to hajnej/charts that referenced this pull request Apr 24, 2019
@ali-essam @hajnej
[stable/apm-server] Add support for Service and Deployment (helm#11661)
cf82744 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
hajnej pushed a commit to hajnej/charts that referenced this pull request Apr 24, 2019
@mumoshu @hajnej
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
29c3b49 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
devnulled pushed a commit to devnulled/charts that referenced this pull request Apr 25, 2019
@ali-essam @devnulled
[stable/apm-server] Add support for Service and Deployment (helm#11661)
522bfdc 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
devnulled pushed a commit to devnulled/charts that referenced this pull request Apr 25, 2019
@mumoshu @devnulled
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
e9352f8 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
dermorz pushed a commit to dermorz/charts that referenced this pull request Apr 26, 2019
@ali-essam
[stable/apm-server] Add support for Service and Deployment (helm#11661)
524882d 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
dermorz pushed a commit to dermorz/charts that referenced this pull request Apr 26, 2019
@mumoshu
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
634aef8 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
dpkirchner pushed a commit to dpkirchner/charts that referenced this pull request May 9, 2019
@ali-essam @dpkirchner
[stable/apm-server] Add support for Service and Deployment (helm#11661)
81853f9 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
dpkirchner pushed a commit to dpkirchner/charts that referenced this pull request May 9, 2019
@mumoshu @dpkirchner
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
e1cd85f 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
edsoncsouza pushed a commit to socialbase/charts that referenced this pull request May 14, 2019
@mumoshu @edsoncsouza
[stable/apm-server] add @mumoshu to OWNERS (#12066)
0fcce10 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm/charts#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
goshlanguage pushed a commit to goshlanguage/charts that referenced this pull request May 17, 2019
@ali-essam @goshlanguage
[stable/apm-server] Add support for Service and Deployment (helm#11661)
216bec3 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
goshlanguage pushed a commit to goshlanguage/charts that referenced this pull request May 17, 2019
@mumoshu @goshlanguage
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
e35c0aa 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
eyenx pushed a commit to eyenx/charts that referenced this pull request May 28, 2019
@ali-essam @eyenx
[stable/apm-server] Add support for Service and Deployment (helm#11661)
7f783e6 
* [stable/apm-server] Add support for Deployment

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Update labels & apiVersion

Signed-off-by: Ali Essam <[email protected]>

* [stable/apm-server] Bump version to 1.0.0

Signed-off-by: Ali Essam <[email protected]>

* feat(probes): add probes to deployment. Set default host as 0.0.0.0

Signed-off-by: Mahdi Dibaiee <[email protected]>

* bump version for breaking change

Signed-off-by: Paul Czarkowski <[email protected]>
eyenx pushed a commit to eyenx/charts that referenced this pull request May 28, 2019
@mumoshu @eyenx
[stable/apm-server] add @mumoshu to OWNERS (helm#12066)
0e549c3 
As I am the original author of the chart. Currently I miss the privilege to put LGTM any pull rquest to chart due to the situation, as seen in helm#11661 (comment)

Signed-off-by: Yusuke Kuoka <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattfarina mattfarina mattfarina left review comments

@paulczar paulczar Awaiting requested review from paulczar

Assignees

@mattfarina mattfarina

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[stable/apm-server] Service for APM Server daemonset
7 participants
@ali-essam @mattfarina @mumoshu @k8s-ci-robot @mdibaiee @paulczar @helm-bot