-
Notifications
You must be signed in to change notification settings - Fork 524
OPRUN-4244: Add EP for introducing Boxcutter library in OLMv1
#1890
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
base: master
Are you sure you want to change the base?
Conversation
Boxcutter library in OLMv1Boxcutter library in OLMv1
|
@anik120: This pull request references OPRUN-4244 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| `CollisionProtection=None` | ||
|
|
||
| **Garbage Collection** | ||
| - Maintains the last 5 archived revisions per `ClusterExtension` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty nit-picky but what we do is slightly more nuanced. The 5 previous revisions are not necessarily archived and some may still be active, particularly in transitional states. The only revisions trimmed from the cluster are revisions that are both archived and exist beyond the total limit of 5.
An extreme but theoretically possible scenario would be a previous list with length > 5, where all revisions are not archived. In this scenario, none of the revisions would be trimmed from the list or the cluster. Also for instance, if there were 6 active revisions and a 7th archived revision, only the 7th archived revision will be trimmed, and the list will remain at length == 6.
TLDR: All previous revisions in archived lifecycle state beyond the revision limit of 5 will be garbage collected. Revisions in active lifecycle state are never garbage collected, even if the list contains more than 5 of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this section based on these pointers, PTAL cc20e02
| dynamic watches for all resource GVKs in the revision | ||
| 8. `ClusterExtensionRevision` controller invokes RevisionEngine.Reconcile() which | ||
| iterates through phases sequentially | ||
| 9. For each object in Phase 1 (e.g., CRDs, RBAC), RevisionEngine applies via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This again may be overly implementation-detail-ey but Phase 1 is Namespaces, 2 is 'policies' like NetworkPolicy, 3 is RBAC, then 4 is CRDs, etc. Not sure if that detail really matters for the purposes of this doc though and would also make the sequence diagram messier 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this section, tried to stay away from getting over implementation detail-y here..PTAL cc20e02
| 6. `ClusterExtensionRevision` controller reconciles new revision following same | ||
| phased rollout process | ||
| 7. When new revision reaches Succeeded=True, `ClusterExtensionRevision` | ||
| controller patches `<extension-name>-1` to set lifecycleState=Archived | ||
| 8. Archived revision controller tears down resources it uniquely owns (those | ||
| not adopted by new revision) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call out the adoption process here rather than as a footnote in step 8?
| 6. `ClusterExtensionRevision` controller reconciles new revision following same | |
| phased rollout process | |
| 7. When new revision reaches Succeeded=True, `ClusterExtensionRevision` | |
| controller patches `<extension-name>-1` to set lifecycleState=Archived | |
| 8. Archived revision controller tears down resources it uniquely owns (those | |
| not adopted by new revision) | |
| > 6. `ClusterExtensionRevision` controller reconciles new revision following same phased rollout process, adopting any existing resources from the previous revision | |
| > 7. When new revision reaches Succeeded=True, `ClusterExtensionRevision` | |
| controller patches `<extension-name>-1` to set lifecycleState=Archived | |
| > 8. Archived revision controller tears down remaining resources not adopted by the previous revision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Archived revision controller tears down remaining resources not adopted by the previous revision
@dtfranz I want to make sure I understand the terminology correctly in this context:
Scenario:
- Revision 1 (old) exists with resources A, B, C
- Revision 2 (new) is created and adopts resources A and B (step 6)
- Revision 2 reaches Succeeded=True (step 7)
- Revision 1 is archived (step 7)
- Revision 1 now tears down resources... (step 8)
Questions:
- In step 8, when we say "previous revision," do we mean:
- Revision 2 (the revision that came after revision 1 and is now active), OR
- Revision 0 or whatever came before revision 1? - My original text says "not adopted by new revision" - is the issue with this wording that:
- It's unclear which revision is "new" at this point in the timeline?
- It should refer to the "newer" revision (revision 2)? - Would clearer wording be something like: "tears down resources not adopted by the new active revision" or "tears
down resources that only revision 1 owned"?
Signed-off-by: Per Goncalves da Silva <[email protected]>
Add some more stuff
|
@anik120: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
No description provided.