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

Auto register #919

Merged
merged 34 commits into from
Dec 14, 2023
Merged

Auto register #919

merged 34 commits into from
Dec 14, 2023

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Dec 7, 2023

Problem

It is not possible to register system using automatic installation.

trello: https://trello.com/c/K94qhHWT/3499-3-agama-add-support-to-register-the-system-using-auto-installation

Solution

Implement it and when touching it also add support for selecting patterns.

Testing

  • Tested manually
  • for unit testing I need to learn how to mock properly in rust

@coveralls
Copy link

coveralls commented Dec 7, 2023

Coverage Status

coverage: 74.78% (-0.3%) from 75.055%
when pulling 9b8f086 on auto_register
into 3953469 on master.

Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Reviewing the Product Client+Store
Still todo: Software Client+Store

rust/agama-lib/share/profile.schema.json Show resolved Hide resolved
rust/agama-lib/src/product.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/product/client.rs Outdated Show resolved Hide resolved

match result {
(0, _) => Ok(()),
(3, description) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers??!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was already in previous code. I think it is defined in dbus API

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it is existing code, but seeing that Register and Deregister have similar yet different sets of result codes

The bigger problem is that these errors, including "network error" and "timeout error" are being merged below at line 74... this will bite us in bug reports.

https://github.com/openSUSE/agama/blob/39534694aed2b3ce941e4c4b896051ab1cd0660b/doc/dbus/org.opensuse.Agama1.Registration.doc.xml#L19C1-L57C1

rust/agama-lib/src/product/client.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/product/store.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/product/store.rs Outdated Show resolved Hide resolved
if let Some(email) = &settings.registration_email {
self.product_client.register(reg_code, email).await?;
} else {
self.product_client.register(reg_code, "").await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to myself: check that email data elsewhere is documented as (non)empty

})
}

pub async fn store(&self, settings: &ProductSettings) -> Result<(), ServiceError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method will happily register something even if no product is selected. Do we want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, if it is single product medium, you do not need to select it...but error handling is good question. I am not sure if it should be done here or earlier during some kind of validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imobachgs what is your opinion?

@@ -80,6 +80,23 @@ impl From<HashMap<String, String>> for SettingObject {
}
}

impl From<String> for SettingObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should get better documentation, because Settings is hard to understand now already, with generic words like Object and Value all around, plus the metaprogramming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, @imobachgs as this is your code and also your patch, I think you will be better to properly document what it do.

@jreidinger jreidinger marked this pull request as ready for review December 8, 2023 09:16
jreidinger and others added 3 commits December 8, 2023 10:22
Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

In Patterns it turns out I am bothered by not propagating our API documentation through all the layers that present it, which becomes acute when the missing docs in question is the meaning of magic numbers.

It does not affect correctness of the code but it increases its poor maintainablilty

.product_proxy
.available_products()
/// Returns the available patterns
pub async fn patterns(&self, filtered: bool) -> Result<Vec<Pattern>, ServiceError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

filtered is not documented here nor in https://github.com/openSUSE/agama/blob/39534694aed2b3ce941e4c4b896051ab1cd0660b/doc/dbus/org.opensuse.Agama.Software1.doc.xml#L8

Filtered according to what?

Not your fault ;-) but let's fix it now

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in b75a6e1

