Skip to content

BUG 1473527: module ssh-authkey-fingerprints fails Input/output error…#1340

Merged
TheRealFalcon merged 9 commits into
canonical:mainfrom
andrew-lee-1089:fix-bug-1473527
Apr 7, 2022
Merged

BUG 1473527: module ssh-authkey-fingerprints fails Input/output error…#1340
TheRealFalcon merged 9 commits into
canonical:mainfrom
andrew-lee-1089:fix-bug-1473527

Conversation

@andrew-lee-1089
Copy link
Copy Markdown
Contributor

@andrew-lee-1089 andrew-lee-1089 commented Mar 17, 2022

This fixes https://bugs.launchpad.net/ubuntu/+source/cloud-init/+bug/1473527?comments=all

Proposed Commit Message

Don't error if we cannot log to /dev/console

We've seen instances on VMware of serial consoles not being set up
correctly by the kernel, making /dev/ttyS0 not set up correctly, and 
hence /dev/console not writeable to.

In such circumstances, cloud-init should not fail, instead it should 
gracefully fall back to logging to stdout.

The only time cloud-init tries to write to `/dev/console` is in the
`multi_log` command- which is called by the
ssh-authkey-fingerprints module

LP: #1473527

Additional Context

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@andrew-lee-1089
Copy link
Copy Markdown
Contributor Author

Sorry - I need to get agreement that I can sigm the CLA from my company - so this will have to wait a few days.

In the meantime I'd be grateful for any ideas on how to add regression testing for this - it's not immediately obvious to me how to emulate the OSError we are seeing in testing

Comment thread cloudinit/util.py
@TheRealFalcon
Copy link
Copy Markdown
Contributor

@andrew-lee-metaswitch , Given how transient this issue is, I think testing that we haven't regressed any existing use cases is the best we can do. That plus unit testing would be sufficient.

@andrew-lee-1089
Copy link
Copy Markdown
Contributor Author

Thanks @TheRealFalcon - I've written some UTs, it wasn't actually hard in the end (though I still maintain that pytest/unittest and mock is a dark magic with an impenetrable syntax). I've had to alter the non-test code to make it mockable/testable.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@andrew-lee-metaswitch , I've been holding off reviewing this one since it says WIP and you had/have issues with the CLA. Have those issues been resolved? Is this ready for review?

@andrew-lee-1089 andrew-lee-1089 changed the title WIP BUG 1473527: module ssh-authkey-fingerprints fails Input/output error… BUG 1473527: module ssh-authkey-fingerprints fails Input/output error… Apr 4, 2022
@andrew-lee-1089
Copy link
Copy Markdown
Contributor Author

Apologies, @TheRealFalcon , I've now got agreement from the legal team and have signed the CLA.

@andrew-lee-1089
Copy link
Copy Markdown
Contributor Author

One minor thing - flake8 defaults to 79 character lines, whereas black to 80 characters, meaning I can run do_format over my code and be left with lines of length 80. check-in my code and still have flake8 fail - it would be nice if the flake8 and black config were consistent.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Looks good! Just a small request inline.

Comment thread cloudinit/util.py Outdated
@TheRealFalcon
Copy link
Copy Markdown
Contributor

Oh also, regarding,

One minor thing - flake8 defaults to 79 character lines, whereas black to 80 characters, meaning I can run do_format over my code and be left with lines of length 80. check-in my code and still have flake8 fail - it would be nice if the flake8 and black config were consistent.

I tried to reproduce this but couldn't. We also have black set to 79 width:
https://github.com/canonical/cloud-init/blob/main/pyproject.toml#L2

Curious if you can come up with a simple reproducer on main

@andrew-lee-1089
Copy link
Copy Markdown
Contributor Author

Oh also, regarding,

One minor thing - flake8 defaults to 79 character lines, whereas black to 80 characters, meaning I can run do_format over my code and be left with lines of length 80. check-in my code and still have flake8 fail - it would be nice if the flake8 and black config were consistent.

I tried to reproduce this but couldn't. We also have black set to 79 width: https://github.com/canonical/cloud-init/blob/main/pyproject.toml#L2

Curious if you can come up with a simple reproducer on main

Ahh, I'll put this down to me running in an odd venv.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Unit tests failed after my last requested update, so I took the liberty of pushing the fix. Assuming CI passes, this looks good to me now.

Thanks for the contribution!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants