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

Fix #461 && unify upgrade images #507

Closed
wants to merge 2 commits into from
Closed

Fix #461 && unify upgrade images #507

wants to merge 2 commits into from

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Aug 10, 2021

Looks like we were not creating the destination dir for the extract so it
was failing to upgrade. Removing the switch as its no longer necessary.

Unify upgrade images into a single test, extract image name and image
version into vars to be reused.

Closes #481

Signed-off-by: Itxaka igarcia@suse.com

Itxaka added 2 commits August 10, 2021 20:05
Looks like we were not creating the destinatio dir for the extract so it
was failing to upgrade. Removing the switch as its no longer neccesary

Signed-off-by: Itxaka <igarcia@suse.com>
Unify upgrade images into a single test, extract image name and image
version into vars to be reused.

This will also help with CI as we cna only run 5 macos instances in
parallel so reducing the number of workers to run the tests will speed
up the full workflow

Signed-off-by: Itxaka <igarcia@suse.com>
@Itxaka Itxaka requested review from mudler and dragonchaser August 10, 2021 18:09
@Itxaka Itxaka changed the title Unify upgrade images Fix #461 && unify upgrade images Aug 10, 2021
@Itxaka Itxaka enabled auto-merge (squash) August 10, 2021 22:50
@@ -165,7 +165,6 @@ upgrade() {
# FIXME: Define default /var/tmp as tmpdir_base in default luet config file
export XDG_RUNTIME_DIR=$temp_upgrade
export TMPDIR=$temp_upgrade
export LUET_PRIVILEGED_EXTRACT=true
Copy link
Contributor

@mudler mudler Aug 11, 2021

Choose a reason for hiding this comment

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

that looks more like a regression, when I've tested this it was not failing to upgrade at all - but it succeeded in the upgrade - the failing aspect was vagrant ssh not working with pubkeys. If now it works without pubkeys (because we login with user/pass), doesn't necessarly mean its bug free in there :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to check if after an upgrade you can still ssh with the vagrant pubkeys?

Copy link
Contributor

Choose a reason for hiding this comment

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

see here: #260 (comment) and #260 (comment)

My test was simply mimicking CI:

  • install
  • do upgrade (which succeeded, and I could boot just fine afterwards)
  • try to vagrant ssh with a priv/pubkey that was previously there already (setted up via cloud-init)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned on the issue we dont use vagrant keys AFAIK, when you do vagrant ssh it uses the user/pass in the vagrantfile

Maybe when it happened we were not using the ssh user/pass ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, original issue had vagrant using its own method for the ssh keys while this PR c3d5ba2#diff-225f3d44285006fdfb0e6a444342ca7f84f2b8f366ae15fa4360265b535be386 changed that to user/pass to avoid spending time in injecting the keys....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this was failing without this patch still, so it should be valid. Doesn't fix #481 thought... :D

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's why I'm asking to test this with ssh keys 🙂 the problem was not present by logging with user/pass, but it was there only while logging in via ssh keys...

@Itxaka Itxaka disabled auto-merge August 11, 2021 08:11
@Itxaka Itxaka marked this pull request as draft August 11, 2021 08:11
@Itxaka
Copy link
Contributor Author

Itxaka commented Aug 11, 2021

superseeded by #509

@Itxaka Itxaka closed this Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set LUET_PRIVILEGED_EXTRACT to false make our test suite to fail
2 participants