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

Support apply PVC template changes to StatefulSet #412

Merged
merged 3 commits into from
Jun 3, 2022
Merged

Support apply PVC template changes to StatefulSet #412

merged 3 commits into from
Jun 3, 2022

Conversation

d-kuro
Copy link
Contributor

@d-kuro d-kuro commented May 11, 2022 •
edited
Loading

refs: #265

Add design documentation for moco-controller to support volume size expansion

Note

Rewrote docs to focus on recreating the StatefulSet rather than extending volume

@d-kuro d-kuro self-assigned this May 11, 2022
@d-kuro d-kuro force-pushed the d-kuro/resize-pvc-design branch from 10b6f7d to 09625b8 Compare May 11, 2022 02:48
@d-kuro d-kuro force-pushed the d-kuro/resize-pvc-design branch from 09625b8 to 8ce8915 Compare May 14, 2022 11:49
@d-kuro d-kuro marked this pull request as ready for review May 14, 2022 12:09
@d-kuro d-kuro requested review from masa213f, ymmt2005 and zoetrope May 14, 2022 12:10
@d-kuro d-kuro mentioned this pull request May 14, 2022
Merged
2 tasks
ymmt2005
ymmt2005 previously approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

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

LGTM

masa213f
masa213f reviewed May 17, 2022
View reviewed changes
docs/designdoc/support_volume_expansion.md Outdated
message: "Validation failed: ..."
```

Set the `VolumeResized` condition's status to `True` if the PVC expansion was successful.
Copy link
Contributor

Choose a reason for hiding this comment

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

That fine with me.
Could you also export this status as a metric?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I came up with two options, which would you prefer?

  1. Count metric, moco_pvc_expansion_failed
    • Count is incremented if PVC resizing fails
  2. Gauge metric, moco_pvc_expansion_status{states="True/False"}
    • Aggregate the number of objects from the list of MySQLCluster's that currently exist with that status

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you export this metrics for each MySQLCluster?
And also use the prefix of moco_cluster_ as with other metrics.
https://github.com/cybozu-go/moco/blob/main/docs/metrics.md#mysql-clusters

How about use the following format?
Count metircs: moco_cluster_volume_resized_total{status="True/False", name="mycluster", namespace="mynamesapce"}

Copy link
Member

Choose a reason for hiding this comment

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

@masa213f
As I told you elsewhere, moco-controller should re-create StatefulSet not only when the storage size is changed, but also when metadata (labels / annotations) is changed. Considering that, the metric name containing volume_resized is not appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

@d-kuro Have you heard about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ymmt2005 @d-kuro
Thank you for the comment. I explained it at yesterday's meeting.
And I requested to the specification change to be able to change the volume size, labels and annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the docs to reflect the comments 🙏 :
e63a1d5

@d-kuro d-kuro requested review from ymmt2005 and masa213f May 24, 2022 20:49
@d-kuro d-kuro force-pushed the d-kuro/resize-pvc-design branch from e934847 to 11475fa Compare June 1, 2022 04:08
@d-kuro d-kuro changed the title Add design doc for PVC expansion Support apply PVC template changes to StatefulSet Jun 1, 2022
@d-kuro
Copy link
Contributor Author

d-kuro commented Jun 1, 2022

@ymmt2005 @masa213f

I have rewritten the document based on conversations we had at our meetings.
Added metrics to notify errors, as moco-controller may endlessly handle errors that are not automatically recovered in some cases.
Please re-review 🙏
11475fa

masa213f
masa213f approved these changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@masa213f masa213f left a comment

Choose a reason for hiding this comment

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

LGTM. Great job! Thank you.

@masa213f masa213f merged commit 1d02efd into main Jun 3, 2022
@masa213f masa213f deleted the d-kuro/resize-pvc-design branch June 3, 2022 02:57
@donch
Copy link

donch commented Aug 22, 2022

Hello,

Do you have any release date which will include this feature please ?

@masa213f
Copy link
Contributor

@donch
thank you for contacting. I will release it in the next week.

@masa213f
Copy link
Contributor

masa213f commented Sep 2, 2022

Sorry, preparation of release has been delayed. I will release it next week. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masa213f masa213f masa213f approved these changes

@zoetrope zoetrope Awaiting requested review from zoetrope

@ymmt2005 ymmt2005 Awaiting requested review from ymmt2005

Assignees

@d-kuro d-kuro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@d-kuro @donch @masa213f @ymmt2005