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

Update autoinstall-reference.rst #1933

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ben-ba
Copy link

@ben-ba ben-ba commented Mar 11, 2024

Missing hyphen for uri will result in an error, if it is the only value in list.

error: { 'uri': 'http://archive.ubuntu.com/ubuntu/' } is not of type 'array'

Missing hypen for uri will result in an error, if it is the only value in list.

error: { 'uri': 'http://archive.ubuntu.com/ubuntu/' } is not of type 'array'
Copy link
Member

@ogayot ogayot left a comment

Choose a reason for hiding this comment

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

Hello and thank you for your contribution!

However, I think the change would introduce invalid autoinstall directives. A arches directive (without a corresponding uri) would be rejected by Subiquity.

Maybe there is something incorrect somewhere in the documentation?

@@ -270,9 +270,9 @@ The default is:
primary:
- country-mirror
- arches: [i386, amd64]
uri: "http://archive.ubuntu.com/ubuntu"
- uri: "http://archive.ubuntu.com/ubuntu"
Copy link
Member

Choose a reason for hiding this comment

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

The change does not look right to me. The key called "primary" expects an array of elements, where each element can either be "country-mirror" or an object with a URI + optionally a set of architectures if applies to.

If we translate the YAML into JSON, we have:

primary:
  - arches: [i386, amd64]
    uri: http://archive.ubuntu.com/ubuntu

equivalent to

{
   "primary": [
        {
            "arches": ["i386", "amd64"],
            "uri": "http://archive.ubuntu.com/ubuntu"
        }
    ]
}

and

primary:
  - arches: [i386, amd64]
  - uri: "http://archive.ubuntu.com/ubuntu"

equivalent to

{
   "primary": [
        {
            "arches": ["i386", "amd64"]
        }, {
            "uri": "http://archive.ubuntu.com/ubuntu"
        }
    ]
}

Copy link
Author

@ben-ba ben-ba Mar 11, 2024

Choose a reason for hiding this comment

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

  1. if you scroll down in the documentation you will see the uri already with an hyphen. -> https://canonical-subiquity.readthedocs-hosted.com/en/latest/reference/autoinstall-reference.html#geoip
  2. If i use arches + uri the hyphen for uri isn't necessary, but if i use only uri, the error mentioned get thrown.

my user-data looks like following for apt;
working one

apt:
    mirror-selection:
      primary:
         - uri: "http://archive.ubuntu.com/ubuntu/"
    fallback: offline-install

failed one, error see 1 comment

apt:
    mirror-selection:
      primary:
         uri: "http://archive.ubuntu.com/ubuntu/"
    fallback: offline-install

Copy link
Author

@ben-ba ben-ba Mar 11, 2024

Choose a reason for hiding this comment

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

got it;

primary is an array of country-mirror and an object with two properties; uri and arches

so each list needs an hyphen, so when uri is needed, the documentation should switch to the following;

why? with the following example in the docuentation u couldn't get in trouble if you only use the necessary property of the list.

primary:
          - country-mirror
          - uri: "http://archive.ubuntu.com/ubuntu"
            arches: [i386, amd64]

Copy link
Contributor

Choose a reason for hiding this comment

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

if you scroll down in the documentation you will see the uri already with an hyphen.

Right, the issue in the proposed change is having arches on its own line with a hyphen.

- uri: my_uri
  arches: [amd64]

and

- arches: [amd64]
  uri: my_uri

are semantically equivalent. The primary key should be an array of items. Either containing the string country-mirror or an object which has a string property uri and (optionally) an array of arches arches which list the supported architectures for that uri.


The reason your second example fails is because it's missing the hyphen. In the following example:

primary:
    uri: "http://archive.ubuntu.com/ubuntu/"

primary is an object with some string property uri. Whereas in:

  primary:
     - uri: "http://archive.ubuntu.com/ubuntu/"

primary is an array of objects with one item. Where that one item has a string property uri.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't refreshed and didn't see your comment, apologies! I also had to look at the code for this one. Perhaps we should update the docs to better reflect what's expected.

Copy link
Member

Choose a reason for hiding this comment

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

@ben-ba I'd be happy to accept this as a doc improvement. Would you like to update this PR with the switched arches / uri statements?

Copy link
Member

@ogayot ogayot Mar 12, 2024

Choose a reason for hiding this comment

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

We'd also need you to sign the CLA before merging the change. We have some instructions on how to do so:

https://github.com/canonical/subiquity/blob/main/CONTRIBUTING.md#contributor-license-agreement

Copy link
Author

Choose a reason for hiding this comment

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

PR updated, CLA signed.

Copy link
Author

@ben-ba ben-ba Mar 15, 2024

Choose a reason for hiding this comment

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

I would be really helpfully to read something about the validation of a autoinstall configuration with
./scripts/validate-autoinstall-user-data.py ../autoinstall/simple/user-data

