✨ OpenStackImage controller #2130
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
api/v1alpha1/openstackimage_types.go
Outdated
|
|
||
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=10 | ||
| type ImageContainerFormat string |
There was a problem hiding this comment.
Is this actually an enum? Or can this vary OSP to OSP? Formats typically are enums
There was a problem hiding this comment.
It is an enum, but unfortunately strictly speaking I think it's variable. Same for disk format.
In practise nobody's ever actually going to use this at all, only disk format. I almost took it out entirely.
I may revisit this once I've got an initial implementation. Perhaps be opinionated with a pragmatic set of formats (for both this and disk format).
api/v1alpha1/openstackimage_types.go
Outdated
|
|
||
| // +kubebuilder:validation:MinLength:=1 | ||
| // +kubebuilder:validation:MaxLength:=255 | ||
| type ImageTag string |
There was a problem hiding this comment.
Can this be regexed or refined to a particular character set?
There was a problem hiding this comment.
The docs don't specify one, only a maximum length. I'd have to check the code to determine if there are any other practical constraints. It's not impossible, as the docs suck.
@stephenfin can you think of any reason this wouldn't just be utf-8?
api/v1alpha1/openstackimage_types.go
Outdated
| // +kubebuilder:validation:MaxLength:=10 | ||
| type ImageDiskFormat string | ||
|
|
||
| // kubebuilder:validation:Enum:=public;private;shared;community |
There was a problem hiding this comment.
Kube APIs use PascalCase for enums, even if the backing implementation needs a translation
There was a problem hiding this comment.
Ack. That would presumably also apply to the hash algorithms.
There was a problem hiding this comment.
FYI I initially made this change, then after I'd lived with it for a while I reverted it. I did this because I found myself having to write conversions for every one of these in code, and realising that users will also mentally have to do the same thing. This is a proxy API, so these strings only have meaning in OpenStack glance, which is the documentation a user is most likely to be looking at when working out what to put here. If I Pascal-ise them, they also need to mentally translate the glance value to whatever I picked here.
I think it's a case of 'pick what you're going to be inconsistent with'. I think following the API we're proxying is less bad here.
api/v1alpha1/openstackimage_types.go
Outdated
| // kubebuilder:validation:Enum:=public;private;shared;community | ||
| type ImageVisibility string | ||
|
|
||
| // kubebuilder:validation:Enum:=md5;sha1;sha256;sha512 |
api/v1alpha1/openstackimage_types.go
Outdated
| ImageHashAlgorithmSHA512 ImageHashAlgorithm = "sha512" | ||
| ) | ||
|
|
||
| // +kubebuilder:validation:XValidation:rule="!in(self, ['created_at', 'updated_at', 'status', 'checksum', 'size', 'virtual_size', 'direct_url', 'self', 'file', 'schema', 'id', 'os_hash_algo', 'os_hash_value', 'location', 'deleted', 'deleted_at', 'container_format', 'disk_format', 'min_disk', 'min_ram', 'name', 'tags', 'owner', 'visibility', 'protected', 'os_hidden'])" |
There was a problem hiding this comment.
Would be better to use Kube like constants and translate to OSP like constants on the backend
There was a problem hiding this comment.
These are truly free-form. They're just going to be merged without translation into the OpenStack request.
As discussed I'll look into removing this entirely in favour of an opinionated set of properties, at least until a customer comes to us with a request for an escape hatch.
There was a problem hiding this comment.
I decided to remove this entirely in favour of an opinionated set. I considered leaving AdditionalProperties as an escape hatch, but I'm going to try without it for now.
api/v1alpha1/openstackimage_types.go
Outdated
| // DiskFormat is the format of the disk image. | ||
| // Normal values are "qcow2", or "raw". Glance may be configured to support others. | ||
| // +optional | ||
| DiskFormat *ImageDiskFormat `json:"diskFormat,omitempty"` |
api/v1alpha1/openstackimage_types.go
Outdated
| // Protected specifies that the image is protected from deletion. | ||
| // If not specified, the default is false. | ||
| // +optional | ||
| Protected *bool `json:"protected,omitempty"` |
There was a problem hiding this comment.
Eww, bools. Suggest to use an enum instead that could be expanded in the future if, for example, you needed to support something like ProtectedWithPassword or ProtectedWithPrivateKey or something
There was a problem hiding this comment.
This is a bool in the glance API, and it actually means 'Not Deletable'. I think this one's ok.
api/v1alpha1/openstackimage_types.go
Outdated
|
|
||
| // Tags is a list of tags which will be applied to the image. A tag has a maximum length of 255 characters. | ||
| // +optional | ||
| Tags []ImageTag `json:"tags,omitempty"` |
There was a problem hiding this comment.
Need an SSA listType tag, is this a map list with a key? Else mark explicitly as atomic
api/v1alpha1/openstackimage_types.go
Outdated
| // SizeB is the size of the image data, in bytes | ||
| SizeB *int64 `json:"sizeB,omitempty"` |
There was a problem hiding this comment.
Why not make this something that's going to be more human readable? Bytes will be a large number, what about KB or something larger with a small amount of rounding?
There was a problem hiding this comment.
I actually think all APIs should use bytes for everything and translate only on input or display.
For this, it's a value that's directly given to us by the underlying API. I'd probably leave it alone in case anybody is expecting to be able to do a comparison with it.
api/v1alpha1/openstackimage_types.go
Outdated
| // +patchStrategy=merge | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` |
There was a problem hiding this comment.
Idiomatically the conditions should be the first item in the list. Don't need the protobuf marker on this line either
There was a problem hiding this comment.
I'd wondered about the protobuf thing! Also patchMergeKey and patchStrategy.
I'll take them out, but it's probably worth noting that I copied them from the docstring of the Condition struct: https://github.com/kubernetes/kubernetes/blob/7060e485694e82698c050a6a6c6ae2af20cf5e57/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1517-L1528
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
mdbooth
force-pushed
the
imagecontroller
branch
2 times, most recently
from
32f1a02
to
fc5c870
Compare
|
/test pull-cluster-api-provider-openstack-e2e-test |
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
mdbooth
force-pushed
the
imagecontroller
branch
3 times, most recently
from
006641e
to
d68dda6
Compare
mdbooth
changed the title
|
We spent some time with Matt pair reviewing this PR.
Otherwise this is really good work, I think this is ready to go and we can iterate later. Thanks for this huge effort! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
mdbooth
force-pushed
the
imagecontroller
branch
10 times, most recently
from
a6de868
to
4420a30
Compare
k8s-ci-robot
added
the
needs-rebase
mdbooth
force-pushed
the
imagecontroller
branch
from
4420a30
to
c47d01f
Compare
k8s-ci-robot
removed
the
needs-rebase
mdbooth
force-pushed
the
imagecontroller
branch
from
c47d01f
to
2f05b8c
Compare
This commits creates a new home for ORC, initially incubated in CAPO. The initial implementation of ORC has an image controller. The image controller is integrated with OpenStackServer and from there into both OpenStackMachine and the bastion. The E2E tests are updated to use the new image controller. Images are now loaded dynamically by the test suite.
mdbooth
force-pushed
the
imagecontroller
branch
from
2f05b8c
to
3090a6c
Compare
|
@pierreprinetti would you mind LGTM if you agree with this PR? |
|
Thank you @mdbooth for your initiative of coding the OpenStackImage API as a building block of ORC. I see that this PR goes out of its way to make the future migration of this code into its natural repository as painless as possible, and I do see benefit in it being incubated here. As discussed out of band:
With this, I am happy to leave my |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
A new controller for creating glances images. Will initially support creation
from a remote URL including:
It supports both glance-direct and web-download, although because the feature set of web-download is limited it will only be used if no hash verification or decompression is required.
This controller uses server-side apply and uses applyconfigurations generated by code-gen added in #2133.
The controller is implemented as the first of a new generation of https://k-orc.cloud/ controller. We also incubate the ORC project as a subdirectory of CAPO until it's ready to be split out.
TODO:
/hold