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

Support LUKS with 'lvm' autoinstall storage layout #1579

Merged
merged 2 commits into from
Mar 3, 2023

Conversation

rmounce
Copy link
Contributor

@rmounce rmounce commented Mar 1, 2023

To support the same behaviour that was available with d-i/preseed.

@dbungert
Copy link
Collaborator

dbungert commented Mar 1, 2023

Thanks for the PR Ryan! I am supportive of landing something like this, but we'll have to tune the details. I had something else in mind for the autoinstall part, I'll get back to you on that.

Looks like this broke CI, have a look at which test is failing if you would. A unit test on subiquity/server/controllers/tests/test_filesystem.py would be nice, TestGuidedV2 has some similar things.

To proceed I will need you to take a look at https://ubuntu.com/legal/contributors

@dbungert
Copy link
Collaborator

dbungert commented Mar 1, 2023

2023-03-01 23:40:50,231 DEBUG root:37 finish: subiquity/Filesystem/guided_POST: SUCCESS: 500 Traceback (most recent call last):                                                                                                         
  File "/home/dbungert/dev/github/canonica...                                                                       
2023-03-01 23:40:50,231 DEBUG subiquity.server.server:456 request to /storage/guided crashed                        Traceback (most recent call last):                                                                                  
  File "/home/dbungert/dev/github/canonical/subiquity/subiquity/common/api/server.py", line 134, in handler         
    result = await implementation(**args)                                                                           
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                           
  File "/home/dbungert/dev/github/canonical/subiquity/subiquity/server/controllers/filesystem.py", line 604, in guided_POST                                                                                                             
    self.guided(GuidedChoiceV2.from_guided_choice(data))                                                            
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                             
  File "/home/dbungert/dev/github/canonical/subiquity/subiquity/common/types.py", line 447, in from_guided_choice   
    use_all_space=choice.use_all_space,                                                                             
                  ^^^^^^^^^^^^^^^^^^^^                                                                              
AttributeError: 'GuidedChoice' object has no attribute 'use_all_space'                                              

@dbungert
Copy link
Collaborator

dbungert commented Mar 1, 2023

make install_deps lint check can help if you want to run it locally. Beware that's going to install a bunch of things.

@rmounce
Copy link
Contributor Author

rmounce commented Mar 1, 2023

make install_deps lint check can help if you want to run it locally. Beware that's going to install a bunch of things.

Thanks, sorry for the CI spam. I've now signed the CLA so not sure if that needs manual review by legal, DB still needs to sync or what I'm missing there.

Had a quick look at the tests in subiquity/server/controllers/tests/test_filesystem.py. Maybe it's going right over my head, I can't see any existing tests that could be spliced together to check:

  • Whether LUKS is being used
  • The sizing of a LV within a PV

Looks like the existing test suite passes for just my first commit which is trivially exposing the existing GUI LUKS option via autoinstall. Are you happy with the approach for this bit?

I'm inclined to open a separate PR for the use_all_space bit once it's passing tests.

@dbungert
Copy link
Collaborator

dbungert commented Mar 2, 2023

I've now signed the CLA so not sure if that needs manual review by legal, DB still needs to sync or what I'm missing there.

Yea, I'm sure that'll sort itself out in a day.

So I ran this branch through an autoinstall, with the following as input:

version: 1
identity:
    hostname: ai-test
    password: "$y$j9T$UdY22v4Kexn/AZcIzBSUc0$2DnuvWDSwoDCFPlbk1ghZeT2qFrEBKY1bpFQoexHdw7"
    username: ubuntu
storage:
    layout:
        name: lvm
        password: passw0rd

Then grepped the output. It would be best if we didn't log the contents of storage.layout.password, looks like there are 3 places that do so.

58:2023-03-02 03:11:53,211 DEBUG subiquity.server.controllers.filesystem:161 load_autoinstall_data {'layout': {'name': 'lvm', 'password': 'passw0rd'}}
59:2023-03-02 03:11:53,211 DEBUG subiquity.server.controllers.filesystem:162 self.ai_data = {'layout': {'name': 'lvm', 'password': 'passw0rd'}}
236:2023-03-02 03:11:53,639 DEBUG subiquity.server.controllers.filesystem:925 self.ai_data = {'layout': {'name': 'lvm', 'password': 'passw0rd'}}

Just remove those log statements for now and I'm good to land this.

Copy link
Collaborator

@dbungert dbungert left a comment

Choose a reason for hiding this comment

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

This is all fine but github's action runners are throwing some error I've never seen before, I'd prefer to let them settle first.

@dbungert dbungert merged commit 86e7fbb into canonical:main Mar 3, 2023
@dbungert
Copy link
Collaborator

dbungert commented Mar 3, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants