Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Allow customisation of healthcheck. #23

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Allow customisation of healthcheck. #23

merged 2 commits into from
Nov 23, 2023

Conversation

dalees
Copy link
Contributor

@dalees dalees commented Nov 2, 2023 •
edited
Loading

  • The autohealing label is analogous to CAPI's healthcheck.
  • Improves test failure output and sets autospec

@dalees
Copy link
Contributor Author

dalees commented Nov 2, 2023

Happy to split this into multiple commits/PRs as appropriate.

@dalees
Copy link
Contributor Author

dalees commented Nov 2, 2023

@mkjpryor - I recall you've got some related changes with CAPO to mount multiple volumes onto nodes?

Hopefully this PR doesn't conflict with that effort, and just takes the boot volume values from Magnum config - there are configuration values in Magnum for additional disks.

  1. CONF.cinder.default_boot_volume_type / CONF.cinder.default_boot_volume_size (implemented in this PR, same value used for control plane and workers)
  2. CONF.cinder.default_etcd_volume_type
  3. CONF.cinder.default_docker_volume_type (May need a path and name update, given we're all using containerd!)

Should we include these in the helm values? It's hard to determine if these should always be mounted on all node types, or are best to be configurable by the Magnum operator and default to CAPI's single disk deployment.

@mkjpryor
Copy link
Member

@dalees Those changes aren't in a CAPO release yet. They are also independent of the root volume specification so I think we should stick with these changes for now.

mkjpryor
mkjpryor approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@mkjpryor mkjpryor left a comment

Choose a reason for hiding this comment

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

LGTM

JohnGarbutt
JohnGarbutt suggested changes Nov 15, 2023
View reviewed changes
Copy link
Member

@JohnGarbutt JohnGarbutt left a comment

Choose a reason for hiding this comment

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

I am not sure about hard coding those CIDRs in the code. Could we add a configuration value for those? I worry about local services that clash with those CIDRs for some users.

(I kinda like how you could override that in the helm chart without us having to hard code it, but I am OK with the driver taking control of this too).

magnum_capi_helm/driver.py Outdated Show resolved Hide resolved
magnum_capi_helm/driver.py Outdated Show resolved Hide resolved
@dalees dalees changed the title Allow customisation of kubenetwork, root volumes and healthcheck. Allow customisation of root volumes and healthcheck. Nov 17, 2023
@dalees dalees mentioned this pull request Nov 17, 2023
Closed
@dalees dalees changed the title Allow customisation of root volumes and healthcheck. Allow customisation of healthcheck. Nov 17, 2023
@dalees dalees requested a review from JohnGarbutt November 17, 2023 19:13
@dalees dalees mentioned this pull request Nov 19, 2023
Merged
JohnGarbutt
JohnGarbutt approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@JohnGarbutt JohnGarbutt left a comment

Choose a reason for hiding this comment

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

I like the refactor here, thank you!

JohnGarbutt
JohnGarbutt approved these changes Nov 22, 2023
View reviewed changes
Copy link
Member

@JohnGarbutt JohnGarbutt left a comment

Choose a reason for hiding this comment

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

This matches what I see in the capi operator around the autoheal (which reminds me, we should fix up the autoscale too, as that works in the charts already)

@JohnGarbutt
Copy link
Member

@dalees Ah, sorry, I merged a conflicting change again, sorry!

@dalees
Add autoheal customisation
f2db4b8 
* The autohealing label is analogous to CAPI's healthcheck.
@dalees
Update tests for autoheal customisation
7a08035 
- Update default healthCheck value in tests
- Add test for disabling healthCheck
- Add autospec=true to all cluster tests to improve matching.
- Change helm values dict comparison to assertDictEqual for better output.
@dalees
Copy link
Contributor Author

dalees commented Nov 23, 2023

@dalees Ah, sorry, I merged a conflicting change again, sorry!

Bound to happen either way you did it. I've got too much refactoring of tests in this one - rebased!

JohnGarbutt
JohnGarbutt approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

@JohnGarbutt JohnGarbutt left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@JohnGarbutt JohnGarbutt merged commit 66e7806 into stackhpc:main Nov 23, 2023
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mkjpryor mkjpryor mkjpryor approved these changes

@JohnGarbutt JohnGarbutt JohnGarbutt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dalees @mkjpryor @JohnGarbutt