Remove request rollout#34904
Remove request rollout#34904alex-hunt-materialize wants to merge 9 commits intoMaterializeInc:mainfrom
Conversation
alex-hunt-materialize
force-pushed
the
remove_request_rollout
branch
8 times, most recently
from
b19aa54 to
725bc8a
Compare
alex-hunt-materialize
force-pushed
the
remove_request_rollout
branch
7 times, most recently
from
4e91281 to
3902b4f
Compare
alex-hunt-materialize
force-pushed
the
remove_request_rollout
branch
from
3902b4f to
1352e8c
Compare
|
this pr is kind of enormous, so it's going to take a bit to go through, but one thing i would like to see here is having the counterpart cloud pr ready and tested before this is merged, because i don't want to merge this and then be blocked from updating the cloud submodule for an extended amount of time while we make sure everything works there. (i'll still review this in the meantime though.) |
| &PatchParams::default(), | ||
| &Patch::Merge(patch), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
how does this work? wouldn't this be reverted by the crd being rewritten every time orchestratord starts up?
There was a problem hiding this comment.
Do you mean when orchestratord installs the CRDs? I don't think that includes the status.
| }; | ||
| let status = mz.status(); | ||
|
|
||
| // TODO should this check be inside the else above? |
There was a problem hiding this comment.
it's not immediately obvious to me whether it can get to a state of being ready to promote while having the version to promote to be invalid - i don't think it really harms things having it outside the else, unless i'm missing something?
There was a problem hiding this comment.
It's about thrashing the status, not about being ready to promote. When we're not ready to promote, each loop through we apply the status update on the else clause above, and then if we're not within the upgrade window we update the status again and bail.
I didn't make the change here, as I wanted to minimize the logic changes during this huge migration, but I think it should be moved inside the else above.
|
QQ: Does this mean the stuff abut using requestRollout https://materialize.com/docs/self-managed-deployments/upgrading/ will no longer be valid after this PR? |
Good catch, I need to update those docs to use v1alpha2 and to remove the requestRollout stuff. |
alex-hunt-materialize
force-pushed
the
remove_request_rollout
branch
from
491e673 to
8265d8a
Compare
alex-hunt-materialize
force-pushed
the
remove_request_rollout
branch
from
96e8bf7 to
e169e3d
Compare
|
@doy-materialize I've opened https://github.com/MaterializeInc/cloud/pull/12223 for the cloud portion. |
kay-kim
left a comment
kay-kim
left a comment
There was a problem hiding this comment.
Thanks for the docs change. Just realized the implications today though. Also, double-checking ...
Because the docs ... when they get merged to main, they're published immediately ... as opposed to the release schedule for the product. So, it might be better to separate the docs work to another branch? I'll carry over the discussion in slack.
|
|
||
| 1. Install cert-manager | ||
|
|
||
| Cert-manager is used for generating TLS certificates needed by the materialize operator |
There was a problem hiding this comment.
materialize -> Materialize
Q: Would this be more "Starting in v26.?, cert-manager is used ..." (Although, I guess install itself won't hurt anything)? Or is it that we now have CRD conversion webhooks?
There was a problem hiding this comment.
Installing cert-manager won't hurt anything.
We need certificates for the CRD conversion webhooks added to orchestratord in this PR, and the easiest way to get them those certs is to use cert-manager. Alternatively, they could manually create a secret containing the cert and pass us the name in the helm values, but that's much more difficult.
We also optionally use cert-manager for certificates for balancerd, the console, and environmentd. We have no other supported way to provide certs for these, so getting them to install cert-manager here moves them in a good direction.
|
|
||
| To minimize unexpected downtime and avoid connection drops at critical | ||
| periods for your application, the upgrade process involves two steps: | ||
| #### Using `kubectl patch` |
There was a problem hiding this comment.
Hey @maheshwarip + @alex-hunt-materialize -- I know there was the decision last fall that the docs would always reflect the current version of the product (so that we wouldn't have to deal with various version annotations ... which can get messy and quickly difficult to manage).
I just realized w.r.t. this removal of the requestRollout config --
Right now, everyone who would need the upgrade instructions are pre-this change. As such, they would need the old instructions to get to version X or greater, yes?
So, we probably want both content (pre and post).
There was a problem hiding this comment.
I can add a section for older releases, but newer orchestratord can install older versions, and we now support skipping versions. We may still need the older docs for upgrading from versions older than v26, though.
|
Closing in favor of #35418 |
3 participants
Introduces a new version of the Materialize CRD, which no longer requires the user to update a
requestRolloutfield to trigger a rollout. Instead it hashes a subset of spec fields and a specific annotation in the CRD, and triggers a rollout when that hash changes.Also removes several deprecated fields.
Motivation
Part of https://linear.app/materializeinc/issue/DEP-7/design-simplifying-upgrade-rollouts-node-rolls-converted-to-project
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.