Complaining is to easy, so i made an PR for this.
#1941

Copy link
Contributor

@Chris-Peterson444 Chris-Peterson444 left a comment

Choose a reason for hiding this comment

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

I'm okay with the change, but I'll wait for another agree to mark approved.

Also, it looks like the CLA checker isn't happy. Could you confirm the email / github account you used to sign it is the same as this one? If it is, it may just be a little slow to update so we can try again later.

@ben-ba
Copy link
Author

ben-ba commented Mar 16, 2024

E-Mail address is the same, but maybe it's an issue, because it is hidden in my github account?!

@ben-ba
Copy link
Author

ben-ba commented Mar 18, 2024

its weird or confusing...

Let me explane what my goal is.
My goal is to have a simple autoinstall setup with all (that means include security <- important) updates installed from an local apt mirror.
To reach this, i started with autoinstall or be exact, with subiquity.
After testing some configs, my finale experience is the following.

  1. do never ever mix up curtin style and subiquity style of config for apt.
  2. an optional subiquity parameter like arches isn't an optional parameter for curtin
  3. docs are never perfect like https://github.com/canonical/curtin/blob/54cc7d980b9300339e773dc579daa5f11fd12fd0/examples/apt-source.yaml#L100 (missing hypen, missing arches key...)

whats the final suggestions from an 'user' perspective;
make it clear in the docs, that the key security won't work if you use mirror-selection or maybe other keys (not tested)

finally here an overview of different configs which are not working - not really clear by documentation or/and no error with the validation script (./scripts/validate-autoinstall-user-data.py ../autoinstall/simple/user-data)

------attempt 1------
no validation error, error when cloud-init is loading -> finish: subiquity/Mirror/load_autoinstall_data: argument of type 'NoneType' is not iterable.

  apt:
    preserve_sources_list: false
    primary:
      - uri: http://linuxmirror.local/ubuntu

------attempt 2------
no validation error, subiquity follows the own recommendation (see (https://wiki.ubuntu.com/SecurityTeam/FAQ#What_repositories_and_pockets_should_I_use_to_make_sure_my_systems_are_up_to_date.3F and (https://www.phoronix.com/news/Ubuntu-Security-Pocket-Issue) to not edit the security update source, but remember, we use curtin configuration inside subiquity, subiquity never overwrite security update source, but curtin says,

security is optional, if not defined it is set to the same value as primary

so my expectation was, security would be also linuxmirror...

  apt:
    preserve_sources_list: false
    primary:
      - uri: http://linuxmirror.local/ubuntu
        arches: [ default ]

------attempt 3------
curtin key security mixing with subiquity mirror-selection, no verification error, not working. but wait, arrrr we forget the arches key for curtins security... but we get a new error, autoinstall process starts, and then the installer crashed...

  apt:
    preserve_sources_list: false
    mirror-selection:
      primary:
      - uri: http://linuxmirror.local/ubuntu
    security:
      - uri: http://linuxmirror.local/ubuntu
    fallback: offline-install

------attempt 4------
curtin key security mixing with subiquity mirror-selection, no verification error, working

  apt:
    preserve_sources_list: false
    mirror-selection:
      primary:
      - uri: http://linuxmirror.local/ubuntu
    security:
      - uri: http://linuxmirror.local/ubuntu
        arches: [ default ]
    fallback: offline-install

------attempt 5------
plain curtin config, no verification error, working

  apt:
    preserve_sources_list: false
    primary:
      - uri: http://linuxmirror.local/ubuntu
        arches: [ default ]
    security:
      - uri: http://linuxmirror.local/ubuntu
        arches: [ default ]
    fallback: offline-install

@Chris-Peterson444
Copy link
Contributor

A few things:

  • For the CLA, I read through the checker code again it's trying to match your membership to https://github.com/CanonicalContributorAgreement. There may be an issue the automation has with adding you to the organization if the email address you provided is hidden. Perhaps the simplest way to to get around this it to make a launchpad account and resubmit the form with your launchpad ID, which is checked as a backup. The launchpad account has to be associated with the email you provide in the CLA form and also has to be the same as the commit author.

  • I see there are two commits. One with the original suggestion and one with the new. Could you squash the commits down to one with just the new effective changes?

  • For the rest of your comments: Looking at the existing code, it doesn't look like we support security under the mirror-selection key. Perhaps we can be more explicit on what the behavior difference is from when primary is and isn't under that key. Please feel free to provide suggestions if you have any to make the docs clearer. Additionally, I am unsure why attempt 1 and 3 are failing. I would need to dig into the code a little more but in general if you believe you are running into an error where you shouldn't be, please feel free to submit a bug here https://bugs.launchpad.net/subiquity/+filebug and we can try to diagnose the issue a little better there with logs from /var/log/installer/.

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