MCO-1518: Create V1 MCN API #2255
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @isabella-janssen! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
/test all |
isabella-janssen
changed the title
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
|
/test all |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
isabella-janssen
changed the title
isabella-janssen
force-pushed
the
create-mcn-v1-api
branch
from
20ff318 to
10ac16b
Compare
isabella-janssen
force-pushed
the
create-mcn-v1-api
branch
2 times, most recently
from
10f6549 to
fa125b7
Compare
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
| #TODO(ijanssen): once MCO side is ready for MCN v1 API, update the v1 crds back to | ||
| # `machineconfiguration/v1/zz_generated.crd-manifests/*.crd.yaml` and remove MCN v1alpha1 crd ref | ||
| # `machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes*.crd.yaml`. |
There was a problem hiding this comment.
How do we plan to co-ordinate this? Do we need to simul-merge a couple of PRs to make this actually work?
There was a problem hiding this comment.
My understanding is we will need to simul-merge PRs in the following repos for this:
- API PR to deploy V1 CRDs: MCO-1519: Deploy v1 CRDs for the MCN API #2273
- MCO PR to adapt to using the new CRDs
Origin PR to adapt tests to using the new CRDs
Linking this Slack thread where this is also being discussed: https://redhat-internal.slack.com/archives/CE4L0F143/p1744292448679729?thread_ts=1744228390.483339&cid=CE4L0F143
There was a problem hiding this comment.
This has been the steps we've done for other v1 APIs, curious if there's a better way to do this, since there's a disconnect between the deployment of the CRD (happens here) vs consuming the API (in the MCO) without having to write a conversion webhook.
There was a problem hiding this comment.
I'm not personally aware of any better ways to do this. When I was on the OLM team, our approach for "breaking" changes like this was to temporarily disable the tests that would fail (and maybe remove ourselves from the payload), force merge the updates simultaneously, and finally re-enable the tests.
I would be curious to explore if there is a better way to do this now using a combination of API discovery and Go generics, but that is likely a heavy handed change.
There was a problem hiding this comment.
My understanding is that we'd have to serve both apis with a v1alpha1 to v1 mutating webhook. Update all clients to v1, we should check for v1alpha1 usage and when there is none remove it.
Given this is all behind TPNU right now and our intention is to transition to v1 before lifting that gate I think we can cut some corners. docs.ci.openshift.org is down right now but I believe there's docs there on how to run payload tests with multiple PRs merged. So we should stage all of the PRs, test it that way, and I can help coordinate making sure they merge at approximately the same time. Of course all contingent on the v1 API review and tests passing.
|
|
||
| // StateProgress is each possible state for each possible MachineConfigNodeType | ||
| // +enum | ||
| type StateProgress string |
There was a problem hiding this comment.
This isn't used in the API?
There was a problem hiding this comment.
It's used as the type definition for the conditions below and in the MCO-side code to define the type of a condition state.
isabella-janssen
force-pushed
the
create-mcn-v1-api
branch
from
8ba2705 to
d846d37
Compare
|
@isabella-janssen: This pull request references MCO-1518 which is a valid jira issue. 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. |
isabella-janssen
force-pushed
the
create-mcn-v1-api
branch
from
20468f1 to
0db77b1
Compare
isabella-janssen
force-pushed
the
create-mcn-v1-api
branch
from
53bc234 to
5fafc7d
Compare
|
@isabella-janssen: 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. |
There was a problem hiding this comment.
Seems safe as a direct lift of the v1alpha API, and we won't be using it until we can switch the MCO code and use both, so it should be considered tested since v1alpha1 is.
I can't speak to whether the tombstoned fields being removed affects anything, but otherwise LGTM!
| #TODO(ijanssen): once MCO side is ready for MCN v1 API, update the v1 crds back to | ||
| # `machineconfiguration/v1/zz_generated.crd-manifests/*.crd.yaml` and remove MCN v1alpha1 crd ref | ||
| # `machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes*.crd.yaml`. |
There was a problem hiding this comment.
This has been the steps we've done for other v1 APIs, curious if there's a better way to do this, since there's a disconnect between the deployment of the CRD (happens here) vs consuming the API (in the MCO) without having to write a conversion webhook.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven, isabella-janssen, yuqi-zhang 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 |
Included work:
Spec.PinnedImageSets(tombstoned in NO ISSUE: TombstoneSpec.PinnedImageSetsin MCN v1alpha1 API #2256) andStatus.PinnedImageSets.LastFailedGenerationErrors(tombstoned in MCO-1543: Update v1alpha1 MCN API #2201) are not referenced in this new v1 API.References: