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.

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

op_mode: T6767: Check latest image version in VRF context for "add system image latest vrf <name>" #4225

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

natali-rs1985
Copy link
Contributor

Change Summary

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe): change lisence file

Related Task(s)

Related PR(s)

Component(s) name

op mode

Proposed changes

How to test

set system update-check url https://raw.githubusercontent.com/vyos/vyos-rolling-nightly-builds/main/version.json
commit

vyos@vyos# run add system image  latest vrf TEST
Validating signature
Signature is valid
Validating image compatibility
Validating image checksums
What would you like to name this image? (Default: 1.5-rolling-202412060007) Would you like to set the new image as the default one for boot? [Y/n]
An active configuration was found. Would you like to copy it to the new image? [Y/n]
Copying configuration directory
Would you like to copy SSH host keys? [Y/n]
Copying SSH host keys
Copying system image files
Cleaning up
Unmounting target filesystems
Removing temporary files
...

Smoketest result

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

Copy link

github-actions bot commented Dec 6, 2024

👍
No issues in PR Title / Commit Title

src/op_mode/image_installer.py Outdated Show resolved Hide resolved
@jestabro
Copy link
Contributor

As mentioned on Slack, this should be rebased over current and pushed to sync with the changes in
#4226
vyos/vyos-build#824

A local build of this PR confirms a successful build.

@c-po
Copy link
Member

c-po commented Dec 10, 2024

@Mergifyio rebase

@jestabro
Copy link
Contributor

I have not found a 'mergifyio rebase' to suffice in the past; this will likely need an explicit rebase and force push to the branch.

Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

I wonder if it's a good idea to show REMOTE_USERNAME and REMOTE_PASSWORD on the CLI on error

The image cannot be fetched from: https://github.com/vyos/vyos-nightly-build/releases/download/1.5-rolling-202412100007/vyos-1.5-rolling-202412100007-generic-amd64.iso [Errno 1] failed to run command: REMOTE_USERNAME= REMOTE_PASSWORD=                 ip vrf exec red /usr/libexec/vyos/simple-download.py                 --local-file /tmp/vyos_installation.iso --remote-path https://github.com/vyos/vyos-nightly-build/releases/download/1.5-rolling-202412100007/vyos-1.5-rolling-202412100007-generic-amd64.iso
returned: HTTPSConnectionPool(host='github.com', port=443): Max retries exceeded with url: /vyos/vyos-nightly-build/releases/download/1.5-rolling-202412100007/vyos-1.5-rolling-202412100007-generic-amd64.iso (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7fe011080a90>: Failed to establish a new connection: [Errno 16] Device or resource busy'))

Copy link

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests (no interfaces) ❌ failed
  • CLI Smoketests (interfaces only) ❌ failed
  • Config tests ❌ failed
  • RAID1 tests ❌ failed
  • TPM tests ❌ failed

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I agree that this solution fixes the immediate problem. I still think we can do it better, though.

First, do we really need to execute the update check script as a subprocess? I think we could import it as a Python module and call a function from it instead.

Second, executing something in a VRF context is a need common enough that I believe we should add a named argument (vrf) to everything in vyos.utils.subprocess. That part should be a separate task, though.

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I don't mind merging it right now since it solves a problem but we need to address the deeper issue of VRF-awareness nonetheless.

@c-po c-po merged commit 11e89b1 into vyos:current Dec 18, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants