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

teleport-cluster chart: set strategy to 'Recreate' if chartMode is 'standalone' (fixes #11484) #11493

Conversation

johanneskastl
Copy link
Contributor

Fixes #11484

In standalone mode a PVC is created. The volume cannot be attached to more than one pod (depending on the storage available), so using a Recreate strategy is needed to avoid stuck deployments on upgrades.

@webvictim
Copy link
Contributor

Hey @johanneskastl - thanks for the PR. Please take a look at https://github.com/gravitational/teleport/blob/master/examples/chart/CONTRIBUTING.md for some additional testing we'd need.

@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch from cb61471 to 35af5b8 Compare March 30, 2022 13:09
@johanneskastl
Copy link
Contributor Author

Hey @johanneskastl - thanks for the PR. Please take a look at https://github.com/gravitational/teleport/blob/master/examples/chart/CONTRIBUTING.md for some additional testing we'd need.

Thanks for the hint @webvictim! I have added a test to tests/deployment_test.yaml that verifies that it sets the strategy in standalone mode. Do you recommend adding more tests?

Did I miss any other steps?

Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

@johanneskastl Thanks! Looks great!

Just one final ask: could you also please add three extra tests to make sure that the strategy is not set (so will default to RollingUpdate) for the other modes (aws, gcp and custom)?

Something similar to this section: https://github.com/gravitational/teleport/blob/master/examples/chart/teleport-cluster/tests/deployment_test.yaml#L234-L310 - you should be able to use the same values files.

examples/chart/teleport-cluster/templates/deployment.yaml Outdated Show resolved Hide resolved
@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch from 35af5b8 to f828785 Compare March 31, 2022 07:01
@johanneskastl
Copy link
Contributor Author

johanneskastl commented Mar 31, 2022

Hey @webvictim, I added three more tests for AWS/GCP/Custom mode to not contain the strategy setting. Could you have a look if this is fine? Anything else missing?

Should I also commit this file that was created by running make lint-helm test-helm?

examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap

@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch 2 times, most recently from 14f97df to 8a2df42 Compare March 31, 2022 07:16
Copy link

@dirien dirien left a comment

Choose a reason for hiding this comment

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

Hi @johanneskastl,

that looks good for me! Looking forward for this PR as it blocks us too currently.

@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch from 8a2df42 to 49fb486 Compare March 31, 2022 19:08
@zmb3
Copy link
Collaborator

zmb3 commented Mar 31, 2022

Looks good to me @johanneskastl - we just need to commit the examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap and we should be good to merge this.

Thanks for the contribution!

Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

LGTM. As @zmb3 says, please commit the snapshot and we'll be good to go!

@webvictim
Copy link
Contributor

/gcbrun

@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch from 81dcc3a to f1b293d Compare April 1, 2022 06:49
@johanneskastl
Copy link
Contributor Author

@webvictim @zmb3 I commited the file, please have a look.

@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch 2 times, most recently from 23b1138 to 2f7c4b9 Compare April 1, 2022 17:41
@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch 3 times, most recently from f0fa0c5 to de1ec3c Compare April 2, 2022 11:43
@johanneskastl
Copy link
Contributor Author

Rebased on current master and solved conflict in examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap.

Can this be merged? Or is something missing?

@webvictim
Copy link
Contributor

/gcbrun

Johannes Kastl added 3 commits April 3, 2022 10:03
…gy to 'Recreate' if chartMode is 'standalone' (fixes gravitational#11484)

Signed-off-by: Johannes Kastl <[email protected]>
…for strategy in standalone mode

Signed-off-by: Johannes Kastl <[email protected]>
@johanneskastl johanneskastl force-pushed the 20220328_helm_chart_strategy_recreate branch from de1ec3c to db1640d Compare April 3, 2022 09:00
@johanneskastl
Copy link
Contributor Author

@webvictim I'll stop rebasing this, as I cannot run the tests anyway.

Please ping me if I can assists or if fixes are needed.

@webvictim
Copy link
Contributor

@johanneskastl Thanks - I'm sure it's fine, we'll take a look when people are back at work on Monday.

@webvictim
Copy link
Contributor

/gcbrun

1 similar comment
@webvictim
Copy link
Contributor

/gcbrun

@webvictim webvictim enabled auto-merge (squash) April 4, 2022 01:39
@webvictim
Copy link
Contributor

It looks like the review bot is refusing to approve this because reasons. I'll open a new buddy PR and get it merged.

@johanneskastl
Copy link
Contributor Author

It looks like the review bot is refusing to approve this because reasons. I'll open a new buddy PR and get it merged.

Thanks @webvictim

webvictim added a commit that referenced this pull request Apr 5, 2022
* examples/chart/teleport-cluster/templates/deployment.yaml: set strategy to 'Recreate' if chartMode is 'standalone' (fixes #11484)

Signed-off-by: Johannes Kastl <[email protected]>

* examples/chart/teleport-cluster/tests/deployment_test.yaml: add test for strategy in standalone mode

Signed-off-by: Johannes Kastl <[email protected]>

* update examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap after running local tests

Co-authored-by: Johannes Kastl <[email protected]>
@johanneskastl
Copy link
Contributor Author

@webvictim I see you merged 11693 containing my changes, so can we close this one?

@webvictim
Copy link
Contributor

@johanneskastl Thanks for the contribution!

@webvictim webvictim closed this Apr 5, 2022
auto-merge was automatically disabled April 5, 2022 13:59

Pull request was closed

webvictim added a commit that referenced this pull request Apr 5, 2022
* examples/chart/teleport-cluster/templates/deployment.yaml: set strategy to 'Recreate' if chartMode is 'standalone' (fixes #11484)

Signed-off-by: Johannes Kastl <[email protected]>

* examples/chart/teleport-cluster/tests/deployment_test.yaml: add test for strategy in standalone mode

Signed-off-by: Johannes Kastl <[email protected]>

* update examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap after running local tests

Co-authored-by: Johannes Kastl <[email protected]>
webvictim added a commit that referenced this pull request Apr 11, 2022
* helm: Buddy merge for #11493 (#11693)

* examples/chart/teleport-cluster/templates/deployment.yaml: set strategy to 'Recreate' if chartMode is 'standalone' (fixes #11484)

Signed-off-by: Johannes Kastl <[email protected]>

* examples/chart/teleport-cluster/tests/deployment_test.yaml: add test for strategy in standalone mode

Signed-off-by: Johannes Kastl <[email protected]>

* update examples/chart/teleport-cluster/tests/__snapshot__/deployment_test.yaml.snap after running local tests

Co-authored-by: Johannes Kastl <[email protected]>

* helm: Allow probe timeouts to be configurable (buddy merge of #11176) (#11396)

* Allow for probe timeouts to be configurable

When setting up a new Teleport enterprise cluster on GCP,
I noticed that I needed to set the probe timeouts to get the
cluster to be healthy. This seems to be a known issue (kubernetes/kubernetes#89898).

As a "stopgap", I've updated the helm chart to allow for end users
to be able to configure these timeouts.

* Update configuration option name and add documentation

* Update docs/pages/kubernetes-access/helm/reference.mdx

Co-authored-by: Gus Luxton <[email protected]>

* Add tests for probeTimeoutSeconds

* Add probeTimeoutSeconds to required values

* Add probeTimeoutSeconds to teleport-kube-agent

* Add tests for probeTimeoutSeconds to teleport-kube-agent

* Add probeTimeoutSeconds to teleport-kube-agent reference

Co-authored-by: Hunter Madison <[email protected]>
Co-authored-by: Hunter Madison <[email protected]>

Co-authored-by: Johannes Kastl <[email protected]>
Co-authored-by: Hunter Madison <[email protected]>
Co-authored-by: Hunter Madison <[email protected]>
@johanneskastl johanneskastl deleted the 20220328_helm_chart_strategy_recreate branch April 16, 2022 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart teleport-cluster: "Multi-Attach error for volume" when upgrading via "helm upgrade"
4 participants