Remove request rollout#34904
Remove request rollout#34904alex-hunt-materialize wants to merge 4 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.) |
| /// If running in AWS, override the IAM role to use to give | ||
| /// environmentd access to the persist S3 bucket. | ||
| #[kube(deprecated)] | ||
| pub environmentd_iam_role_arn: Option<String>, |
There was a problem hiding this comment.
why is this still in the v2 resource?
There was a problem hiding this comment.
Because I forgot to remove it. It requires extra code to move it to the annotations, and I didn't want to remove it until after I had that in place.
| &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. |
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.