MCO-1443: Promote onclusterbuild to GA #2090
Conversation
|
Skipping CI for Draft Pull Request. |
|
Hello @yuqi-zhang! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/test all |
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
from
8465feb
to
2ca4842
Compare
|
/test all |
First step to GA'ing the currently v1alpha1 APIs. Don't add to payload manifests yet, and the featuregate is retained.
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
from
2ca4842
to
4c9f154
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
yuqi-zhang
changed the title
|
@yuqi-zhang: This pull request references MCO-1443 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 task to target the "4.18.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. |
openshift-ci-robot
added
the
jira/valid-reference
| @@ -0,0 +1,64 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | |||
| name: "[TechPreview] MachineOSBuild" | |||
There was a problem hiding this comment.
No longer tech preview? I know it kind of is, but, we aren't likely to remember to update this when we update the gate, so this will become disjoint
| @@ -0,0 +1,134 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | |||
| name: "[TechPreview] MachineOSConfig" | |||
| // version tracks the newest MachineOSBuild for each MachineOSConfig | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Required | ||
| Version int64 `json:"version"` |
There was a problem hiding this comment.
I'm not following how this interoperates, in particular, with ConfigGeneration? Can you remind me?
There was a problem hiding this comment.
I think the original intent was that they track a corresponding MachineOSConfig generation to do rebuilds, but in techpreview the implementation hasn't leveraged either field (hard set to 1). Let me check that and get back to you.
| // host[:port][/namespace]/name:<tag> or svc_name.namespace.svc[:port]/repository/name:<tag> | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=447 | ||
| // +kubebuilder:validation:XValidation:rule=`((self.split(':').size() == 2 && self.split(':')[1].matches('^([a-zA-Z0-9-./:])+$')) || self.matches('^[^.]+\\.[^.]+\\.svc:\\d+\\/[^\\/]+\\/[^\\/]+:[^\\/]+$'))`,message="the OCI Image reference must end with a valid :<tag>, where '<digest>' is 64 characters long and '<tag>' is any valid string Or it must be a valid .svc followed by a port, repository, image name, and tag." |
There was a problem hiding this comment.
The message here references digest, but it's not in the example?
| // +listMapKey=type | ||
| // +optional | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` | ||
| // ImageBuilderType describes the image builder set in the MachineOSConfig |
There was a problem hiding this comment.
Godoc for this field is wrong, it's talking about a different field name?
| // +kubebuilder:default:=noarch | ||
| // +optional | ||
| ContainerfileArch ContainerfileArch `json:"containerfileArch"` | ||
| // content is the custom content to be built |
There was a problem hiding this comment.
What is this? Can we expand this documentation?
|
|
||
| type MachineOSImageBuilder struct { | ||
| // imageBuilderType specifies the backend to be used to build the image. | ||
| // +kubebuilder:default:=PodImageBuilder |
There was a problem hiding this comment.
Are we sure we want this defaulted? What if we need to change this in the future as we decide there's a better, default image build method that doesn't rely on today's pod based image builder?
There was a problem hiding this comment.
Is it difficult to change the default here in the API if we wanted to change it in the future?
| // +kubebuilder:validation:XValidation:rule=`((self.split(':').size() == 2 && self.split(':')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')) || self.matches('^[^.]+\\.[^.]+\\.svc:\\d+\\/[^\\/]+\\/[^\\/]+:[^\\/]+$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme. Or it must be a valid .svc followed by a port, repository, image name, and tag." | ||
| // +kubebuilder:validation:Required | ||
| RenderedImagePushspec string `json:"renderedImagePushspec"` | ||
| // releaseVersion is associated with the base OS Image. This is the version of Openshift that the Base Image is associated with. |
There was a problem hiding this comment.
This line almost says the same thing twice, perhaps this should consolidate into one?
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
3 times, most recently
from
60085e1
to
416f317
Compare
|
Updated based on comments: Also temporarily reverted using format.dns1123subdomain() library while figuring out how it works |
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
2 times, most recently
from
a663367
to
8f72efb
Compare
Mostly fixups, with some minor changes to the v1alpha1 API: - Removed Version and ConfigGeneration from MOSB as they were unused - Updated relatedobjects list - Changed all optional,omitempty structs to pointers - Removed default for ImageBuilderType, but keeping default build arch to noarch as we don’t foresee changing that. - Fixed RenderedImagePushspec validators to match description
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
from
8f72efb
to
c6619bf
Compare
|
/test e2e-aws-serial-techpreview |
| // describes that the machine-os-builder will use a custom pod builder that uses buildah | ||
| PodBuilder MachineOSImageBuilderType = "PodImageBuilder" | ||
| // describes that the machine-os-builder will use a Job to spin up a custom pod builder that uses buildah | ||
| PodBuilder MachineOSImageBuilderType = "JobImageBuilder" |
There was a problem hiding this comment.
Should this be renamed to JobBuilder as well?
| // +unionMember | ||
| // +optional | ||
| PodImageBuilder *ObjectReference `json:"buildPod,omitempty"` | ||
| JobImageBuilderStatus *ObjectReference `json:"jobImageBuilderStatus,omitempty"` |
There was a problem hiding this comment.
I'm not sure that BuilderStatus makes sense here because this is mostly just a reference to the build executor which doesn't include any kind of status message (to my knowledge). Maybe something like JobImageBuilderRef or something like that would be more appropriate? Better yet, this could just be ImageBuilderRef. That, coupled with the value in ImageBuilderType, should be enough.
There was a problem hiding this comment.
I think the comment above this field might need an update to reflect that it is a reference as well.
Stepping back a bit, I'm guessing the reason we are using a union here is we could have different builders, and they could need different types(other than ObjectReference) to represent their reference? If we are always going to use ObjectReference for multiple kind of builders, then maybe we don't even need to have to a union discriminator? It could be just two fields within this struct as Zack suggested: type and ref.
There was a problem hiding this comment.
Those values are arguably common to all image builders. There could be some additional information that may be desired specific to each type of image builder, but we can add that onto this later as a specific type.
There was a problem hiding this comment.
And taking a step back from this, if the ObjectReference includes GVK (Group Version Kind) information, we could potentially eliminate the ImageBuilderType field here since I see that as being primarily user-facing.
There was a problem hiding this comment.
Ok, so instead of having individual ImageBuilderRef's, I'll just make it one optional field here to encompass all potential object references.
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
3 times, most recently
from
7597202
to
ddb8b4e
Compare
| } | ||
|
|
||
| // ObjectReference contains enough information to let you inspect or modify the referred object. | ||
| type ObjectReference struct { |
There was a problem hiding this comment.
Should we also include a UID field here as well?
There was a problem hiding this comment.
We could add one, the corev1 default does have it, but some of the locally defined types do not. Do you see a need for the UID in the status? I thought this was mostly so a user can cross-reference the object from here, and if they need more information, they can look directly at the referenced object (Job)
There was a problem hiding this comment.
After some offline discussion about the use-case I had in mind, I don't think we need the UID after all.
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
from
ddb8b4e
to
dd3b585
Compare
| // +kubebuilder:validation:XValidation:rule=`(self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$'))`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long" | ||
| // +kubebuilder:validation:XValidation:rule=`(self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$'))`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme" | ||
| // +optional | ||
| CurrentImagePullspec string `json:"currentImagePullspec,omitempty"` |
There was a problem hiding this comment.
I feel like it might be useful to have a reference to the currently active MachineOSBuild here. While it is possible to get the currently active MachineOSBuild just from the current image pullspec, doing so is a bit cumbersome; especially if one is an end-user. In practice, I've been using an annotation for this right now, but having a field on the status object would be more intuitive.
Update from PodImageBuilder to JobImageBuilder, and add a MachineOSBuild reference to MachineOSConfig. This matches MCO changes, and add more description around the fields.
yuqi-zhang
force-pushed
the
promote-onclusterbuild
branch
from
dd3b585
to
44656cf
Compare
|
@yuqi-zhang: 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. |
Opened for testing. Based on guidance this is currently the first step: create new v1 API, gate remains off, v1 API is excluded from the image manifests
Also adds in: #2089