rust/agama-lib/src/software/client.rs Outdated Show resolved Hide resolved
Ok(self.product_proxy.selected_product().await?)
/// Returns the selected patterns by user
pub async fn user_selected_patterns(&self) -> Result<Vec<String>, ServiceError> {
const USER_SELECTED: u8 = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

rust/agama-lib/src/software/settings.rs Outdated Show resolved Hide resolved
@mvidner
Copy link
Contributor

mvidner commented Dec 8, 2023

Please show me how you tested manually so that I can replicate (or run into errors, the error paths should be fun too)

Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

My manual test loses information:

# grep -A3 product.*: rust/agama-lib/share/examples/profile_Dolomite.json
  "product": {
    "id": "ALP-Dolomite",
    "registrationCode": "FILL IT UP",
    "registrationEmail": "jreidinger@suse.com"
# (cd rust/; cargo run --bin agama -- -f yaml config load agama-lib/share/examples/profile_Dolomite.json)
# (cd rust/; cargo run --bin agama -- -f yaml config show)
[...]
product:
  id: ALP-Dolomite
  registrationCode: ''
  registrationEmail: ''

But Josef says it is actually working as designed: config show talks to our D-Bus layer which only changes email once Register is called. Hmm I think CLI users will find it confusing.

@mvidner
Copy link
Contributor

mvidner commented Dec 8, 2023

Not a fault of this PR, but agama config add seems to have never worked:

rust # ./target/debug/agama -f yaml config add -h            
Add an element to a collection

Usage: agama config add <KEY> [VALUES]...

Arguments:
  <KEY>        
  [VALUES]...  

Options:
  -h, --help  Print help
rust # ./target/debug/agama -f yaml config add software.patterns gnome
Missing the '=' separator in 'gnome'
rust # ./target/debug/agama -f yaml config add software.patterns=gnome
Unknown attribute 'software.patterns=gnome'
rust # ./target/debug/agama -f yaml config add software patterns=gnome
thread 'main' panicked at agama-cli/src/config.rs:56:44:
called `Result::unwrap()` on an `Err` value: InvalidKeyName("software")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
rust # ./target/debug/agama -f yaml config add software patterns gnome
Missing the '=' separator in 'patterns'
rust # ./target/debug/agama -f yaml config add nosuch.software
thread 'main' panicked at agama-cli/src/config.rs:56:44:
called `Result::unwrap()` on an `Err` value: "Unknown section"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@mvidner
Copy link
Contributor

mvidner commented Dec 8, 2023

Error reporting

When I agama config load profile_Dolomite.json, it says "Unknown Registration Code" correctly, because our sample code is bogus...
But it does so in the service log, which is in the journal, or in daemon.log when I run it via (cd service/; bundle exec bin/agamactl -d) &> daemon.log

That is below par. Par for the course is what happens when I config load a profile which is missing a user section:
the CLI then says

Wrong user parameters: '["No username entered.\nTry again.", "The username may contain only\nLatin letters and digits, \"-\", \".\", and \"_\"\nand must begin with a letter or \"_\".\nTry again.", "No password entered.\nTry again."]'

which could still be styled better but does present the problem right away without digging in the logs.

And similar problem with reporting product+patterns errors when I apply the Dolomite profile on a TW system.

jreidinger and others added 2 commits December 8, 2023 22:22
Before:
"Connection to registration server failed"

After:
"Connection to registration server failed: Unknown Registration Code"
Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

tl;dr: This was not enough but I've fixed that myself 💪

(result, message) = self.product_client.register(reg_code, "").await?;
}
if result != 0 {
return Err(ServiceError::FailedRegistration(message));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

... ... but ... my testing shows:

# agama config load agama-lib/share/examples/profile_Dolomite.json
Registration failed: 'Connection to registration server failed'

So far so good, right?

# tail -n2 daemon.log
Announcing system to https://scc.suse.com ...
[ERROR]: software: Error connecting to registration server: Unknown Registration Code.

So the CLI would have me debug network connections instead of checking for typos in the regcode. We still need to improve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I quickly insert the integer result code into FailedRegistration it is revealed to be 6 which means "API error" which is a disappointment, I hoped to see 8 "incorrect credentials".

Copy link
Contributor

Choose a reason for hiding this comment

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

aaand I have fixed it in our Ruby backend 😄

Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Adding the error reporting has revealed this, which makes the whole call fail:

# rust/target/debug/agama config load agama-lib/share/examples/profile_tw.json
Registration failed: 'Product does not require registration'

profile_tw.json does not include registrationCode, the CLI is forcing it:

  "product": {
    "id": "Tumbleweed"
  },

@mvidner
Copy link
Contributor

mvidner commented Dec 11, 2023

Not a fault of this PR, but agama config add seems to have never worked:

On Imo's advice,
agama config add software.patterns value=gnome
has worked, but IMHO it's wrongly exposing an implementation detail.

Also, we should update the help text, rust/agama-cli/doc/CLI_API.md and doc/cli_guidelines.md
And fix the panics

@mvidner
Copy link
Contributor

mvidner commented Dec 11, 2023

Pattern selection errors are logged only the second time the profile is loaded, and hidden from the CLI.

profile_tw_crocheting.json is copied from profile_tw.json but the gnome patterns is changed to a (sadly) non-existent crocheting pattern to test pattern selection errors.

First run of "agama config load agama-lib/share/examples/profile_tw_crocheting.json" ``` 13929 dbus-daemon[13929]: Activating service name='org.opensuse.Agama.Manager1' requested by ':1.0' (uid=0 pid=13935 comm="target/debug/agama -f yaml config load agama-lib/s") [INFO]: manager: Starting the service Loading mocked module /checkout/service/lib/agama/dbus/y2dir/manager/modules/Package.rb dbus-daemon[13929]: Activating service name='org.opensuse.Agama.Software1' requested by ':1.1' (uid=0 pid=13948 comm="ruby /checkout/service/bin/agamactl manager") [INFO]: software: Starting the service Loading mocked module /checkout/service/lib/agama/dbus/y2dir/software/modules/PackageCallbacks.rb /usr/lib64/ruby/vendor_ruby/3.2.0/yast/scr.rb:113: warning: undefining the allocator of T_DATA class Yast::YCode Loading mocked module /checkout/service/lib/agama/dbus/y2dir/software/modules/SpaceCalculation.rb [INFO]: software: Reading configuration from /checkout/service/etc/agama.yaml dbus-daemon[13929]: Activating service name='org.opensuse.Agama1' requested by ':1.2' (uid=0 pid=13952 comm="ruby /checkout/service/bin/agamactl software") 08:48:26 �[0m�[34m[INFO] �[0mStarted questions interface 08:48:29 �[0m�[34m[INFO] �[0mStarted locale interface 08:48:29 �[0m�[34m[INFO] �[0mStarted network interface dbus-daemon[13929]: Successfully activated service 'org.opensuse.Agama1' 08:48:29 �[0m�[33m[WARN] �[0mIgnoring network device 'lo' (unsupported type '32') 08:48:29 �[0m�[33m[WARN] �[0mIgnoring network device 'tap0' (unsupported type '16') 08:48:29 �[0m�[34m[INFO] �[0mPublishing network connection 'lo' 08:48:29 �[0m�[34m[INFO] �[0mPublishing network connection 'Ethernet network device 1' dbus-daemon[13929]: Successfully activated service 'org.opensuse.Agama.Software1' [INFO]: software: Exported /org/opensuse/Agama/Software1, /org/opensuse/Agama/Software1/Product, /org/opensuse/Agama/Software1/Proposal objects Loading mocked module /checkout/service/lib/agama/dbus/y2dir/manager/modules/PackagesProposal.rb [INFO]: manager: Reading configuration from /checkout/service/etc/agama.yaml dbus-daemon[13929]: [INFO]: manager: Exported /org/opensuse/Agama/Manager1, /org/opensuse/Agama/Users1 objects Successfully activated service 'org.opensuse.Agama.Manager1' dbus-daemon[13929]: Activating service name='org.opensuse.Agama.Storage1' requested by ':1.4' (uid=0 pid=13935 comm="target/debug/agama -f yaml config load agama-lib/s") [INFO]: manager: Startup phase done [INFO]: storage: Starting the service Loading mocked module /checkout/service/lib/agama/dbus/y2dir/manager/modules/Package.rb Loading mocked module /checkout/service/lib/agama/dbus/y2dir/manager/modules/PackagesProposal.rb [INFO]: storage: Reading configuration from /checkout/service/etc/agama.yaml dbus-daemon[13929]: Successfully activated service 'org.opensuse.Agama.Storage1' [INFO]: storage: Exported /org/opensuse/Agama/Storage1 objects 08:48:34 �[0m�[31m[ERROR] �[0mCould not add/update the connection Ethernet network device 1: D-Bus service error 08:48:34 �[0m�[33m[WARN] �[0mIgnoring network device 'lo' (unsupported type '32') 08:48:34 �[0m�[33m[WARN] �[0mIgnoring network device 'tap0' (unsupported type '16') 08:48:34 �[0m�[34m[INFO] �[0mPublishing network connection 'lo' 08:48:34 �[0m�[34m[INFO] �[0mPublishing network connection 'Ethernet network device 1' [INFO]: software: Selecting product Tumbleweed [INFO]: manager: Probing Storage (1/2) [INFO]: storage: Activating storage devices (1/4) [INFO]: storage: Activating iSCSI [INFO]: storage: Probing storage devices (2/4) [INFO]: storage: Probing iSCSI [INFO]: storage: Calculating the storage proposal (3/4) [INFO]: storage: Selecting Linux Security Modules (4/4) [INFO]: manager: Probing Software (2/2) [INFO]: software: Probing software [INFO]: software: removing /etc/zypp/repos.d [INFO]: software: Initializing target repositories (1/4) [INFO]: software: Importing GPG keys: ["/usr/lib/rpm/gnupg/keys/gpg-pubkey-29b700a4-62b07e22.asc", "/usr/lib/rpm/gnupg/keys/gpg-pubkey-39db7c82-5f68629b.asc"] [INFO]: software: Initializing sources (2/4) [INFO]: software: Refreshing repositories metadata (3/4) [INFO]: software: Calculating the software proposal (4/4) I, [2023-12-11T08:48:54.960678 #13952] INFO -- : Selecting the base product 'openSUSE' I, [2023-12-11T08:48:56.884719 #13952] INFO -- : Solver run true [INFO]: software: Proposal result: true [INFO]: manager: Config phase done [INFO]: software: Setting patterns to ["crocheting"] [INFO]: software: Solver run true [INFO]: manager: Setting first user Jane Doe [INFO]: manager: Setting Root Password [INFO]: storage: Calculating storage proposal from D-Bus. D-Bus settings: {"BootDevice"=>"/dev/dm-1"} Agama settings: # ```
Second run of "agama config load agama-lib/share/examples/profile_tw_crocheting.json"
08:52:31 �[0m�[31m[ERROR] �[0mCould not add/update the connection Ethernet network device 1: D-Bus service error
08:52:31 �[0m�[33m[WARN] �[0mIgnoring network device 'lo' (unsupported type '32')
08:52:31 �[0m�[33m[WARN] �[0mIgnoring network device 'tap0' (unsupported type '16')
08:52:31 �[0m�[34m[INFO] �[0mPublishing network connection 'lo'
08:52:31 �[0m�[34m[INFO] �[0mPublishing network connection 'Ethernet network device 1'
[INFO]: manager: Probing Storage (1/2)
[INFO]: storage: Activating storage devices (1/4)
[INFO]: storage: Activating iSCSI
[INFO]: storage: Probing storage devices (2/4)
[INFO]: storage: Probing iSCSI
[INFO]: storage: Calculating the storage proposal (3/4)
[INFO]: storage: Selecting Linux Security Modules (4/4)
[INFO]: manager: Probing Software (2/2)
[INFO]: software: Probing software
[INFO]: software: Refreshing repositories metadata (1/2)
[INFO]: software: Calculating the software proposal (2/2)
I, [2023-12-11T08:52:37.040132 #13952]  INFO -- : Selecting the base product 'openSUSE'
Error: Failed to select default product patterns:
crocheting
Patterns have not been found.

This can probably be fixed by adding
more installation repositories by going back to
the 'Registration' or 'Add On Product' screens.
I, [2023-12-11T08:52:39.088720 #13952]  INFO -- : Solver run true
[INFO]: software: Proposal result: false
[INFO]: manager: Config phase done
[INFO]: software: Setting patterns to ["crocheting"]
[INFO]: software: Solver run true
[INFO]: manager: Setting first user Jane Doe
[INFO]: manager: Setting Root Password
[INFO]: storage: Calculating storage proposal from D-Bus.
 D-Bus settings: {"BootDevice"=>"/dev/dm-1", "LVM"=>false}
Agama settings: #<Agama::Storage::ProposalSettings:0x00007fe5eae5d350 ...>

@jreidinger
Copy link
Contributor Author

Well, reason is that it is well hidden that pattern is missing. Code is from https://github.com/yast/yast-packager/blob/4e7687c61f165140248a8045149dc67cd547ebf1/src/modules/Packages.rb#L1907

@jreidinger
Copy link
Contributor Author

Ok, so code computes it is basically
https://github.com/yast/yast-packager/blob/4e7687c61f165140248a8045149dc67cd547ebf1/src/modules/Packages.rb#L1850
Question is what dbus API to adapt? Add/Remove/Set Patterns? Or just Set Patterns as it works on set of patterns? We need to modify its dbus API to report issue, so we will be able to display it. I do not expect that web UI is affected as it display only known and valid patterns.
Another option is to not modify DBus API and instead check list of all patterns ( so unfiltered ) in rust code and report it before calling those actions.
@imobachgs @mvidner opinions what will be better?

@imobachgs
Copy link
Contributor

Ok, so code computes it is basically https://github.com/yast/yast-packager/blob/4e7687c61f165140248a8045149dc67cd547ebf1/src/modules/Packages.rb#L1850 Question is what dbus API to adapt? Add/Remove/Set Patterns? Or just Set Patterns as it works on set of patterns? We need to modify its dbus API to report issue, so we will be able to display it. I do not expect that web UI is affected as it display only known and valid patterns. Another option is to not modify DBus API and instead check list of all patterns ( so unfiltered ) in rust code and report it before calling those actions. @imobachgs @mvidner opinions what will be better?

As usual, I am not sure, but I think that the D-Bus API should be responsible for reporting that problem. Actually, I would expect all of them (Add/Remove/Set) to be able to inform us when a pattern does not exist. I do not know how hard it could be.

About error reporting, perhaps you could redefine the "Report" module for this case (as we did with other modules).

@mvidner
Copy link
Contributor

mvidner commented Dec 12, 2023

In general, D-Bus API can stay unchanged, as long as you are able to raise a D-Bus error with a single string description.
If the error needs more structured data, D-Bus protocol still allows that but tooling (D-Feet) gets confused, so an API change is better.

@jreidinger
Copy link
Contributor Author

@mvidner wrong pattern is now properly reported. See this screenshot from my VM:
agama_pattern_not_exist

Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Thanks!

Errors get reported to the CLI user now

Co-authored-by: Martin Vidner <mvidner@suse.cz>
@jreidinger jreidinger merged commit c374ec9 into master Dec 14, 2023
6 checks passed
@jreidinger jreidinger deleted the auto_register branch December 14, 2023 12:54
@imobachgs imobachgs mentioned this pull request Dec 21, 2023
imobachgs added a commit that referenced this pull request Dec 21, 2023
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.

4 participants