Conversation
GoryMoon
left a comment
There was a problem hiding this comment.
I just had a quick look and noticed some things that I have questions about
go.mod
Outdated
| github.com/gobwas/glob v0.2.3 | ||
| github.com/google/go-cmp v0.7.0 | ||
| github.com/google/uuid v1.6.0 | ||
| github.com/gophercloud/gophercloud v1.14.1 |
There was a problem hiding this comment.
I noticed you are using a v1 legacy version of gophercloud (last version was over a year ago) rather than the current stable v2 version.
Is there a specific reason, or just missed that there is a more current version?
If there is no specific reason, here is a migration guide to v2 to help update: https://github.com/gophercloud/gophercloud/blob/main/docs/MIGRATING.md
There was a problem hiding this comment.
Thank you for noticing. There wasn't any specific reason, just my mistake. I will update the code to use v2
| // This is needed otherwise we get the following error when authenticating: | ||
| // You must provide exactly one of DomainID or DomainName to | ||
| // authenticate by Username | ||
| // Even with an RC file that works perfectly fine with `openstack token issue` |
There was a problem hiding this comment.
This seems to be related to this issue: gophercloud/gophercloud#3440
TL;DR It currently only uses the OS_DOMAIN_NAME env instead of OS_PROJECT_DOMAIN_NAME and OS_USER_DOMAIN_NAME env variables.
There was a problem hiding this comment.
I linked the issue from within the code comment.
pkg/cloud/openstack/openstack.go
Outdated
| return fmt.Errorf("Failed to authenticate to OpenStack: %w", err) | ||
| } | ||
|
|
||
| client, err := ostack.NewImageServiceV2(provider, gophercloud.EndpointOpts{}) |
There was a problem hiding this comment.
I'm not familiar with OpenStack, but from looking at some RC examples it can contain a region. It might be good to include the functionality here as well.
The example on gophercloud has the following code:
openstack.NewImageV2(ctx, providerClient, gophercloud.EndpointOpts{
Region: os.Getenv("OS_REGION_NAME"),
})
pkg/cloud/openstack/openstack.go
Outdated
| @@ -0,0 +1,89 @@ | |||
| //go:build cgo | |||
There was a problem hiding this comment.
This build constraint will make this feature only available with cgo; I don't think gophercloud requires that, so it's unnecessary here.
There was a problem hiding this comment.
I took the libvirt.go as a template and forgot to remove this like. Good catch, thank you.
mvo5
left a comment
There was a problem hiding this comment.
Thank you! Looks good, tiny nitpicks/suggestions inline. Fixing the cgo thing would be great too.
| } | ||
|
|
||
| func (ou *openstackUploader) Check(status io.Writer) error { | ||
| return nil |
There was a problem hiding this comment.
Might be nice to do some of the checks we do in UploadAndRegister here already. Not a blocker though.
There was a problem hiding this comment.
To be honest, I am confused about the purpose of the Check method. My expectation was that if the check fails, we don't even try to upload. Which doesn't seem to be true. I return errors from my Check and UploadAndRegister gets called anyway.
There was a problem hiding this comment.
Thanks, interesting. Then I must misremember how this works, its fine for now, we can always refactor it. The idea was that we would run "Check()" before we do an expensive upload and then discover that some permissions to move the image into the right place are missing (iirc that can happen on AWS). But if you say its not working for you we will investigate. Its easy enough to move some of the checks here in a followup.
There was a problem hiding this comment.
I agree, adding the checks later should be easy. Check this out.
func (ou *openstackUploader) Check(status io.Writer) error {
return errors.New("FAILED UPLOAD")
}The upload starts happening
[jkadlcik@hive image-builder]$ ./image-builder upload --to openstack ~/git/copr-image-builder/output/image/disk.raw --arch x86_64 --openstack-image frostyx_copr_hv_x86_64_01_prod_06740723_20250913_1418098
Uploading to OpenStack...
0 B / 10.00 GiB [________________________________________________________________________________________________________________________________________________________________________________________________________________] 0.00% ? p/s
And it's not a glitch because the "Uploading to OpenStack..." is there also. So we are definitely inside of the UploadAndRegister function.
| // authenticate by Username | ||
| // Even with an RC file that works perfectly fine with `openstack token issue` | ||
| if opts.DomainName == "" { | ||
| opts.DomainName = os.Getenv("OS_USER_DOMAIN_NAME") |
There was a problem hiding this comment.
Should we double check that os.Getenv("OS_USER_DOMAIN_NAME") != "" ?
There was a problem hiding this comment.
I don't think we should. If it is "" then it will fail with
error: Failed to authenticate to OpenStack: You must provide exactly one of DomainID or DomainName to authenticate by Username
which is not ideal but it is better than failing because it is empty even though the upload might maybe work without that? I am not sure what happens on other OpenStack instances.
But if you prefer to have the check, I can definitely add it.
In Copr, we upload our builder images to OSUOSL OpenStack using the `glance image-create` command. I think it would be really cool if we could use `image-builder upload` instead.
Changes with 0.205.0 ---------------- - Convert the test config map to a config list (osbuild/images#1900) - Author: Achilleas Koutsou, Reviewers: Sanne Raymaekers, Simon de Vlieger - Repos: Add definitions for AlmaLinux 9.7, 9.8, 10.1 and 10.2 (osbuild/images#1926) - Author: Eduard Abdullin, Reviewers: Achilleas Koutsou, Brian C. Lane, Simon de Vlieger - Support uploading to OpenStack (osbuild/images#1921) - Author: Jakub Kadlčík, Reviewers: Michael Vogt, Simon de Vlieger - disk: make addPartitionsForBootMode() arch specific (osbuild/images#1928) - Author: Michael Vogt, Reviewers: Brian C. Lane, Simon de Vlieger - distro: add ova image type for bootc [HMS-9503] (osbuild/images#1938) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger - fedora/minimal: drop uuids in partition tables (osbuild/images#1932) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt Changes with 0.206.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1945) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - deps: bump blueprint to 1.16.0 (osbuild/images#1952) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Sanne Raymaekers - fedora: Drop tigervnc on F42 and later (osbuild/images#1942) - Author: Brian C. Lane, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: document root kickstart (osbuild/images#1936) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt - fedora: rawhide is 44 (osbuild/images#1943) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Tomáš Hozza - many: lorax template split (HMS-9524) (osbuild/images#1949) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Tomáš Hozza Changes with 0.207.0 ---------------- - Enable fedora 43 unit testing (osbuild/images#1954) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: update cloud_kernel_options (osbuild/images#1953) - Author: Sanne Raymaekers, Reviewers: Achilleas Koutsou, Simon de Vlieger - test/data/repos/rhel-10.2: fix copy & paste error (osbuild/images#1956) - Author: Tomáš Hozza, Reviewers: Achilleas Koutsou, Simon de Vlieger Changes with 0.208.0 ---------------- - Schutzfile: switch CI runner to Fedora 42 (osbuild/images#1955) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - Support uploading to IBM Cloud (osbuild/images#1924) - Author: Jakub Kadlčík, Reviewers: Achilleas Koutsou, Simon de Vlieger - pkg/osbuild: generate osbuild result from status scanner entries (osbuild/images#1941) - Author: Sanne Raymaekers, Reviewers: Nobody
Changes with 0.205.0 ---------------- - Convert the test config map to a config list (osbuild/images#1900) - Author: Achilleas Koutsou, Reviewers: Sanne Raymaekers, Simon de Vlieger - Repos: Add definitions for AlmaLinux 9.7, 9.8, 10.1 and 10.2 (osbuild/images#1926) - Author: Eduard Abdullin, Reviewers: Achilleas Koutsou, Brian C. Lane, Simon de Vlieger - Support uploading to OpenStack (osbuild/images#1921) - Author: Jakub Kadlčík, Reviewers: Michael Vogt, Simon de Vlieger - disk: make addPartitionsForBootMode() arch specific (osbuild/images#1928) - Author: Michael Vogt, Reviewers: Brian C. Lane, Simon de Vlieger - distro: add ova image type for bootc [HMS-9503] (osbuild/images#1938) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger - fedora/minimal: drop uuids in partition tables (osbuild/images#1932) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt Changes with 0.206.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1945) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - deps: bump blueprint to 1.16.0 (osbuild/images#1952) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Sanne Raymaekers - fedora: Drop tigervnc on F42 and later (osbuild/images#1942) - Author: Brian C. Lane, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: document root kickstart (osbuild/images#1936) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt - fedora: rawhide is 44 (osbuild/images#1943) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Tomáš Hozza - many: lorax template split (HMS-9524) (osbuild/images#1949) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Tomáš Hozza Changes with 0.207.0 ---------------- - Enable fedora 43 unit testing (osbuild/images#1954) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: update cloud_kernel_options (osbuild/images#1953) - Author: Sanne Raymaekers, Reviewers: Achilleas Koutsou, Simon de Vlieger - test/data/repos/rhel-10.2: fix copy & paste error (osbuild/images#1956) - Author: Tomáš Hozza, Reviewers: Achilleas Koutsou, Simon de Vlieger Changes with 0.208.0 ---------------- - Schutzfile: switch CI runner to Fedora 42 (osbuild/images#1955) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - Support uploading to IBM Cloud (osbuild/images#1924) - Author: Jakub Kadlčík, Reviewers: Achilleas Koutsou, Simon de Vlieger - pkg/osbuild: generate osbuild result from status scanner entries (osbuild/images#1941) - Author: Sanne Raymaekers, Reviewers: Nobody
Changes with 0.205.0 ---------------- - Convert the test config map to a config list (osbuild/images#1900) - Author: Achilleas Koutsou, Reviewers: Sanne Raymaekers, Simon de Vlieger - Repos: Add definitions for AlmaLinux 9.7, 9.8, 10.1 and 10.2 (osbuild/images#1926) - Author: Eduard Abdullin, Reviewers: Achilleas Koutsou, Brian C. Lane, Simon de Vlieger - Support uploading to OpenStack (osbuild/images#1921) - Author: Jakub Kadlčík, Reviewers: Michael Vogt, Simon de Vlieger - disk: make addPartitionsForBootMode() arch specific (osbuild/images#1928) - Author: Michael Vogt, Reviewers: Brian C. Lane, Simon de Vlieger - distro: add ova image type for bootc [HMS-9503] (osbuild/images#1938) - Author: Michael Vogt, Reviewers: Achilleas Koutsou, Simon de Vlieger - fedora/minimal: drop uuids in partition tables (osbuild/images#1932) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt Changes with 0.206.0 ---------------- - Update osbuild dependency commit ID to latest (osbuild/images#1945) - Author: SchutzBot, Reviewers: Achilleas Koutsou, Simon de Vlieger - deps: bump blueprint to 1.16.0 (osbuild/images#1952) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Sanne Raymaekers - fedora: Drop tigervnc on F42 and later (osbuild/images#1942) - Author: Brian C. Lane, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: document root kickstart (osbuild/images#1936) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Michael Vogt - fedora: rawhide is 44 (osbuild/images#1943) - Author: Simon de Vlieger, Reviewers: Achilleas Koutsou, Tomáš Hozza - many: lorax template split (HMS-9524) (osbuild/images#1949) - Author: Simon de Vlieger, Reviewers: Brian C. Lane, Tomáš Hozza Changes with 0.207.0 ---------------- - Enable fedora 43 unit testing (osbuild/images#1954) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - fedora: update cloud_kernel_options (osbuild/images#1953) - Author: Sanne Raymaekers, Reviewers: Achilleas Koutsou, Simon de Vlieger - test/data/repos/rhel-10.2: fix copy & paste error (osbuild/images#1956) - Author: Tomáš Hozza, Reviewers: Achilleas Koutsou, Simon de Vlieger Changes with 0.208.0 ---------------- - Schutzfile: switch CI runner to Fedora 42 (osbuild/images#1955) - Author: Achilleas Koutsou, Reviewers: Simon de Vlieger, Tomáš Hozza - Support uploading to IBM Cloud (osbuild/images#1924) - Author: Jakub Kadlčík, Reviewers: Achilleas Koutsou, Simon de Vlieger - pkg/osbuild: generate osbuild result from status scanner entries (osbuild/images#1941) - Author: Sanne Raymaekers, Reviewers: Nobody
In Copr, we upload our builder images to OSUOSL OpenStack using the
glance image-createcommand. I think it would be really cool if we could useimage-builder uploadinstead.