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

updates: capture offline discussion on eus-upgrades-mvp #800

Merged
merged 1 commit into from
Jul 21, 2021
Merged

updates: capture offline discussion on eus-upgrades-mvp #800

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions enhancements/update/eus-upgrades-mvp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,38 @@ customers.

## Design Details

### Offline Discussion

To recap an offline discussion about this for those that were not able
to attend based on [review comments on the original
PR](https://github.com/openshift/enhancements/pull/762/files#r626609451):

* The API server ships with a set of built-in types (pod and node).
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear why this matters?

Do you want to say that the pod subresources are implemented through APIs served by the kubelet and called by the kube-apiserver? This puts a constraint on maximal skew.

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'm not sure what the background is, I wasn't in the meeting. I'm capturing @derekwaynecarr's comment from the original PR.

* The upstream project via API review/human or project operator
oversight ensures that skew between API server and kubelet is n-2.
* The API server operator is being asked to report Upgradeable=False
to capture that API review/human/operational oversight because it is
the component that must not upgrade.
* The Machine Config Operator is the component that must upgrade, so
reporting Upgradeable=False is confusing.
* The Machine Config Operator should report a message in its reporting
that admins should perform an action.

I (@derekwaynecarr) agree that the API server (and its operators)
should not have to understand all API types it serves and all its
clients. I think its reasonable for API types that are built-in, whose
upstream set of human operators/api-reviewers/etc. are trying to
ensure that skew in either direction (api-server or kubelet) is
preserved that both operators in our context should report a desired
action (apiserver operator should say I cannot upgrade, machine config
operator should promote an upgrade).
Copy link
Contributor

Choose a reason for hiding this comment

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

I still disagree with this assessment. This is not about REST API skew (of built-in resources) at all, and hence kubelet as an API client does not matter. This is about kube-apiserver->kubelet API usage to implement certain subresources. Hence, the apiserver is the client and the kubelet dictates which APIs it serves. And therefore it makes sense that the apiserver has to block upgrades.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear: the outcome of my logic is the same: the ckao will report Upgradable=False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sttts Does that mean this enhancement doc needs a different change? I want to capture the argument in the body, so maybe this text needs to be proposed as an update to the alternatives section, instead of here? I'm not aware of what the final outcome was, so help me work out what updates are needed, please.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the intent is really to capture the leading component must not upgrade, in this scenario that's the apiserver. The details around API types and compatibility are just in support of that.


For types that are not built-in (CRDs, etc.), it is extremely
reasonable to say that the future philosophy is not for the API server
to block its own upgrade on those types because the upgrade of the API
server itself is not what will evolve those CRD definitions, unlike
the built-in types.

### Open Questions [optional]

- As this is a bit of a meta-enhancement calling out epic level work it should be
Expand Down