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

Fixes #35270 - Enable boot image download #837

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixes #35270 - Enable boot image download #837

wants to merge 1 commit into from

Conversation

bastian-src
Copy link

Manage ISO images as Installation media. Refers to this PR.

Includes the following changes:

  • Implement fetch and extract boot image
  • Apply correct file permissions
  • Implement classes for file extraction and permission changes

ToDo: Add tests.

@bastian-src bastian-src mentioned this pull request Jul 25, 2022
Closed
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 1dcf666 to 5abea3c Compare July 26, 2022 09:40
ekohl
ekohl reviewed Jul 26, 2022
View reviewed changes
lib/proxy/filesystem_permission.rb Outdated Show resolved Hide resolved
lib/proxy/archive_extract.rb Outdated Show resolved Hide resolved
modules/tftp/server.rb Outdated Show resolved Hide resolved
lib/proxy/archive_extract.rb Outdated Show resolved Hide resolved
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 5abea3c to 9fdcb20 Compare July 27, 2022 15:21
@bastian-src bastian-src mentioned this pull request Jul 27, 2022
Closed
ekohl
ekohl reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

We should also expose this as a capability. In https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html I've explained this, but this allows the Foreman side to know if the API is supported. You could even look up the presence of 7z and only present the capability if it's present. That's another way of dealing with it.

Of course the Foreman side also needs to be modified to respect this.

modules/tftp/server.rb Outdated Show resolved Hide resolved
modules/tftp/tftp_api.rb Outdated Show resolved Hide resolved
lib/proxy/archive_extract.rb Outdated Show resolved Hide resolved
test/tftp/tftp_test.rb Outdated Show resolved Hide resolved
modules/tftp/server.rb Outdated Show resolved Hide resolved
lzap
lzap reviewed Aug 2, 2022
View reviewed changes
modules/tftp/server.rb Outdated Show resolved Hide resolved
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 9fdcb20 to 3a4bdf8 Compare August 16, 2022 14:11
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 3a4bdf8 to 83d6c14 Compare August 16, 2022 14:16
ekohl
ekohl reviewed Aug 17, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I left some comments about the command task. Perhaps ArchiveExtract shouldn't call super at all in start but rather implement its own logic.

lib/proxy/util.rb Outdated Show resolved Hide resolved
modules/tftp/server.rb Outdated Show resolved Hide resolved
lib/proxy/archive_extract.rb Outdated Show resolved Hide resolved
lib/proxy/archive_extract.rb Outdated Show resolved Hide resolved
lib/proxy/util.rb Outdated Show resolved Hide resolved
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 83d6c14 to 3a40fce Compare August 19, 2022 09:40
@bastian-src
Copy link
Author

We should also expose this as a capability. In https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html I've explained this, but this allows the Foreman side to know if the API is supported. You could even look up the presence of 7z and only present the capability if it's present. That's another way of dealing with it.

Of course the Foreman side also needs to be modified to respect this.

I had a look at your comment and I think it makes totally sense to have such a capability handling for this feature. I just added this commit from what I see from other modules (like the bmc plugin here) and your blog post. Can you maybe give me an example how the capability is checked on the Foreman-side? I would implement it then before triggering the iso image download API.

@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 4ec8d4d to 0f04cee Compare August 19, 2022 13:19
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 0f04cee to 6aa3f2a Compare August 20, 2022 10:28
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 7a0027a to 4f38afe Compare September 28, 2022 09:34
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch 6 times, most recently from 02d6874 to 691f12c Compare September 30, 2022 10:17
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 691f12c to 45c6d78 Compare November 10, 2022 09:39
@bastian-src
Copy link
Author

@ekohl coming back to this. I've added tests for the tftp_boot_image_api with respect to the tests of httpboot. The tests run fine when executing them on-by-one. Unfortunately, my tests fail by checking the capabilites here when I want to execute all tests at once. I think the which function is not available in the testing context. Is there an approach to mock this for testing?

Moreover, I'd like to know whether we could also trigger the bootimage_download API endpoint within a Foreman job task. I think this might make it more easy to handle concurrency?

@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 45c6d78 to 7713fbc Compare November 14, 2022 12:33
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 7713fbc to 0c50d94 Compare November 14, 2022 14:32
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch 3 times, most recently from 2fa7469 to a4fddf8 Compare November 28, 2022 15:38
@bastian-src
Copy link
Author

bastian-src commented Nov 28, 2022 •
edited
Loading

@ekohl tests are green.

The /fetch_system_image API endpoint is not a dangling API call anymore. It triggers another thread which downloads the OS image file and extracts the boot files accordingly. Moreover, I tried to make the API endpoint concurrency safe by including a lock. The endpoint returns a status accordingly:

  • 200: image and boot files exist already - no need to do anything
  • 202: any of the files does not exist - process triggered
  • 423: another download/extraction process is still running (resource locked)
  • 500: any other error (input error for example)

What do you think about it? Can we continue with the foreman-side of this feature?

This was referenced Dec 12, 2022
Closed
Open
Open
@bastian-src
Copy link
Author

@ekohl I've also added PRs for foreman-installer and foreman-packaging due to the new settings variable and package dependency.

@bastian-src bastian-src mentioned this pull request Dec 12, 2022
Open
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from a4fddf8 to 4e15e9c Compare March 3, 2023 14:07
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 4e15e9c to 25b60a3 Compare March 28, 2023 12:28
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 25b60a3 to 3b7ab39 Compare June 20, 2023 08:24
@bastian-src @ekohl
Fixes #35270 - Support system image download
a230b04 
* Implement fetch and extract system image
* Implement class for file extraction with isoinfo
* Add capability for archive extraction
* Separate logging and file writing tasks
* Add additional API endpoint /tftp/system_image/

Co-Authored-By: Ewoud Kohl van Wijngaarden <[email protected]>
@bastian-src bastian-src force-pushed the fix_35270_automate_iso_download branch from 3b7ab39 to a230b04 Compare October 17, 2023 09:07
@bastian-src
Copy link
Author

@ekohl do you know why there are no test results? Did I forgot anything, maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-bucher m-bucher m-bucher left review comments

@ekohl ekohl ekohl left review comments

@lzap lzap lzap left review comments

Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bastian-src @ekohl @lzap @m-bucher @theforeman-bot